Re: [Tarantool-patches] [PATCH 2/2] fiber: destroy fiber.storage created by iproto

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the review!

On 12/12/2019 09:46, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/12/12 00:27]:
>> - I moved cleanup to iproto.cc, and called it tx_fiber_cleanup().
>>   By analogue with tx_fiber_init() which currently initializes the
>>   fiber;
>>
>> - I made cleanup triggers always clean them by themselves to remove
>>   trigger_destroy() from fiber.c.
>>
>> But it didn't help me to understand, why are you saying, that
>> tx and session should not use these triggers. Tx, in case it was
>> not removed from fiber before its cleanup, should be destroyed.
>> Session uses that trigger to destroy local one-fiber sessions,
>> which die together with the fiber getting recycled/killed. So
>> what is a problem? That trigger perfectly fits all the needs.
> 
> OK, the function itself may be a match, but it doesn't make the
> idea of using the same trigger object for everything right, don't
> you think?

For everything possible to imagine - no. For txn, session, and
language specific fiber-local data - yes. And by coincidence that
is pretty much everything we have now.

> 
> My point is that you do  not only move where the function is
> invoked, but also where the trigger list object is stored.

Nope. The trigger list is still in struct fiber.

> 
> Can you move it to Lua fiber object and iproto_msg object?

I am not sure I understand what you mean. I can't move lua fiber
storage destructor to iproto. I need Lua for that. So this should
be in Lua folder.

If you are talking about moving trigger list to iproto and Lua,
and somehow saying pointer at that list to a member in struct
fiber, then
  1) I would need to patch C version too, because we have public
     C API. The fact you forgot about C API says, that it is not
     a good idea to make that cleanup in each front end. Easy to
     miss something;
  2) That will give exactly the same - a member in struct fiber.
     There just no other communication channel between different
     frontends in which a fiber may participate during lifetime;
  3) We would need to add a new member to struct fiber and
     increase its size.

> There could be no other entry points to the request, so my point
> is that the trigger is part of request state, not fiber state.
'Request' idea is different for session, for txn, for iproto.
You can't define one request for all things.

While you are trying to define a 'request' concept, there is an
already defined concept of fiber function. The function which a
fiber executes. That should be the anchor. This is the 'request'
in terms of fibers. Fiber owns session, fiber owns txn, fiber
owns Lua resources. So it is his task to clean them up. And it
should be in his triggers. On_cleanup is fiber's destructor.

Yes, fiber can hand out these resources, but all access to them
goes through fiber object. Fiber owns them, and fiber should
destroy them when its current task is done. This is fiber's
responsibility.

> 
> Does it make any sense?
>