From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id DAE0046971A for ; Thu, 12 Dec 2019 02:59:12 +0300 (MSK) Date: Thu, 12 Dec 2019 02:57:03 +0300 From: Igor Munkin Message-ID: <20191211235703.GK1214@tarantool.org> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH 1/2] fiber: unref fiber.storage via global Lua state List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org Vlad, Thanks for the patch! I spent some time knee deep into fibers machinery and totally agree with this fix: the most safe approach is "unrefing" storage entity via tarantool_L coro, taking into account the possible unref of the fiber's one and its consequtive collection. However, I don't get why we need to apply these changes, considering your follow-up patch. Could you please provide a bit more detailed rationale? On 08.12.19, Vladislav Shpilevoy wrote: > Fiber.storage is a table, available from anywhere in the fiber. It > is destroyed after fiber function is finished. That provides a > reliable fiber-local storage, similar to thread-local in C/C++. > > But there is a problem that the storage may be created via one > struct lua_State, and destroyed via another. Here is an example: > > function test_storage() > fiber.self().storage.key = 100 > end > box.schema.func.create('test_storage') > _ = fiber.create(function() > box.func.test_storage:call() > end) > > There are 3 struct lua_State: > tarantool_L - global always alive Lua state; > L1 - stack of the fiber, created by fiber.create(); > L2 - stack created by that fiber to execute test_storage(). Minor: Strictly saying, lua_State is a Lua coroutine, and not just a guest stack. Please, consider adjusting the commit message, but feel free to ignore this remark. > > Fiber.storage is created on stack of L2 and referenced by global > LUA_REGISTRYINDEX. Then it is unreferenced from L1 when the fiber > is being destroyed. > > That is generally ok as soon as the storage object is always in > LUA_REGISTRYINDEX, which is shared by all Lua states. > > But soon during destruction of the fiber.storage there will be > only tarantool_L and the original L2. Original L2 may be already > deleted by the time the storage is being destroyed. So this patch > makes unref of the storage via reliable tarantool_L. > > Needed for #4662 > --- > src/lua/fiber.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/lua/fiber.c b/src/lua/fiber.c > index 53ebec9aa..a7b75f9bf 100644 > --- a/src/lua/fiber.c > +++ b/src/lua/fiber.c > @@ -445,7 +445,7 @@ lua_fiber_run_f(MAYBE_UNUSED va_list ap) > /* Destroy local storage */ > int storage_ref = f->storage.lua.ref; > if (storage_ref > 0) > - luaL_unref(L, LUA_REGISTRYINDEX, storage_ref); > + luaL_unref(tarantool_L, LUA_REGISTRYINDEX, storage_ref); > /* > * If fiber is not joinable > * We can unref child stack here, > -- > 2.21.0 (Apple Git-122.2) > -- Best regards, IM