Re: [Tarantool-patches] [PATCH v2 2/3] merger: clean fiber-local Lua stack after next()

[Igor Munkin <imun@tarantool.org>]

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