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

[Vladislav Shpilevoy <v.shpilevoy@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 <state>, <tuple> got %d "
> +		diag_set(IllegalParams, "Expected <state>, <tuple>, 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.h>           /* lua_*() */
> +#include <lauxlib.h>       /* 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 <state>, <buffer>, got 3 return values$',
> +    },
> +    {
> +        'buffer source, bad gen result',
> +        source_new = merger.new_buffer_source,
> +        source_gen = wrong_type,
> +        exp_err = '^Expected <state>, <buffer>$',
> +    },
> +    {
> +        '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$',
> +    },