From 0cc1c9c9dcea5955f7dab921d9a2fff78c4e1729 Mon Sep 17 00:00:00 2001 From: Benoit Germain Date: Thu, 8 Nov 2018 17:32:05 +0100 Subject: Make sure any linda operation that can raise an error won't ever leave a mutex unreleased --- CHANGES | 4 +++ docs/index.html | 2 +- src/keeper.c | 11 ++++-- src/keeper.h | 1 + src/lanes.c | 99 ++++++++++++++++++++++++++++++++---------------------- tests/deadlock.lua | 28 +++++++++++++++ 6 files changed, 101 insertions(+), 44 deletions(-) create mode 100644 tests/deadlock.lua diff --git a/CHANGES b/CHANGES index 228038b..a90358b 100644 --- a/CHANGES +++ b/CHANGES @@ -1,5 +1,9 @@ CHANGES: +CHANGE 133: BGe 8-Nov-18 + * Make sure any linda operation that can raise an error won't ever leave a mutex unreleased + * lane:join() now returns nil, "timeout" in case of timeout + CHANGE 132: BGe 7-Nov-18 * __lanesclone mechanism should actually work now diff --git a/docs/index.html b/docs/index.html index 9c76bef..ef227c8 100644 --- a/docs/index.html +++ b/docs/index.html @@ -855,7 +855,7 @@

- Waits until the lane finishes, or timeout seconds have passed. Returns nil on timeout, nil,err,stack_tbl if the lane hit an error, nil, "killed" if forcefully killed (starting with v3.3.0), or the return values of the lane. + Waits until the lane finishes, or timeout seconds have passed. Returns nil, "timeout" on timeout (since v3.13), nil,err,stack_tbl if the lane hit an error, nil, "killed" if forcefully killed (starting with v3.3.0), or the return values of the lane. Unlike in reading the results in table fashion, errors are not propagated.

