From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-f196.google.com (mail-lj1-f196.google.com [209.85.208.196]) (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 F2C2846970E for ; Wed, 22 Jan 2020 01:32:23 +0300 (MSK) Received: by mail-lj1-f196.google.com with SMTP id o11so4259759ljc.6 for ; Tue, 21 Jan 2020 14:32:23 -0800 (PST) Date: Wed, 22 Jan 2020 01:32:21 +0300 From: Konstantin Osipov Message-ID: <20200121223221.GA5931@atlas> References: <31e884f2b6d2ab7b222229f1b204fa067d63ae65.1579211601.git.v.shpilevoy@tarantool.org> <20200117074552.GD24940@atlas> <20200120072234.GB19835@atlas> <36fe0fdc-c192-f850-c241-6a761777ad6b@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <36fe0fdc-c192-f850-c241-6a761777ad6b@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/22 01:25]: > On 20/01/2020 08:22, Konstantin Osipov wrote: > > * Vladislav Shpilevoy [20/01/20 09:59]: > >>> 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? > >> > >> We already discussed that in the previous patch version. We decided > >> to move cleanup to iproto.cc, because it depends on when a request > >> ends. Fiber pool knows nothing about requests. Iproto.cc is request > >> processing layer, and this is the right place for request data > >> destruction. > > > > True, but since you abstracted out the destruction via an opaque > > trigger, why not move the invocation of the trigger to fiber pool? > > fiber pool has most knowledge about fiber life cycle, so it seems > > natural to invoke the triggers in it - it will tie the *timing* to > > fiber pool, but not what's going on inside the trigger. > > > > Thoughts? > > And we came to the exactly the same patch I had in the first > version, with rolled back on_stop -> on_cleanup rename. > > Motivation for calling on_stop in iproto was that only iproto > is interested in clearing the fiber's state after each request, > and fiber pool should not decide when a request ends. From what > I understood. But I am ok with both solutions. > > Anyway, here is the new patch. lgtm. -- Konstantin Osipov, Moscow, Russia