From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 A15F742F4AD for ; Wed, 1 Jul 2020 23:47:04 +0300 (MSK) Date: Wed, 1 Jul 2020 23:36:50 +0300 From: Igor Munkin Message-ID: <20200701203650.GB5559@tarantool.org> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH v2 2/3] merger: clean fiber-local Lua stack after next() List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Turenko Cc: tarantool-patches@dev.tarantool.org, Vladislav Shpilevoy Sasha, Thanks for the patch! Considering several typos and a couple of nits that can be freely considered as side notes (i.e. no changes are required) it LGTM. On 18.06.20, Alexander Turenko wrote: > Now those functions may be called only from Lua and if the fiber-local > Lua state is present it is the same as one that is passed to a Lua/C Typo: s/present/presented/. > function. After a Lua/C call a Lua interpreter automatically removes > everything below returned values. So the stack will not accumulate any > garbage. > The merge_source_next() implementations do not leave any garbage on a > Lua stack at success path, but may left something when an error occurs > (say, when a Lua iterator generator returns more then two values). I Typo: s/then/than/. > would not bother with finding and fixing all such cases, considering > that it would be valid for usual Lua/C code. However it seems reasonable > to ensure that a stack is even when we releasing a temporary Lua state. > --- > src/box/lua/merger.c | 47 ++-- > test/CMakeLists.txt | 1 + > test/box-tap/CMakeLists.txt | 4 + > test/box-tap/check_merge_source.c | 108 +++++++++ > test/box-tap/gh-4954-merger-via-c.test.lua | 251 +++++++++++++++++++++ > 5 files changed, 392 insertions(+), 19 deletions(-) > create mode 100644 test/box-tap/CMakeLists.txt > create mode 100644 test/box-tap/check_merge_source.c > create mode 100755 test/box-tap/gh-4954-merger-via-c.test.lua > > diff --git a/src/box/lua/merger.c b/src/box/lua/merger.c > index cc5626cbc..3e97969c8 100644 > --- a/src/box/lua/merger.c > +++ b/src/box/lua/merger.c > @@ -206,14 +216,10 @@ luaT_temp_luastate(int *coro_ref) > * It is the other half of `luaT_temp_luastate()`. > */ > static void > -luaT_release_temp_luastate(int coro_ref) > +luaT_release_temp_luastate(struct lua_State *L, int coro_ref, int top) > { > - /* > - * FIXME: The reusable fiber-local Lua state is not > - * unreferenced here (coro_ref == LUA_REFNIL), but > - * it must be truncated to its past top to prevent > - * stack overflow. > - */ > + if (top >= 0) > + lua_settop(L, top); > luaL_unref(tarantool_L, LUA_REGISTRYINDEX, coro_ref); Minor: You can just either restore top value for fiber-local Lua state or unreference Lua coroutine without restoring a pointer to its stack top slot. As a result you need to preserve the top value only for the first case (i.e. when the coro_ref is LUA_NOREF) and ignore the value for all other cases. > } > > diff --git a/test/box-tap/check_merge_source.c b/test/box-tap/check_merge_source.c > new file mode 100644 > index 000000000..8247f9df3 > --- /dev/null > +++ b/test/box-tap/check_merge_source.c > @@ -0,0 +1,108 @@ > +static int > +lbox_check_merge_source_call_next(struct lua_State *L) > +{ > + assert(lua_gettop(L) == 1); > + > + /* > + * Ensure that there is reusable temporary Lua stack. > + * > + * Note: It may be the same as L (and usually do). Minor: It would be nice to mention (at least for inquisitive persons) a case when differs from the given for Lua-born fibers. > + */ > -- > 2.25.0 > -- Best regards, IM