diff --git a/src/keeper.c b/src/keeper.c index 715583b..b67bee2 100644 --- a/src/keeper.c +++ b/src/keeper.c @@ -188,7 +188,7 @@ static void push_table( lua_State* L, int idx_) int keeper_push_linda_storage( Universe* U, lua_State* L, void* ptr_, ptrdiff_t magic_) { - Keeper* const K = keeper_acquire( U->keepers, magic_); + Keeper* const K = which_keeper( U->keepers, magic_); lua_State* const KL = K ? K->L : NULL; if( KL == NULL) return 0; STACK_GROW( KL, 4); @@ -233,7 +233,6 @@ int keeper_push_linda_storage( Universe* U, lua_State* L, void* ptr_, ptrdiff_t STACK_END( L, 1); lua_pop( KL, 1); // STACK_END( KL, 0); - keeper_release( K); return 1; } @@ -715,6 +714,14 @@ void init_keepers( Universe* U, lua_State* L) STACK_END( L, 0); } +// should be called only when inside a keeper_acquire/keeper_release pair (see linda_protected_call) +Keeper* which_keeper(Keepers* keepers_, ptrdiff_t magic_) +{ + int const nbKeepers = keepers_->nb_keepers; + unsigned int i = (unsigned int)((magic_ >> KEEPER_MAGIC_SHIFT) % nbKeepers); + return &keepers_->keeper_array[i]; +} + Keeper* keeper_acquire( Keepers* keepers_, ptrdiff_t magic_) { int const nbKeepers = keepers_->nb_keepers; diff --git a/src/keeper.h b/src/keeper.h index 37922fb..60410da 100644 --- a/src/keeper.h +++ b/src/keeper.h @@ -29,6 +29,7 @@ typedef struct s_Keepers Keepers; void init_keepers( Universe* U, lua_State* L); void close_keepers( Universe* U, lua_State* L); +Keeper* which_keeper( Keepers* keepers_, ptrdiff_t magic_); Keeper* keeper_acquire( Keepers* keepers_, ptrdiff_t magic_); #define KEEPER_MAGIC_SHIFT 3 void keeper_release( Keeper* K); diff --git a/src/lanes.c b/src/lanes.c index c3e64fb..72f9dd6 100644 --- a/src/lanes.c +++ b/src/lanes.c @@ -453,6 +453,34 @@ static void check_key_types( lua_State* L, int start_, int end_) } } +LUAG_FUNC( linda_protected_call) +{ + int rc = LUA_OK; + struct s_Linda* linda = lua_toLinda( L, 1); + + // acquire the keeper + Keeper* K = keeper_acquire( linda->U->keepers, LINDA_KEEPER_HASHSEED(linda)); + lua_State* KL = K ? K->L : NULL; // need to do this for 'STACK_CHECK' + if( KL == NULL) return 0; + + // retrieve the actual function to be called and move it before the arguments + lua_pushvalue( L, lua_upvalueindex( 1)); + lua_insert( L, 1); + // do a protected call + rc = lua_pcall( L, lua_gettop( L) - 1, LUA_MULTRET, 0); + + // release the keeper + keeper_release( K); + + // if there was an error, forward it + if( rc != LUA_OK) + { + return lua_error( L); + } + // return whatever the actual operation provided + return lua_gettop( L); +} + /* * bool= linda_send( linda_ud, [timeout_secs=-1,] [linda.null,] key_num|str|bool|lightuserdata, ... ) * @@ -514,7 +542,7 @@ LUAG_FUNC( linda_send) { bool_t try_again = TRUE; Lane* const s = get_lane_from_registry( L); - Keeper* K = keeper_acquire( linda->U->keepers, LINDA_KEEPER_HASHSEED( linda)); + Keeper* K = which_keeper( linda->U->keepers, LINDA_KEEPER_HASHSEED( linda)); lua_State* KL = K ? K->L : NULL; // need to do this for 'STACK_CHECK' if( KL == NULL) return 0; STACK_CHECK( KL); @@ -578,10 +606,8 @@ LUAG_FUNC( linda_send) } } STACK_END( KL, 0); - keeper_release( K); } - // must trigger error after keeper state has been released if( pushed < 0) { return luaL_error( L, "tried to copy unsupported types"); @@ -676,7 +702,7 @@ LUAG_FUNC( linda_receive) { bool_t try_again = TRUE; Lane* const s = get_lane_from_registry( L); - Keeper* K = keeper_acquire( linda->U->keepers, LINDA_KEEPER_HASHSEED( linda)); + Keeper* K = which_keeper( linda->U->keepers, LINDA_KEEPER_HASHSEED( linda)); if( K == NULL) return 0; for( ;;) { @@ -735,10 +761,8 @@ LUAG_FUNC( linda_receive) } } } - keeper_release( K); } - // must trigger error after keeper state has been released if( pushed < 0) { return luaL_error( L, "tried to copy unsupported types"); @@ -779,8 +803,7 @@ LUAG_FUNC( linda_set) check_key_types( L, 2, 2); { - Keeper* K = keeper_acquire( linda->U->keepers, LINDA_KEEPER_HASHSEED( linda)); - if( K == NULL) return 0; + Keeper* K = which_keeper( linda->U->keepers, LINDA_KEEPER_HASHSEED( linda)); if( linda->simulate_cancel == CANCEL_NONE) { @@ -813,7 +836,6 @@ LUAG_FUNC( linda_set) push_unique_key( L, CANCEL_ERROR); pushed = 1; } - keeper_release( K); } // must trigger any error after keeper state has been released @@ -835,10 +857,8 @@ LUAG_FUNC( linda_count) check_key_types( L, 2, lua_gettop( L)); { - Keeper* K = keeper_acquire( linda->U->keepers, LINDA_KEEPER_HASHSEED( linda)); - if( K == NULL) return 0; + Keeper* K = which_keeper( linda->U->keepers, LINDA_KEEPER_HASHSEED( linda)); pushed = keeper_call( linda->U, K->L, KEEPER_API( count), L, linda, 2); - keeper_release( K); if( pushed < 0) { return luaL_error( L, "tried to count an invalid key"); @@ -864,8 +884,7 @@ LUAG_FUNC( linda_get) // make sure the key is of a valid type (throws an error if not the case) check_key_types( L, 2, 2); { - Keeper* K = keeper_acquire( linda->U->keepers, LINDA_KEEPER_HASHSEED( linda)); - if( K == NULL) return 0; + Keeper* K = which_keeper( linda->U->keepers, LINDA_KEEPER_HASHSEED( linda)); if( linda->simulate_cancel == CANCEL_NONE) { @@ -881,9 +900,7 @@ LUAG_FUNC( linda_get) push_unique_key( L, CANCEL_ERROR); pushed = 1; } - keeper_release( K); - // must trigger error after keeper state has been released - // (an error can be raised if we attempt to read an unregistered function) + // an error can be raised if we attempt to read an unregistered function if( pushed < 0) { return luaL_error( L, "tried to copy unsupported types"); @@ -913,8 +930,7 @@ LUAG_FUNC( linda_limit) check_key_types( L, 2, 2); { - Keeper* K = keeper_acquire( linda->U->keepers, LINDA_KEEPER_HASHSEED( linda)); - if( K == NULL) return 0; + Keeper* K = which_keeper( linda->U->keepers, LINDA_KEEPER_HASHSEED( linda)); if( linda->simulate_cancel == CANCEL_NONE) { @@ -932,7 +948,6 @@ LUAG_FUNC( linda_limit) push_unique_key( L, CANCEL_ERROR); pushed = 1; } - keeper_release( K); } // propagate pushed boolean if any return pushed; @@ -948,15 +963,11 @@ LUAG_FUNC( linda_cancel) { struct s_Linda* linda = lua_toLinda( L, 1); char const* who = luaL_optstring( L, 2, "both"); - Keeper* K; + Keeper* K = which_keeper( linda->U->keepers, LINDA_KEEPER_HASHSEED( linda)); // make sure we got 3 arguments: the linda, a key and a limit luaL_argcheck( L, lua_gettop( L) <= 2, 2, "wrong number of arguments"); - // signalling must be done from inside the K locking area - K = keeper_acquire( linda->U->keepers, LINDA_KEEPER_HASHSEED( linda)); - if( K == NULL) return 0; - linda->simulate_cancel = CANCEL_SOFT; if( strcmp( who, "both") == 0) // tell everyone writers to wake up { @@ -976,14 +987,6 @@ LUAG_FUNC( linda_cancel) SIGNAL_ALL( &linda->read_happened); } else - { - // error ... - linda = NULL; - } - keeper_release( K); - - // ... but we must raise it outside the lock - if( !linda) { return luaL_error( L, "unknown wake hint '%s'", who); } @@ -1184,18 +1187,16 @@ static void* linda_id( lua_State* L, DeepOp op_) struct s_Linda* linda = lua_touserdata( L, 1); ASSERT_L( linda); - /* Clean associated structures in the keeper state. - */ + // Clean associated structures in the keeper state. K = keeper_acquire( linda->U->keepers, LINDA_KEEPER_HASHSEED( linda)); if( K && K->L) // can be NULL if this happens during main state shutdown (lanes is GC'ed -> no keepers -> no need to cleanup) { + // hopefully this won't ever raise an error as we would jump to the closest pcall site while forgetting to release the keeper mutex... keeper_call( linda->U, K->L, KEEPER_API( clear), L, linda, 0); } keeper_release( K); - /* There aren't any lanes waiting on these lindas, since all proxies - * have been gc'ed. Right? - */ + // There aren't any lanes waiting on these lindas, since all proxies have been gc'ed. Right? SIGNAL_FREE( &linda->read_happened); SIGNAL_FREE( &linda->write_happened); free( linda); @@ -1225,34 +1226,46 @@ static void* linda_id( lua_State* L, DeepOp op_) lua_pushcfunction( L, LG_linda_concat); lua_setfield( L, -2, "__concat"); - // [-1]: linda metatable + // protected calls, to ensure associated keeper is always released even in case of error + // all function are the protected call wrapper, where the actual operation is provided as upvalue + // note that this kind of thing can break function lookup as we use the function pointer here and there + lua_pushcfunction( L, LG_linda_send); + lua_pushcclosure( L, LG_linda_protected_call, 1); lua_setfield( L, -2, "send"); lua_pushcfunction( L, LG_linda_receive); + lua_pushcclosure( L, LG_linda_protected_call, 1); lua_setfield( L, -2, "receive"); lua_pushcfunction( L, LG_linda_limit); + lua_pushcclosure( L, LG_linda_protected_call, 1); lua_setfield( L, -2, "limit"); lua_pushcfunction( L, LG_linda_set); + lua_pushcclosure( L, LG_linda_protected_call, 1); lua_setfield( L, -2, "set"); lua_pushcfunction( L, LG_linda_count); + lua_pushcclosure( L, LG_linda_protected_call, 1); lua_setfield( L, -2, "count"); lua_pushcfunction( L, LG_linda_get); + lua_pushcclosure( L, LG_linda_protected_call, 1); lua_setfield( L, -2, "get"); lua_pushcfunction( L, LG_linda_cancel); + lua_pushcclosure( L, LG_linda_protected_call, 1); lua_setfield( L, -2, "cancel"); lua_pushcfunction( L, LG_linda_deep); lua_setfield( L, -2, "deep"); lua_pushcfunction( L, LG_linda_dump); + lua_pushcclosure( L, LG_linda_protected_call, 1); lua_setfield( L, -2, "dump"); + // some constants lua_pushliteral( L, BATCH_SENTINEL); lua_setfield(L, -2, "batched"); @@ -2074,13 +2087,14 @@ static THREAD_RETURN_T THREAD_CALLCONV lane_main( void* vs) // But don't register it in the lookup database because of the s_lane pointer upvalue lua_pushlightuserdata( L, s); lua_pushcclosure( L, LG_set_debug_threadname, 1); - lua_setglobal( L, "set_debug_threadname" ); + lua_setglobal( L, "set_debug_threadname"); // Tie "cancel_test()" to the state lua_pushcfunction( L, LG_cancel_test); populate_func_lookup_table( L, -1, "cancel_test"); lua_setglobal( L, "cancel_test"); + // this could be done in lane_new before the lane body function is pushed on the stack to avoid unnecessary stack slot shifting around #if ERROR_FULL_STACK // Tie "set_error_reporting()" to the state lua_pushcfunction( L, LG_set_error_reporting); @@ -2654,7 +2668,10 @@ LUAG_FUNC( thread_join) bool_t done = THREAD_ISNULL( s->thread) || THREAD_WAIT( &s->thread, wait_secs, &s->done_signal, &s->done_lock, &s->status); if( !done || !L2) { - return 0; // timeout: pushes none, leaves 'L2' alive + STACK_GROW( L, 2); + lua_pushnil( L); + lua_pushliteral( L, "timeout"); + return 2; } STACK_CHECK( L); diff --git a/tests/deadlock.lua b/tests/deadlock.lua new file mode 100644 index 0000000..8ccae89 --- /dev/null +++ b/tests/deadlock.lua @@ -0,0 +1,28 @@ +local lanes = require('lanes').configure() +local linda = lanes.linda "deadlock_linda" + +local do_extra_stuff = true + +if do_extra_stuff then + -- just something to make send() succeed and receive() fail: + local payload = { lanes.require('socket').connect } + + -- lane generator + local g = lanes.gen('*', function() + set_debug_threadname "deadlock_lane" + print("In lane 1:", table.unpack{pcall(linda.receive, linda, 'tmp')}) + print("In lane 2:", table.unpack{pcall(linda.receive, linda, 'tmp')}) + return 33, 55 + end) + + -- send payload twice + linda:send('tmp', payload, payload) + local h = g() + local err, stack = h:join() + print('we reach here and then deadlock', err, stack) +end +-- start lane + +-- wait some +lanes.sleep(2) +print('we never reach here') \ No newline at end of file -- cgit v1.2.3-55-g6feb