From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp44.i.mail.ru (smtp44.i.mail.ru [94.100.177.104]) (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 7193846971A for ; Fri, 13 Dec 2019 02:37:59 +0300 (MSK) References: <20191210083258.GD21413@atlas> <2c8fe897-9a9d-849d-463e-5fadff982b8c@tarantool.org> <20191211070830.GA5953@atlas> <20191212000047.GL1214@tarantool.org> From: Vladislav Shpilevoy Message-ID: <1d40cedb-c631-a420-2391-1f27a5a07fe5@tarantool.org> Date: Fri, 13 Dec 2019 00:37:57 +0100 MIME-Version: 1.0 In-Reply-To: <20191212000047.GL1214@tarantool.org> 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: Igor Munkin Cc: tarantool-patches@dev.tarantool.org Thanks for the review! >> diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c >> index 00ae8cded..d89c640aa 100644 >> --- a/src/lib/core/fiber.c >> +++ b/src/lib/core/fiber.c >> @@ -787,7 +801,7 @@ static void >> fiber_reset(struct fiber *fiber) >> { >> rlist_create(&fiber->on_yield); >> - rlist_create(&fiber->on_stop); >> + rlist_create(&fiber->on_cleanup); >> fiber->flags = FIBER_DEFAULT_FLAGS; >> #if ENABLE_FIBER_TOP >> clock_stat_reset(&fiber->clock_stat); >> @@ -856,8 +870,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_cleanup(fiber); > > I see you dropped the if above and fiber_cleanup doesn't have any > corresponding within. Is this removal an intentional one? Yes. This is because trigger_run works fine with empty rlist, so this check was useless. > > Minor: There is a mess with whitespace above. Feel free to ignore the > remark, since indentation was broken before your changes. I blamed > several lines and it looks more like a ridiculous code style, and not a > problem with editor. Thereby you just implicitly continue the mix. Wow, good catch! I didn't notice it in git diff nor in the editor. Fixed. > >> /* reset pending wakeups */ >> rlist_del(&fiber->state); >> if (! (fiber->flags & FIBER_IS_JOINABLE)) >> diff --git a/src/lib/core/fiber.h b/src/lib/core/fiber.h >> index c5b975513..21fff8f88 100644 >> --- a/src/lib/core/fiber.h >> +++ b/src/lib/core/fiber.h >> @@ -458,8 +458,17 @@ struct fiber { >> >> /** Triggers invoked before this fiber yields. Must not throw. */ >> struct rlist on_yield; >> - /** Triggers invoked before this fiber stops. Must not throw. */ >> - 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. >> + * In particular, for fibers not from fiber pool the >> + * cleanup is done before destroy and death. >> + * Pooled fibers are cleaned up after each request, even >> + * if they are never destroyed. >> + */ >> + struct rlist on_cleanup; > > Minor: both trigger lists above has the "Must not throw" note. Does the > introduced list respect this rule? Please, adjust the comment if yes. Nope. These 'must not throw' are outdated. Our triggers does not throw errors anymore by design. So I deleted it from the updated comment. >> /** >> * The list of fibers awaiting for this fiber's timely >> * (or untimely) death. >> diff --git a/src/lua/fiber.c b/src/lua/fiber.c >> index a7b75f9bf..4d2828f1c 100644 >> --- a/src/lua/fiber.c >> +++ b/src/lua/fiber.c >> @@ -606,12 +601,43 @@ lbox_fiber_name(struct lua_State *L) >> } >> } >> >> +/** >> + * Trigger invoked when the fiber has finished execution of its >> + * current request. Only purpose - delete storage.lua.ref keeping >> + * a reference of Lua fiber.storage object. Unlike Lua stack, >> + * Lua fiber storage may be created not only for fibers born from >> + * Lua land. For example, an IProto request may execute a Lua >> + * function, which can create the storage. Trigger guarantees, >> + * that even for non-Lua fibers the Lua storage is destroyed. >> + */ >> +static int >> +lbox_fiber_on_cleanup(struct trigger *trigger, void *event) >> +{ >> + struct fiber *f = (struct fiber *) event; >> + int storage_ref = f->storage.lua.ref; >> + assert(storage_ref > 0); >> + luaL_unref(tarantool_L, LUA_REGISTRYINDEX, storage_ref); >> + f->storage.lua.ref = 0; > > Though 0 value is specific "ref" (a table slot, where the LRU ref is > stored) and this value cannot be yield from luaL_ref. Please, consider > using LUA_NOREF value for such cases, since it's being provided by Lua > and lua_unref does nothing for it, as well as for LUA_REFNIL. Thanks, didn't know about these constants. =================================================================== assert(storage_ref > 0); luaL_unref(tarantool_L, LUA_REGISTRYINDEX, storage_ref); - f->storage.lua.ref = 0; + f->storage.lua.ref = LUA_NOREF; trigger_clear(trigger); free(trigger); ===================================================================