From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-f66.google.com (mail-lf1-f66.google.com [209.85.167.66]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id C284C46971A for ; Tue, 10 Dec 2019 11:33:00 +0300 (MSK) Received: by mail-lf1-f66.google.com with SMTP id v201so12951504lfa.11 for ; Tue, 10 Dec 2019 00:33:00 -0800 (PST) Date: Tue, 10 Dec 2019 11:32:58 +0300 From: Konstantin Osipov Message-ID: <20191210083258.GD21413@atlas> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org * Vladislav Shpilevoy [19/12/08 23:47]: > static int > -session_on_stop(struct trigger *trigger, void * /* event */) > +session_on_fiber_cleanup(struct trigger *trigger, void * /* event */) Please come up with a better name, and provide a comment. > - trigger_add(&fiber()->on_stop, &s->fiber_on_stop); > + trigger_create(&s->fiber_on_cleanup, session_on_fiber_cleanup, > + NULL, NULL); > + /* Add a trigger to destroy session on fiber cleanup. */ > + trigger_add(&fiber()->on_cleanup, &s->fiber_on_cleanup); I don't understand why these two concerns are intermixed here. Why is this code part of session now? I think if you change the semantics of the trigger, you should move setting/resetting it to the place which controls the relevant object and its life cycle. Specifically, if you change fiber.storage semantics to be per-request *or* per Lua fiber, you should move the trigger to lua fiber create/destroy + iproto request processing start/end. Instead you move it to the core library, which is oblivious of these events. The way the core is implemented may change, and whoever is changing it will have to be aware of the legacy behaviour you make it depend on. > index eff3d7a67..710e4919b 100644 > --- a/src/box/session.h > +++ b/src/box/session.h > @@ -103,8 +103,11 @@ struct session { > union session_meta meta; > /** Session user id and global grants */ > struct credentials credentials; > - /** Trigger for fiber on_stop to cleanup created on-demand session */ > - struct trigger fiber_on_stop; > + /** > + * Trigger for fiber on_cleanup to cleanup created > + * on-demand session. > + */ > + struct trigger fiber_on_cleanup; ... > - trigger_clear(&txn->fiber_on_stop); > + trigger_clear(&txn->fiber_on_cleanup); Please see the above comment. Fiber cleanup trigger, as defined now, does not have anything to do with the session, or fiber, or transaction. It belongs to the request processing layer and should be set/cleared from it. > - struct rlist on_stop; > + /** > + * Triggers invoked before this fiber is > + * stopped/reset/recycled/destroyed/reused. In other > + * words, each time when the fiber has finished execution > + * of a request. > + */ > + struct rlist on_cleanup; Please make the comment describing the timing when this trigger is called less broad and more accurate, calling out the specific places when the trigger is called, and call the trigger accordingly. on_request_or_fiber_end? > +/** Invoke cleanup triggers and delete them. */ > +void > +fiber_cleanup(struct fiber *f); The call for a better name for the event and the accompanying action applies to all of the new api methods this patch introduces. > cmsg_deliver(msg); > + /* > + * Different messages = different requests. They > + * should not affect each other. This is why > + * cleanup is done here. > + */ > + fiber_cleanup(f); Ugh. I think this is a layering violation, and the cleanup call should be somewhere inside cmsg_deliver for the relevant messages. The rlist with cleanup triggers is better stored in a fiber key. -- Konstantin Osipov, Moscow, Russia