[Tarantool-patches] [PATCH v2 2/3] merger: clean fiber-local Lua stack after next()
Igor Munkin
imun at tarantool.org
Wed Jul 1 23:36:50 MSK 2020
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:
<snipped>
> 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.
<snipped>
> 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.
<snipped>
> ---
> 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
<snipped>
> @@ -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.
> }
>
<snipped>
> 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 @@
<snipped>
> +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 <temporary_L> differs from the given <L> for Lua-born fibers.
> + */
<snipped>
> --
> 2.25.0
>
--
Best regards,
IM
More information about the Tarantool-patches
mailing list