From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-f68.google.com (mail-lf1-f68.google.com [209.85.167.68]) (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 EE96146970E for ; Fri, 17 Jan 2020 10:45:54 +0300 (MSK) Received: by mail-lf1-f68.google.com with SMTP id 15so17617501lfr.2 for ; Thu, 16 Jan 2020 23:45:54 -0800 (PST) Date: Fri, 17 Jan 2020 10:45:53 +0300 From: Konstantin Osipov Message-ID: <20200117074552.GD24940@atlas> References: <31e884f2b6d2ab7b222229f1b204fa067d63ae65.1579211601.git.v.shpilevoy@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <31e884f2b6d2ab7b222229f1b204fa067d63ae65.1579211601.git.v.shpilevoy@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v2 2/3] 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 [20/01/17 00:57]: > Fiber.storage was not deleted when created in a fiber started from > the thread pool used by IProto requests. The problem was that > fiber.storage was created and deleted in Lua land only, assuming > that only Lua-born fibers could have it. But in fact any fiber can > create a Lua storage. Including the ones used to serve IProto > requests. > > Not deletion of the storage led to a possibility of meeting a > non-empty fiber.storage in the beginning of an iproto request, and > to not deletion of the memory caught by the storage until its > explicit nullification. > > Now the storage destructor works for any fiber, which managed to > create the storage. The destructor unrefs and nullifies the > storage. > > For destructor purposes the fiber.on_stop triggers were reworked. > Now they can be called multiple times during fiber's lifetime. > After every request done by that fiber. > > Closes #4662 > Closes #3462 I like this version of the patch much more than the previous one. It's way more clear. Instead of a generic name, you made a good comment for fiber::on_stop. > +/** > + * Stop the current fiber after a request is executed to make it > + * possible to reuse the fiber for a next request. On_stop > + * triggers remove all request-specific data from there. > + */ > +static inline void > +tx_fiber_on_stop() > +{ > + fiber_on_stop(fiber()); > +} > + > static void > tx_process_disconnect(struct cmsg *m) > { > @@ -1335,6 +1351,7 @@ tx_process_disconnect(struct cmsg *m) > if (! rlist_empty(&session_on_disconnect)) { > tx_fiber_init(con->session, 0); > session_run_on_disconnect_triggers(con->session); > + tx_fiber_on_stop(); > } > } Why did you have to add so many invocation points to fiber_on_stop() rather than simply adding fiber_on_stop invocation to fiber_pool.c? Maybe we discussed this, but it's been 3 weeks ago and I lost the context/rationale. > +void > +fiber_on_stop(struct fiber *f) > +{ > + if (trigger_run(&f->on_stop, f) != 0) > + panic("On_stop triggers can't fail"); > + /* > + * All on_stop triggers are supposed to remove themselves. > + * So as no to waste time on that here, and to make them > + * all work uniformly. > + */ Nice. > + assert(rlist_empty(&f->on_stop)); > +} > + > static void > fiber_recycle(struct fiber *fiber); > > @@ -856,8 +869,7 @@ fiber_loop(MAYBE_UNUSED void *data) > assert(f != fiber); > fiber_wakeup(f); > } > - if (! rlist_empty(&fiber->on_stop)) > - trigger_run(&fiber->on_stop, fiber); > + fiber_on_stop(fiber); This was an attempt to optimize a non-inline function call for the most common case. I would move this !rlist_empty check to fiber_on_stop and add a comment why we explicitly check for the list first. -- Konstantin Osipov, Moscow, Russia