Re: [Tarantool-patches] [PATCH 1/2] fiber: unref fiber.storage via global Lua state

[Igor Munkin <imun@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