From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp53.i.mail.ru (smtp53.i.mail.ru [94.100.177.113]) (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 D732746971A for ; Fri, 13 Dec 2019 03:02:32 +0300 (MSK) References: <20191210083258.GD21413@atlas> <2c8fe897-9a9d-849d-463e-5fadff982b8c@tarantool.org> <20191211070830.GA5953@atlas> <20191212084603.GA24448@atlas> From: Vladislav Shpilevoy Message-ID: <96368326-84a1-0b3c-15f8-54e3d044db85@tarantool.org> Date: Fri, 13 Dec 2019 01:02:31 +0100 MIME-Version: 1.0 In-Reply-To: <20191212084603.GA24448@atlas> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH 2/2] fiber: destroy fiber.storage created by iproto List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Konstantin Osipov , tarantool-patches@dev.tarantool.org Hi! Thanks for the review! On 12/12/2019 09:46, Konstantin Osipov wrote: > * Vladislav Shpilevoy [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? >