From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 4E80842EF5C for ; Thu, 11 Jun 2020 19:20:30 +0300 (MSK) References: <28de36d645dab72681f6678d1b0774d64fa323d3.1591548554.git.alexander.turenko@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Thu, 11 Jun 2020 18:20:28 +0200 MIME-Version: 1.0 In-Reply-To: <28de36d645dab72681f6678d1b0774d64fa323d3.1591548554.git.alexander.turenko@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH 2.5/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 Thanks for the patch! See 6 comments below. > diff --git a/src/box/lua/merger.c b/src/box/lua/merger.c > index b8c432114..c7947d7da 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); 1. lua_settop() works fine even when top is -1. It basically means 'set top to the latest element' = 'leave the stack untouched'. I checked the implementation, should work. > luaL_unref(tarantool_L, LUA_REGISTRYINDEX, coro_ref); > } > @@ -898,7 +906,7 @@ luaL_merge_source_tuple_fetch(struct lua_State *L, > > /* Handle incorrect results count. */ > if (nresult != 2) { > - diag_set(IllegalParams, "Expected , got %d " > + diag_set(IllegalParams, "Expected , , got %d " 2. Unnecessary diff. > "return values", nresult); > return -1; > } > diff --git a/test/box-tap/check_merge_source.c b/test/box-tap/check_merge_source.c > new file mode 100644 > index 000000000..dbbf27bd1 > --- /dev/null > +++ b/test/box-tap/check_merge_source.c > @@ -0,0 +1,101 @@ > +#include /* lua_*() */ > +#include /* struct luaL_Reg */ > +#include "lib/core/diag.h" /* struct error, diag_*() */ > +#include "fiber.h" /* fiber_self() */ > +#include "lua/utils.h" /* luaL_checkcdata() */ > +#include "box/merger.h" /* struct merge_source, > + merge_source_next() */ 3. Do you really need these comments? Anyway they tend to outdate fast, because no one watches these comments when changes the code, uses some new functions from these files, etc. > + > +/** > + * Verify whether a temporary fiber-local Lua state has the same > + * amount of stack slots before and after merge_source_next() > + * call. > + * > + * A merge source is designed to be used from plain C code without > + * passing any Lua state explicitly. There are merge sources > + * ('table', 'buffer', 'tuple') that require temporary Lua stack > + * to fetch next tuple and they use fiber-local Lua stack when it > + * is available. > + * > + * Such calls should not left garbage on the fiber-local Lua > + * stack, because many of them in row may overflow the stack. > + * > + * The module built as a separate dynamic library, but it uses > + * internal tarantool functions. So it is not a 'real' external > + * module, but the stub that imitates usage of a merge source from > + * tarantool code. > + */ > + > +struct tuple; 4. If some merger API returns tuples, I guess it is merger's header responsibility to announce all the needed structures. And it does. So why do you need it here? > + > +/** > + * Extract a merge source from the Lua stack. > + */ > +static struct merge_source * > +luaT_check_merge_source(struct lua_State *L, int idx) > +{ > + uint32_t cdata_type; > + struct merge_source **source_ptr = luaL_checkcdata(L, idx, &cdata_type); > + assert(source_ptr != NULL); > + return *source_ptr; > +} > + > +/** > + * Call merge_source_next() virtual method of a merge source. > + * > + * The purpose of this function is to verify whether the > + * fiber-local Lua stack is properly cleaned after > + * merge_source_next() call on the passed merge source. > + * > + * @param merge_source a merge source to call > + * merge_source_next() on it > + * > + * @retval is_next_ok whether the call is successful > + * @retval err_msg error message from the call or nil > + * @retval is_stack_even whether the fiber-local Lua stack is > + * even after the call 5. This function takes L, and returns an int. These things are details of how L is changed. So they can't be described like that. Also, please, start sentences from capital letters, and end with a dot. It is really hard to read the parameter comments above, they look like one big piece of monolothic text. > + */ > +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). > + */ > + struct lua_State *temporary_L = fiber_self()->storage.lua.stack; > + assert(temporary_L != NULL); > + > + struct tuple *tuple; > + struct merge_source *source = luaT_check_merge_source(L, 1); > + > diff --git a/test/box-tap/gh-4954-merger-via-c.test.lua b/test/box-tap/gh-4954-merger-via-c.test.lua > new file mode 100755 > index 000000000..963b5825a > --- /dev/null > +++ b/test/box-tap/gh-4954-merger-via-c.test.lua > @@ -0,0 +1,247 @@ > +#!/usr/bin/env tarantool > +-- }}} > + > +local cases = { > + { > + 'buffer source, bad gen function', > + source_new = merger.new_buffer_source, > + source_gen = triplet, > + exp_err = '^Expected , , got 3 return values$', > + }, > + { > + 'buffer source, bad gen result', > + source_new = merger.new_buffer_source, > + source_gen = wrong_type, > + exp_err = '^Expected , $', > + }, > + { > + 'buffer source, bad buffer', > + source_new = merger.new_buffer_source, > + source_gen = bad_buffer, > + exp_err = '^Invalid merge source 0x[0-9a-f]+$', > + }, > + -- FIXME: Enable after gh-5048: ('non-array tuple in a buffer > + -- leads to assertion fail'). 6. The file is currently called gh-4954-merger-via-c.test.lua. So will you change the file name, when the test is enabled? Will 5048 get its own test, or it will be covered by this file without any file name changes? > + --[[ > + { > + 'buffer source, bad tuple in buffer', > + source_new = merger.new_buffer_source, > + source_gen = bad_tuple_in_buffer, > + exp_err = '^A tuple must be an array$', > + },