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

[Igor Munkin <imun@tarantool.org>]

Vlad,

On 13.12.19, Vladislav Shpilevoy wrote:
> Hi! Thanks for the review!
> 
> On 12/12/2019 00:57, Igor Munkin wrote:
> > 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?
> 
> My follow up patch is about a different bug and I wanted to keep
> these independent fixes separated. That would have been strange,
> if in the second commit I had just silently replaced the old L
> with tarantool_L.
> 

Yes, on second thought I agree with you and Kostja, so here is my LGTM
for this patch.

> > 
> > 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.
> > 
> 
> Agree, here is a new version of this paragraph:
> 
> "
>     There are 3 struct lua_State:
>         tarantool_L - global always alive state;
>         L1 - Lua coroutine of the fiber, created by fiber.create();
>         L2 - Lua coroutine created by that fiber to execute
>              test_storage().
> "

-- 
Best regards,
IM