[Tarantool-patches] [PATCH v2 0/3] Merger's NULL defererence

[Tarantool-patches] [PATCH v2 0/3] Merger's NULL defererence

[Alexander Turenko]

Differences from v1
-------------------

- Added the patch to don't leave garbage on a reusable Lua stack (2.5th
  patch in v1 series).
- Dropped the patch, which trims luaL_ prefix from next() and destroy()
  implementations (1st patch in v1 series).
- [minor] Drop redundant `struct tuple;` definition from a test.
- [minor] Make `luacheck -r` happy with gh-4954-merger-via-c.test.lua.
- [minor] Small wording and formatting fixes.

See the v1 series for more detailed description of changes and the
related discussions:

https://lists.tarantool.org/pipermail/tarantool-patches/2020-June/017325.html

Overview of the patchset
------------------------

The first patch just fixes NULL pointer dereference that occurs due to
the wrong assumption: I did think that fiber().storage.lua.stack is
always exists and non-NULL.

The second patch fix merge sources to don't leave any garbage on a
reusable Lua stack (it may matter if we'll use a merge source from C
code).

The third commit is the optimization: it allows to don't create a new
Lua state in merger when possible.

Changelog entry
---------------

@ChangeLog

- merger: fix NULL pointer dereference when merger is called via the
  binary protocol (say, via net.box) (gh-4954)

Links
-----

https://github.com/tarantool/tarantool/issues/4954
Totktonada/gh-4954-fix-merger-segfault-full-ci

----

Alexander Turenko (3):
  merger: fix NULL dereference when called via iproto
  merger: clean fiber-local Lua stack after next()
  lua: expose temporary Lua state for iproto calls

 src/box/lua/call.c                            |  27 ++
 src/box/lua/merger.c                          | 200 ++++++++++++--
 src/lib/core/fiber.h                          |  14 +-
 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 ++++++++++++++++++
 .../gh-4954-merger-via-net-box.test.lua       | 129 +++++++++
 8 files changed, 708 insertions(+), 26 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
 create mode 100755 test/box-tap/gh-4954-merger-via-net-box.test.lua

-- 
2.25.0

[Tarantool-patches] [PATCH v2 1/3] merger: fix NULL dereference when called via iproto

[Alexander Turenko]

A merge source API is designed to be quite abstract: the base structure
and virtual methods do not depend on Lua anyhow. Each source should
implement next() and destroy() virtual methods, which may be called from
C without a Lua state. This design allows to use any source as from C as
well as from Lua. The Lua API is based on the C API and supports any
source. Even merger itself is implemented in pure C according to the
merge source API and so may be used from Lua.

A particular source implementation may use a Lua state internally, but
it is not part of the API and should be hidden under hood. In fact all
sources we have now (except merger itself) store some references in
LUA_REGISTRYINDEX and need a temporary Lua stack to work with them in
the next() virtual method.

Before this patch, the sources ('buffer', 'table', 'tuple') assume that
a Lua state always exists in the fiber storage of a fiber, where next()
is called. This looks valid on the first glance, because it may be
called either from a Lua code or from merger, which in turn is called
from a Lua code. However background fibers (they serve binary protocol
requests) do not store a Lua state in the fiber storage even for Lua
call / eval requests.

Possible solution would be always store a Lua state in a fiber storage.
There are two reasons why it is not implemented here:

1. There should be a decision about right balance between speed and
   memory footprint and maybe some eviction strategy for cached Lua
   states. Not sure we can just always store a state in each background
   fiber. It would be wasteful for instances that serve box DQL / DML,
   SQL and/or C procedure calls.
2. Technically contract of the next() method would assume that a Lua
   state should exist in a fiber storage. Such requirement looks quite
   unnatural for a C API and also looks fragile: what if we'll implement
   some less wasteful Lua state caching strategy and the assumption
   about presence of the Lua state will get broken?

Obviously, next() will spend extra time to create a temporary state when
it is called from a background fiber. We should reuse existing Lua state
at least when a Lua call is performed via a binary protocol. I consider
it as the optimization and will solve in the next commit.

A few words about the implementation. I have added three functions,
which acquire a temporary Lua state, call a function and release the
state. It may be squashed into one function that would accept a function
pointer and variable number of arguments. However GCC does not
devirtualize such calls at -O2 level, so it seems it is better to avoid
this. It maybe possible to write some weird macro that will technically
reduce code duplication, but I prefer to write in C, not some macro
based meta-language.

Usage of the fiber-local Lua state is not quite correct now: a merge
source may left garbage on a stack in case of failures (like unexpected
result of Lua iterator generator function). This behaviour is kept as is
here, but it will be resolved by the next patch.

Added API comments for destroy() and next() virtual methods to uniform
them visually with other merge source functions.

Changed order of luaL_merge_source_tuple_fetch() arguments to unify it
with other functions (<struct lua_State *>, <struct merge_source *>).

Fixes #4954
---
 src/box/lua/merger.c                          | 189 ++++++++++++++++--
 .../gh-4954-merger-via-net-box.test.lua       | 129 ++++++++++++
 2 files changed, 297 insertions(+), 21 deletions(-)
 create mode 100755 test/box-tap/gh-4954-merger-via-net-box.test.lua

diff --git a/src/box/lua/merger.c b/src/box/lua/merger.c
index 1b155152b..cc5626cbc 100644
--- a/src/box/lua/merger.c
+++ b/src/box/lua/merger.c
@@ -149,6 +149,74 @@ luaT_gettuple(struct lua_State *L, int idx, struct tuple_format *format)
 	return tuple;
 }
 
+/**
+ * Get a temporary Lua state.
+ *
+ * Use case: a function does not accept a Lua state as an argument
+ * to allow using from C code, but uses a Lua value, which is
+ * referenced in LUA_REGISTRYINDEX. A temporary Lua stack is needed
+ * to get and process the value.
+ *
+ * The returned state shares LUA_REGISTRYINDEX with `tarantool_L`.
+ *
+ * This Lua state should be used only from one fiber: otherwise
+ * one fiber may change the stack and another one will access a
+ * wrong stack slot when it will be scheduled for execution after
+ * yield.
+ *
+ * Return a Lua state on success and set @a coro_ref. This
+ * reference should be passed to `luaT_release_temp_luastate()`,
+ * when the state is not needed anymore.
+ *
+ * Return NULL and set a diag at failure.
+ */
+static struct lua_State *
+luaT_temp_luastate(int *coro_ref)
+{
+	if (fiber()->storage.lua.stack != NULL) {
+		*coro_ref = LUA_REFNIL;
+		return fiber()->storage.lua.stack;
+	}
+
+	/*
+	 * luaT_newthread() pops the new Lua state from
+	 * tarantool_L and it is right thing to do: if we'll push
+	 * something to it and yield, then another fiber will not
+	 * know that a stack top is changed and may operate on a
+	 * wrong slot.
+	 *
+	 * Second, many requests that push a value to tarantool_L
+	 * and yield may exhaust available slots on the stack.
+	 */
+	struct lua_State *L = luaT_newthread(tarantool_L);
+	if (L == NULL)
+		return NULL;
+	/*
+	 * The new state is not referenced from anywhere (reasons
+	 * are above), so we should keep a reference to it in the
+	 * registry while it is in use.
+	 */
+	*coro_ref = luaL_ref(tarantool_L, LUA_REGISTRYINDEX);
+	return L;
+}
+
+/**
+ * Release a temporary Lua state.
+ *
+ * It is the other half of `luaT_temp_luastate()`.
+ */
+static void
+luaT_release_temp_luastate(int coro_ref)
+{
+	/*
+	 * 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.
+	 */
+	luaL_unref(tarantool_L, LUA_REGISTRYINDEX, coro_ref);
+}
+
 /* }}} */
 
 /* {{{ Create, destroy structures from Lua */
@@ -396,16 +464,12 @@ luaL_merge_source_buffer_new(struct lua_State *L)
 }
 
 /**
- * Call a user provided function to get a next data chunk (a
- * buffer).
- *
- * Return 1 when a new buffer is received, 0 when a buffers
- * iterator ends and -1 at error and set a diag.
+ * Helper for `luaL_merge_source_buffer_fetch()`.
  */
 static int
-luaL_merge_source_buffer_fetch(struct merge_source_buffer *source)
+luaL_merge_source_buffer_fetch_impl(struct lua_State *L,
+				    struct merge_source_buffer *source)
 {
-	struct lua_State *L = fiber()->storage.lua.stack;
 	int nresult = luaL_iterator_next(L, source->fetch_it);
 
 	/* Handle a Lua error in a gen function. */
@@ -446,8 +510,32 @@ luaL_merge_source_buffer_fetch(struct merge_source_buffer *source)
 	return 1;
 }
 
+/**
+ * Call a user provided function to get a next data chunk (a
+ * buffer).
+ *
+ * Return 1 when a new buffer is received, 0 when a buffers
+ * iterator ends and -1 at error and set a diag.
+ */
+static int
+luaL_merge_source_buffer_fetch(struct merge_source_buffer *source)
+{
+	int coro_ref = LUA_REFNIL;
+	struct lua_State *L = luaT_temp_luastate(&coro_ref);
+	if (L == NULL)
+		return -1;
+	int rc = luaL_merge_source_buffer_fetch_impl(L, source);
+	luaT_release_temp_luastate(coro_ref);
+	return rc;
+}
+
 /* Virtual methods */
 
+/**
+ * destroy() virtual method implementation for a buffer source.
+ *
+ * @see struct merge_source_vtab
+ */
 static void
 luaL_merge_source_buffer_destroy(struct merge_source *base)
 {
@@ -462,6 +550,11 @@ luaL_merge_source_buffer_destroy(struct merge_source *base)
 	free(source);
 }
 
+/**
+ * next() virtual method implementation for a buffer source.
+ *
+ * @see struct merge_source_vtab
+ */
 static int
 luaL_merge_source_buffer_next(struct merge_source *base,
 			      struct tuple_format *format,
@@ -586,9 +679,9 @@ luaL_merge_source_table_new(struct lua_State *L)
  * received and -1 at an error (set a diag).
  */
 static int
-luaL_merge_source_table_fetch(struct merge_source_table *source)
+luaL_merge_source_table_fetch(struct lua_State *L,
+			      struct merge_source_table *source)
 {
-	struct lua_State *L = fiber()->storage.lua.stack;
 	int nresult = luaL_iterator_next(L, source->fetch_it);
 
 	/* Handle a Lua error in a gen function. */
@@ -625,6 +718,11 @@ luaL_merge_source_table_fetch(struct merge_source_table *source)
 
 /* Virtual methods */
 
+/**
+ * destroy() virtual method implementation for a table source.
+ *
+ * @see struct merge_source_vtab
+ */
 static void
 luaL_merge_source_table_destroy(struct merge_source *base)
 {
@@ -639,12 +737,15 @@ luaL_merge_source_table_destroy(struct merge_source *base)
 	free(source);
 }
 
+/**
+ * Helper for `luaL_merge_source_table_next()`.
+ */
 static int
-luaL_merge_source_table_next(struct merge_source *base,
-			     struct tuple_format *format,
-			     struct tuple **out)
+luaL_merge_source_table_next_impl(struct lua_State *L,
+				  struct merge_source *base,
+				  struct tuple_format *format,
+				  struct tuple **out)
 {
-	struct lua_State *L = fiber()->storage.lua.stack;
 	struct merge_source_table *source = container_of(base,
 		struct merge_source_table, base);
 
@@ -659,7 +760,7 @@ luaL_merge_source_table_next(struct merge_source *base,
 	while (source->ref == 0 || lua_isnil(L, -1)) {
 		if (source->ref > 0)
 			lua_pop(L, 2);
-		int rc = luaL_merge_source_table_fetch(source);
+		int rc = luaL_merge_source_table_fetch(L, source);
 		if (rc < 0)
 			return -1;
 		if (rc == 0) {
@@ -687,6 +788,25 @@ luaL_merge_source_table_next(struct merge_source *base,
 	return 0;
 }
 
+/**
+ * next() virtual method implementation for a table source.
+ *
+ * @see struct merge_source_vtab
+ */
+static int
+luaL_merge_source_table_next(struct merge_source *base,
+			     struct tuple_format *format,
+			     struct tuple **out)
+{
+	int coro_ref = LUA_REFNIL;
+	struct lua_State *L = luaT_temp_luastate(&coro_ref);
+	if (L == NULL)
+		return -1;
+	int rc = luaL_merge_source_table_next_impl(L, base, format, out);
+	luaT_release_temp_luastate(coro_ref);
+	return rc;
+}
+
 /* Lua functions */
 
 /**
@@ -763,8 +883,8 @@ luaL_merge_source_tuple_new(struct lua_State *L)
  * Return -1 at error (set a diag).
  */
 static int
-luaL_merge_source_tuple_fetch(struct merge_source_tuple *source,
-			       struct lua_State *L)
+luaL_merge_source_tuple_fetch(struct lua_State *L,
+			      struct merge_source_tuple *source)
 {
 	int nresult = luaL_iterator_next(L, source->fetch_it);
 
@@ -792,6 +912,11 @@ luaL_merge_source_tuple_fetch(struct merge_source_tuple *source,
 
 /* Virtual methods */
 
+/**
+ * destroy() virtual method implementation for a tuple source.
+ *
+ * @see struct merge_source_vtab
+ */
 static void
 luaL_merge_source_tuple_destroy(struct merge_source *base)
 {
@@ -804,16 +929,19 @@ luaL_merge_source_tuple_destroy(struct merge_source *base)
 	free(source);
 }
 
+/**
+ * Helper for `luaL_merge_source_tuple_next()`.
+ */
 static int
-luaL_merge_source_tuple_next(struct merge_source *base,
-			     struct tuple_format *format,
-			     struct tuple **out)
+luaL_merge_source_tuple_next_impl(struct lua_State *L,
+				  struct merge_source *base,
+				  struct tuple_format *format,
+				  struct tuple **out)
 {
-	struct lua_State *L = fiber()->storage.lua.stack;
 	struct merge_source_tuple *source = container_of(base,
 		struct merge_source_tuple, base);
 
-	int rc = luaL_merge_source_tuple_fetch(source, L);
+	int rc = luaL_merge_source_tuple_fetch(L, source);
 	if (rc < 0)
 		return -1;
 	/*
@@ -834,6 +962,25 @@ luaL_merge_source_tuple_next(struct merge_source *base,
 	return 0;
 }
 
+/**
+ * next() virtual method implementation for a tuple source.
+ *
+ * @see struct merge_source_vtab
+ */
+static int
+luaL_merge_source_tuple_next(struct merge_source *base,
+			     struct tuple_format *format,
+			     struct tuple **out)
+{
+	int coro_ref = LUA_REFNIL;
+	struct lua_State *L = luaT_temp_luastate(&coro_ref);
+	if (L == NULL)
+		return -1;
+	int rc = luaL_merge_source_tuple_next_impl(L, base, format, out);
+	luaT_release_temp_luastate(coro_ref);
+	return rc;
+}
+
 /* Lua functions */
 
 /**
diff --git a/test/box-tap/gh-4954-merger-via-net-box.test.lua b/test/box-tap/gh-4954-merger-via-net-box.test.lua
new file mode 100755
index 000000000..e2bd6f8b9
--- /dev/null
+++ b/test/box-tap/gh-4954-merger-via-net-box.test.lua
@@ -0,0 +1,129 @@
+#!/usr/bin/env tarantool
+
+local merger_lib = require('merger')
+local buffer = require('buffer')
+local msgpack = require('msgpack')
+local net_box = require('net.box')
+local fiber = require('fiber')
+local tap = require('tap')
+
+
+-- {{{ Helpers
+
+-- Lua iterator generator function to iterate over an array.
+local function array_next(arr, idx)
+    idx = idx or 1
+    local item = arr[idx]
+    if item == nil then
+        return
+    end
+    return idx + 1, item
+end
+
+-- Lua iterator generator to iterate over an array with yields.
+local function array_yield_next(arr, idx)
+    fiber.sleep(0)
+    return array_next(arr, idx)
+end
+
+-- }}}
+
+-- {{{ Code that is run in a background fiber (via net.box)
+
+local function use_table_source(tuples)
+    local source = merger_lib.new_source_fromtable(tuples)
+    return source:select()
+end
+_G.use_table_source = use_table_source
+
+local function use_buffer_source(tuples)
+    local buf = buffer.ibuf()
+    msgpack.encode(tuples, buf)
+    local source = merger_lib.new_source_frombuffer(buf)
+    return source:select()
+end
+_G.use_buffer_source = use_buffer_source
+
+local function use_tuple_source(tuples)
+    local source = merger_lib.new_tuple_source(array_next, tuples)
+    return source:select()
+end
+_G.use_tuple_source = use_tuple_source
+
+local function use_table_source_yield(tuples)
+    local chunks = {}
+    for i, t in ipairs(tuples) do
+        chunks[i] = {t}
+    end
+    local source = merger_lib.new_table_source(array_yield_next, chunks)
+    return source:select()
+end
+_G.use_table_source_yield = use_table_source_yield
+
+local function use_buffer_source_yield(tuples)
+    local buffers = {}
+    for i, t in ipairs(tuples) do
+        buffers[i] = buffer.ibuf()
+        msgpack.encode({t}, buffers[i])
+    end
+    local source = merger_lib.new_buffer_source(array_yield_next, buffers)
+    return source:select()
+end
+_G.use_buffer_source_yield = use_buffer_source_yield
+
+local function use_tuple_source_yield(tuples)
+    local source = merger_lib.new_tuple_source(array_yield_next, tuples)
+    return source:select()
+end
+_G.use_tuple_source_yield = use_tuple_source_yield
+
+-- }}}
+
+box.cfg({
+    listen = os.getenv('LISTEN') or 'localhost:3301'
+})
+box.schema.user.grant('guest', 'execute', 'universe', nil,
+                      {if_not_exists = true})
+
+local test = tap.test('gh-4954-merger-via-net-box.test.lua')
+test:plan(6)
+
+local tuples = {
+    {1},
+    {2},
+    {3},
+}
+
+local connection = net_box.connect(box.cfg.listen)
+
+local res = connection:call('use_table_source', {tuples})
+test:is_deeply(res, tuples, 'verify table source')
+local res = connection:call('use_buffer_source', {tuples})
+test:is_deeply(res, tuples, 'verify buffer source')
+local res = connection:call('use_tuple_source', {tuples})
+test:is_deeply(res, tuples, 'verify tuple source')
+
+local function test_verify_source_async(test, func_name, request_count)
+    test:plan(request_count)
+
+    local futures = {}
+    for _ = 1, request_count do
+        local future = connection:call(func_name, {tuples}, {is_async = true})
+        table.insert(futures, future)
+    end
+    for i = 1, request_count do
+        local res = unpack(futures[i]:wait_result())
+        test:is_deeply(res, tuples, ('verify request %d'):format(i))
+    end
+end
+
+test:test('verify table source, which yields', test_verify_source_async,
+          'use_table_source_yield', 100)
+test:test('verify buffer source, which yields', test_verify_source_async,
+          'use_buffer_source_yield', 100)
+test:test('verify tuple source, which yields', test_verify_source_async,
+          'use_tuple_source_yield', 100)
+
+box.schema.user.revoke('guest', 'execute', 'universe')
+
+os.exit(test:check() and 0 or 1)
-- 
2.25.0

Re: [Tarantool-patches] [PATCH v2 1/3] merger: fix NULL dereference when called via iproto

[Vladislav Shpilevoy]

Thanks for the patch!

> diff --git a/src/box/lua/merger.c b/src/box/lua/merger.c
> index 1b155152b..cc5626cbc 100644
> --- a/src/box/lua/merger.c
> +++ b/src/box/lua/merger.c
> @@ -396,16 +464,12 @@ luaL_merge_source_buffer_new(struct lua_State *L)
>  }
>  
>  /**
> - * Call a user provided function to get a next data chunk (a
> - * buffer).
> - *
> - * Return 1 when a new buffer is received, 0 when a buffers
> - * iterator ends and -1 at error and set a diag.
> + * Helper for `luaL_merge_source_buffer_fetch()`.
>   */
>  static int
> -luaL_merge_source_buffer_fetch(struct merge_source_buffer *source)
> +luaL_merge_source_buffer_fetch_impl(struct lua_State *L,
> +				    struct merge_source_buffer *source)

I see my comment to the former first commit was missed somewhy, so I say
it here:

    - If a function takes lua_State, it is not mean it should get
      luaL_ prefix. Since the first commit was dropped, this comment
      we can't fix.

    - If a function is a method of struct, struct pointer goes *always*
      first, like 'this' argument in C++. Present of lua_State does
      not mean it should be first. This can and should be fixed, I think.

This function in the first place is a method of merge_source_buffer.
So it should take this argument first. luaL_merge_source_buffer_fetch_impl()
currently is the only method of merge_source_buffer, which takes
merge_source_buffer not in the first argument. It means, I don't see
how the consistency is improved here. The same for
luaL_merge_source_table_fetch, luaL_merge_source_table_next_impl,
luaL_merge_source_tuple_fetch, luaL_merge_source_tuple_next_impl.

Re: [Tarantool-patches] [PATCH v2 1/3] merger: fix NULL dereference when called via iproto

[Alexander Turenko]

> >  static int
> > -luaL_merge_source_buffer_fetch(struct merge_source_buffer *source)
> > +luaL_merge_source_buffer_fetch_impl(struct lua_State *L,
> > +				    struct merge_source_buffer *source)
> 
> I see my comment to the former first commit was missed somewhy, so I say
> it here:

Sorry, I misread it a bit and understood as part of illustration that
there are still no consistency even after dropping luaL prefix.

> 
>     - If a function takes lua_State, it is not mean it should get
>       luaL_ prefix. Since the first commit was dropped, this comment
>       we can't fix.
> 
>     - If a function is a method of struct, struct pointer goes *always*
>       first, like 'this' argument in C++. Present of lua_State does
>       not mean it should be first. This can and should be fixed, I think.
> 
> This function in the first place is a method of merge_source_buffer.
> So it should take this argument first. luaL_merge_source_buffer_fetch_impl()
> currently is the only method of merge_source_buffer, which takes
> merge_source_buffer not in the first argument. It means, I don't see
> how the consistency is improved here. The same for
> luaL_merge_source_table_fetch, luaL_merge_source_table_next_impl,
> luaL_merge_source_tuple_fetch, luaL_merge_source_tuple_next_impl.

Moved <struct lua_State *> to the end of an argument list.

Removed the following paragraph from the commit message:

 | Changed order of luaL_merge_source_tuple_fetch() arguments to unify it
 | with other functions (<struct lua_State *>, <struct merge_source *>).

Re: [Tarantool-patches] [PATCH v2 1/3] merger: fix NULL dereference when called via iproto

[Vladislav Shpilevoy]

> diff --git a/src/box/lua/merger.c b/src/box/lua/merger.c
> index 1b155152b..f72e70ee8 100644
> --- a/src/box/lua/merger.c
> +++ b/src/box/lua/merger.c
> @@ -764,7 +884,7 @@ luaL_merge_source_tuple_new(struct lua_State *L)
>   */
>  static int
>  luaL_merge_source_tuple_fetch(struct merge_source_tuple *source,
> -			       struct lua_State *L)
> +			      struct lua_State *L)

This hunk seems to be unnecessary.

Re: [Tarantool-patches] [PATCH v2 1/3] merger: fix NULL dereference when called via iproto

[Alexander Turenko]

On Sat, Jun 20, 2020 at 01:32:17AM +0200, Vladislav Shpilevoy wrote:
> > diff --git a/src/box/lua/merger.c b/src/box/lua/merger.c
> > index 1b155152b..f72e70ee8 100644
> > --- a/src/box/lua/merger.c
> > +++ b/src/box/lua/merger.c
> > @@ -764,7 +884,7 @@ luaL_merge_source_tuple_new(struct lua_State *L)
> >   */
> >  static int
> >  luaL_merge_source_tuple_fetch(struct merge_source_tuple *source,
> > -			       struct lua_State *L)
> > +			      struct lua_State *L)
> 
> This hunk seems to be unnecessary.

Thanks for noticing!

Fixed.

Re: [Tarantool-patches] [PATCH v2 1/3] merger: fix NULL dereference when called via iproto

[Igor Munkin]

Sasha,

Thanks for the patch! It LGTM except the couple of nits I left below.

On 18.06.20, Alexander Turenko wrote:

<snipped>

> A particular source implementation may use a Lua state internally, but
> it is not part of the API and should be hidden under hood. In fact all

Typo: s/under hood/under the hood/ or s/under hood/under its hood/.

> sources we have now (except merger itself) store some references in
> LUA_REGISTRYINDEX and need a temporary Lua stack to work with them in
> the next() virtual method.

<snipped>

> A few words about the implementation. I have added three functions,
> which acquire a temporary Lua state, call a function and release the
> state. It may be squashed into one function that would accept a function
> pointer and variable number of arguments. However GCC does not
> devirtualize such calls at -O2 level, so it seems it is better to avoid
> this. It maybe possible to write some weird macro that will technically
> reduce code duplication, but I prefer to write in C, not some macro
> based meta-language.

Side note: No one pushes you to create a particular DSL for this case,
but I see nothing criminal to use macros sometimes. I personally prefer
to generalize the occurrences you mentioned above. On the second thought
I guess performance deviation is negligible and the benefits for the
further maintenance are doubtful.

> 

<snipped>

> ---
>  src/box/lua/merger.c                          | 189 ++++++++++++++++--
>  .../gh-4954-merger-via-net-box.test.lua       | 129 ++++++++++++
>  2 files changed, 297 insertions(+), 21 deletions(-)
>  create mode 100755 test/box-tap/gh-4954-merger-via-net-box.test.lua
> 
> diff --git a/src/box/lua/merger.c b/src/box/lua/merger.c
> index 1b155152b..cc5626cbc 100644
> --- a/src/box/lua/merger.c
> +++ b/src/box/lua/merger.c
> @@ -149,6 +149,74 @@ luaT_gettuple(struct lua_State *L, int idx, struct tuple_format *format)
>  	return tuple;
>  }
>  
> +/**
> + * Get a temporary Lua state.
> + *
> + * Use case: a function does not accept a Lua state as an argument
> + * to allow using from C code, but uses a Lua value, which is
> + * referenced in LUA_REGISTRYINDEX. A temporary Lua stack is needed
> + * to get and process the value.
> + *
> + * The returned state shares LUA_REGISTRYINDEX with `tarantool_L`.

Pardon, I don't get this line.

> + *
> + * This Lua state should be used only from one fiber: otherwise
> + * one fiber may change the stack and another one will access a
> + * wrong stack slot when it will be scheduled for execution after
> + * yield.
> + *
> + * Return a Lua state on success and set @a coro_ref. This
> + * reference should be passed to `luaT_release_temp_luastate()`,
> + * when the state is not needed anymore.
> + *
> + * Return NULL and set a diag at failure.
> + */
> +static struct lua_State *
> +luaT_temp_luastate(int *coro_ref)
> +{
> +	if (fiber()->storage.lua.stack != NULL) {
> +		*coro_ref = LUA_REFNIL;

It definitely doesn't affect the implemented behaviour (considering
you're not referencing a <nil> value within <luaT_temp_luastate>); I'm
just too pedantic here: LUA_REFNIL is the ref value obtained from
<luaL_ref> call anchoring a <nil> slot. At the same time there is
another special ref value for your purposes -- LUA_NOREF[1].
Furthermore, it's the way more convenient to use it for *all* initial
ref values below.

> +		return fiber()->storage.lua.stack;
> +	}
> +
> +	/*
> +	 * luaT_newthread() pops the new Lua state from
> +	 * tarantool_L and it is right thing to do: if we'll push
> +	 * something to it and yield, then another fiber will not
> +	 * know that a stack top is changed and may operate on a
> +	 * wrong slot.

It seems to relate more to <luaT_newthread> contract, so you can just
mention that it leaves no garbage on the given coroutine stack, ergo
nothing need to be popped in the caller function.

> +	 *
> +	 * Second, many requests that push a value to tarantool_L
> +	 * and yield may exhaust available slots on the stack.

Pardon, I don't get this line.

> +	 */
> +	struct lua_State *L = luaT_newthread(tarantool_L);
> +	if (L == NULL)
> +		return NULL;
> +	/*
> +	 * The new state is not referenced from anywhere (reasons
> +	 * are above), so we should keep a reference to it in the
> +	 * registry while it is in use.
> +	 */
> +	*coro_ref = luaL_ref(tarantool_L, LUA_REGISTRYINDEX);
> +	return L;
> +}
> +
> +/**
> + * Release a temporary Lua state.
> + *
> + * It is the other half of `luaT_temp_luastate()`.

It's not a half, it's a complement for <luaT_temp_luastate> function.

> + */

<snipped>

> -- 
> 2.25.0
> 

[1]: https://www.lua.org/manual/5.1/manual.html#pdf-LUA_NOREF

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH v2 1/3] merger: fix NULL dereference when called via iproto

[Alexander Turenko]

> > A particular source implementation may use a Lua state internally, but
> > it is not part of the API and should be hidden under hood. In fact all
> 
> Typo: s/under hood/under the hood/ or s/under hood/under its hood/.

Fixed.

> > A few words about the implementation. I have added three functions,
> > which acquire a temporary Lua state, call a function and release the
> > state. It may be squashed into one function that would accept a function
> > pointer and variable number of arguments. However GCC does not
> > devirtualize such calls at -O2 level, so it seems it is better to avoid
> > this. It maybe possible to write some weird macro that will technically
> > reduce code duplication, but I prefer to write in C, not some macro
> > based meta-language.
> 
> Side note: No one pushes you to create a particular DSL for this case,
> but I see nothing criminal to use macros sometimes. I personally prefer
> to generalize the occurrences you mentioned above. On the second thought
> I guess performance deviation is negligible and the benefits for the
> further maintenance are doubtful.

I don't find a good way to generate declarations and definitions (or at
least definitions) for those functions using some macro. The main
problem is that each function has its own arguments list (with its own
types).

> > +/**
> > + * Get a temporary Lua state.
> > + *
> > + * Use case: a function does not accept a Lua state as an argument
> > + * to allow using from C code, but uses a Lua value, which is
> > + * referenced in LUA_REGISTRYINDEX. A temporary Lua stack is needed
> > + * to get and process the value.
> > + *
> > + * The returned state shares LUA_REGISTRYINDEX with `tarantool_L`.
> 
> Pardon, I don't get this line.

I made an attempt to clarify the comment:

 | * The resulting Lua state has a separate Lua stack, but the same
 | * globals and registry as `tarantool_L` (and all Lua states in
 | * tarantool at the moment of writing this).

Existing terms are not perfect: both lua_newthread() and luaL_newstate()
return the same type of pointer: <struct lua_State *>. So I called any
so typed structure 'Lua state' or just 'state'. I hope now the idea 'you
can reference something in a registry of some other Lua state and then
get it from the registry using this temporary Lua state' is clear.

> 
> > + *
> > + * This Lua state should be used only from one fiber: otherwise
> > + * one fiber may change the stack and another one will access a
> > + * wrong stack slot when it will be scheduled for execution after
> > + * yield.
> > + *
> > + * Return a Lua state on success and set @a coro_ref. This
> > + * reference should be passed to `luaT_release_temp_luastate()`,
> > + * when the state is not needed anymore.
> > + *
> > + * Return NULL and set a diag at failure.
> > + */
> > +static struct lua_State *
> > +luaT_temp_luastate(int *coro_ref)
> > +{
> > +	if (fiber()->storage.lua.stack != NULL) {
> > +		*coro_ref = LUA_REFNIL;
> 
> It definitely doesn't affect the implemented behaviour (considering
> you're not referencing a <nil> value within <luaT_temp_luastate>); I'm
> just too pedantic here: LUA_REFNIL is the ref value obtained from
> <luaL_ref> call anchoring a <nil> slot. At the same time there is
> another special ref value for your purposes -- LUA_NOREF[1].
> Furthermore, it's the way more convenient to use it for *all* initial
> ref values below.
>
> [1]: https://www.lua.org/manual/5.1/manual.html#pdf-LUA_NOREF

LUA_REFNIL is used more often in this sense in tarantool (I don't know
why) and there is no practical difference (of course, when <nil> is not
valid value to reference). I just chose one that is more widely used
within our sources. But considering your opinion that LUA_NOREF name
better fit here, I changed it to LUA_NOREF now.

> > +	/*
> > +	 * luaT_newthread() pops the new Lua state from
> > +	 * tarantool_L and it is right thing to do: if we'll push
> > +	 * something to it and yield, then another fiber will not
> > +	 * know that a stack top is changed and may operate on a
> > +	 * wrong slot.
> 
> It seems to relate more to <luaT_newthread> contract, so you can just
> mention that it leaves no garbage on the given coroutine stack, ergo
> nothing need to be popped in the caller function.

I have two goals here:

1. Clarify luaT_newthread() contract on the caller side, because it is
   unusual for Lua.

2. Explain why we should not leave the new state on top of tarantool_L
   in luaT_temp_luastate().

There are two reasons, why leaving 'garbage' on tarantool_L is not
acceptable here. I want to mention both here.

I reformatted the comment a bit to make it more clear:

 | /*
 |  * Unlike lua_newthread(), luaT_newthread() does not leave
 |  * the new Lua state on tarantool_L.
 |  *
 |  * It is desired behaviour here, because of two reasons.
 |  *
 |  * First, if we'll push something to tarantool_L and
 |  * yield, then another fiber will not know that a stack
 |  * top is changed and may operate on a wrong slot.
 |  *
 |  * Second, many requests that push a value to tarantool_L
 |  * and yield may exhaust available slots on the stack. It
 |  * is limited by LUAI_MAXSTACK build time constant (~65K).
 |  */

> > +	 *
> > +	 * Second, many requests that push a value to tarantool_L
> > +	 * and yield may exhaust available slots on the stack.
> 
> Pardon, I don't get this line.

I don't know how clarify it and don't make it huge.

The case is the following: tarantool serves many similar requests. All
requests execute luaT_temp_luastate() function and call a merge source
fetch function (an iterator generator), which yields. If we would leave
new Lua states on tarantool_L, it would be possible that all available
stack slots in tarantool_L are occupied and the next request will fails
with the 'stack overflow' error.

After offline discussion with Igor we agreed to clarify the comment with
mention of the LUAI_MAXSTACK constant. The new comment is the following:

 | * Second, many requests that push a value to tarantool_L
 | * and yield may exhaust available slots on the stack. It
 | * is limited by LUAI_MAXSTACK build time constant (~65K).

> > +/**
> > + * Release a temporary Lua state.
> > + *
> > + * It is the other half of `luaT_temp_luastate()`.
> 
> It's not a half, it's a complement for <luaT_temp_luastate> function.

Thanks! I thought on the right word and failed with it. The new wording:

 | /**
 |  * Release a temporary Lua state.
 |  *
 |  * It complements `luaT_temp_luastate()`.
 |  */

Re: [Tarantool-patches] [PATCH v2 1/3] merger: fix NULL dereference when called via iproto

[Igor Munkin]

Sasha,

Thanks for the updates!

On 16.07.20, Alexander Turenko wrote:

<snipped>

> > > +/**
> > > + * Get a temporary Lua state.
> > > + *
> > > + * Use case: a function does not accept a Lua state as an argument
> > > + * to allow using from C code, but uses a Lua value, which is
> > > + * referenced in LUA_REGISTRYINDEX. A temporary Lua stack is needed
> > > + * to get and process the value.
> > > + *
> > > + * The returned state shares LUA_REGISTRYINDEX with `tarantool_L`.
> > 
> > Pardon, I don't get this line.
> 
> I made an attempt to clarify the comment:
> 
>  | * The resulting Lua state has a separate Lua stack, but the same
>  | * globals and registry as `tarantool_L` (and all Lua states in
>  | * tarantool at the moment of writing this).

Neat, thanks!

> 
> Existing terms are not perfect: both lua_newthread() and luaL_newstate()
> return the same type of pointer: <struct lua_State *>. So I called any
> so typed structure 'Lua state' or just 'state'. I hope now the idea 'you
> can reference something in a registry of some other Lua state and then
> get it from the registry using this temporary Lua state' is clear.

Yes, Lua terms are too close and ambiguous:
* There is a "state" (<struct global_State>, Lua universe) consisting
  such global entities of runtime as GC state, registry, string table,
  debug hooks, etc.
* There is a "thread" (<strict lua_State>, Lua coroutine) consisting
  such coroutine-local entities as coroutine guest stack, top and base
  slots of the current frame, reference to global state, etc.

I'm totally fine with your wording now, but guess we already need kinda
glossary for internal usage :)

> 

<snipped>

> 
> > > +	/*
> > > +	 * luaT_newthread() pops the new Lua state from
> > > +	 * tarantool_L and it is right thing to do: if we'll push
> > > +	 * something to it and yield, then another fiber will not
> > > +	 * know that a stack top is changed and may operate on a
> > > +	 * wrong slot.
> > 
> > It seems to relate more to <luaT_newthread> contract, so you can just
> > mention that it leaves no garbage on the given coroutine stack, ergo
> > nothing need to be popped in the caller function.
> 
> I have two goals here:
> 
> 1. Clarify luaT_newthread() contract on the caller side, because it is
>    unusual for Lua.
> 
> 2. Explain why we should not leave the new state on top of tarantool_L
>    in luaT_temp_luastate().
> 
> There are two reasons, why leaving 'garbage' on tarantool_L is not
> acceptable here. I want to mention both here.
> 
> I reformatted the comment a bit to make it more clear:
> 
>  | /*
>  |  * Unlike lua_newthread(), luaT_newthread() does not leave
>  |  * the new Lua state on tarantool_L.

I was around to it today and unfortunately it does[1]. So you need to
explicitly pop a newly created coroutine from the guest stack right
after anchoring it to the registry.

>  |  *
>  |  * It is desired behaviour here, because of two reasons.
>  |  *
>  |  * First, if we'll push something to tarantool_L and
>  |  * yield, then another fiber will not know that a stack
>  |  * top is changed and may operate on a wrong slot.
>  |  *
>  |  * Second, many requests that push a value to tarantool_L
>  |  * and yield may exhaust available slots on the stack. It
>  |  * is limited by LUAI_MAXSTACK build time constant (~65K).
>  |  */
> 
> > > +	 *
> > > +	 * Second, many requests that push a value to tarantool_L
> > > +	 * and yield may exhaust available slots on the stack.
> > 
> > Pardon, I don't get this line.
> 
> I don't know how clarify it and don't make it huge.
> 
> The case is the following: tarantool serves many similar requests. All
> requests execute luaT_temp_luastate() function and call a merge source
> fetch function (an iterator generator), which yields. If we would leave
> new Lua states on tarantool_L, it would be possible that all available
> stack slots in tarantool_L are occupied and the next request will fails
> with the 'stack overflow' error.
> 
> After offline discussion with Igor we agreed to clarify the comment with
> mention of the LUAI_MAXSTACK constant. The new comment is the following:
> 
>  | * Second, many requests that push a value to tarantool_L
>  | * and yield may exhaust available slots on the stack. It
>  | * is limited by LUAI_MAXSTACK build time constant (~65K).

That's more than enough, thanks!

> 

<snipped>

[1]: https://github.com/tarantool/tarantool/blob/master/src/lua/utils.h#L617

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH v2 1/3] merger: fix NULL dereference when called via iproto

[Igor Munkin]

Sasha,

This patch is also LGTM, except the single comment below.

On 17.07.20, Igor Munkin wrote:

<snipped>

> > 
> > I reformatted the comment a bit to make it more clear:
> > 
> >  | /*
> >  |  * Unlike lua_newthread(), luaT_newthread() does not leave
> >  |  * the new Lua state on tarantool_L.
> 
> I was around to it today and unfortunately it does[1]. So you need to
> explicitly pop a newly created coroutine from the guest stack right
> after anchoring it to the registry.

As you mentioned in offline discussion there is an implicit pop
underneath <luaL_ref> routine, so the implementation is fine and only
this misleading comment need to be fixed.

> 
> >  |  *
> >  |  * It is desired behaviour here, because of two reasons.
> >  |  *
> >  |  * First, if we'll push something to tarantool_L and
> >  |  * yield, then another fiber will not know that a stack
> >  |  * top is changed and may operate on a wrong slot.
> >  |  *
> >  |  * Second, many requests that push a value to tarantool_L
> >  |  * and yield may exhaust available slots on the stack. It
> >  |  * is limited by LUAI_MAXSTACK build time constant (~65K).
> >  |  */

<snipped>

> 
> -- 
> Best regards,
> IM

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH v2 1/3] merger: fix NULL dereference when called via iproto

[Alexander Turenko]

> Yes, Lua terms are too close and ambiguous:
> * There is a "state" (<struct global_State>, Lua universe) consisting
>   such global entities of runtime as GC state, registry, string table,
>   debug hooks, etc.
> * There is a "thread" (<strict lua_State>, Lua coroutine) consisting
>   such coroutine-local entities as coroutine guest stack, top and base
>   slots of the current frame, reference to global state, etc.
> 
> I'm totally fine with your wording now, but guess we already need kinda
> glossary for internal usage :)

It looks as the good candidate to include into Tarantool Internals set
of articles [1] or to our GitHub wiki.

[1]: https://tarantool-ref.readthedocs.io/en/latest/

> > > > +	/*
> > > > +	 * luaT_newthread() pops the new Lua state from
> > > > +	 * tarantool_L and it is right thing to do: if we'll push
> > > > +	 * something to it and yield, then another fiber will not
> > > > +	 * know that a stack top is changed and may operate on a
> > > > +	 * wrong slot.
> > > 
> > > It seems to relate more to <luaT_newthread> contract, so you can just
> > > mention that it leaves no garbage on the given coroutine stack, ergo
> > > nothing need to be popped in the caller function.
> > 
> > I have two goals here:
> > 
> > 1. Clarify luaT_newthread() contract on the caller side, because it is
> >    unusual for Lua.
> > 
> > 2. Explain why we should not leave the new state on top of tarantool_L
> >    in luaT_temp_luastate().
> > 
> > There are two reasons, why leaving 'garbage' on tarantool_L is not
> > acceptable here. I want to mention both here.
> > 
> > I reformatted the comment a bit to make it more clear:
> > 
> >  | /*
> >  |  * Unlike lua_newthread(), luaT_newthread() does not leave
> >  |  * the new Lua state on tarantool_L.
> 
> I was around to it today and unfortunately it does[1]. So you need to
> explicitly pop a newly created coroutine from the guest stack right
> after anchoring it to the registry.

Ouch! I was sure that it does not leave the value... It seems I misread
the source. Many thanks for catching this!

I verified the actual behaviour, you're right: luaT_newthread() works
just like lua_newthread().

There is luaL_ref() below, which pops the thread from tarantool_L, so
the actual behaviour is correct. I moved and rephrased the comment:

 | /* Popped by luaL_ref(). */
 | struct lua_State *L = luaT_newthread(tarantool_L);
 | if (L == NULL) 
 |         return NULL;
 | /*
 |  * We should remove the reference to the newly created Lua
 |  * thread from tarantool_L, because of two reasons:
 |  *
 |  * First, if we'll push something to tarantool_L and
 |  * yield, then another fiber will not know that a stack
 |  * top is changed and may operate on a wrong slot.
 |  *
 |  * Second, many requests that push a value to tarantool_L
 |  * and yield may exhaust available slots on the stack. It
 |  * is limited by LUAI_MAXSTACK build time constant (~65K).
 |  *
 |  * We cannot just pop the value, but should keep the
 |  * reference in the registry while it is in use.
 |  * Otherwise it may be garbage collected.
 |  */
 | *coro_ref = luaL_ref(tarantool_L, LUA_REGISTRYINDEX);

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

[Alexander Turenko]

merge_source_next() implementations for 'buffer', 'table' and 'tuple'
sources use a temporary Lua state to process data passed from Lua. It
may use the fiber-local Lua state when it is present. In this case the
state is not freed when all operations are done and may be reused later.
We should ensure that merge_source_next() code do not leave extra values
on the stack, because otherwise the stack may be overflowed.

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
function. After a Lua/C call a Lua interpreter automatically removes
everything below returned values. So the stack will not accumulate any
garbage.

However the merge source API is desined in the way to allow to use any
merge source from C. If we'll use it this way in a future, we can meet
the described problem.

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
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.

Reported-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>

Follows up #4954
---
 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
@@ -164,18 +164,27 @@ luaT_gettuple(struct lua_State *L, int idx, struct tuple_format *format)
  * wrong stack slot when it will be scheduled for execution after
  * yield.
  *
- * Return a Lua state on success and set @a coro_ref. This
- * reference should be passed to `luaT_release_temp_luastate()`,
- * when the state is not needed anymore.
+ * Return a Lua state on success and set @a coro_ref and @a top.
+ * These values should be passed to
+ * `luaT_release_temp_luastate()`, when the state is not needed
+ * anymore.
  *
  * Return NULL and set a diag at failure.
  */
 static struct lua_State *
-luaT_temp_luastate(int *coro_ref)
+luaT_temp_luastate(int *coro_ref, int *top)
 {
 	if (fiber()->storage.lua.stack != NULL) {
+		/*
+		 * Reuse existing stack. In the releasing function
+		 * we should drop a stack top to its initial
+		 * value to don't exhaust available slots by
+		 * many requests in row.
+		 */
+		struct lua_State *L = fiber()->storage.lua.stack;
 		*coro_ref = LUA_REFNIL;
-		return fiber()->storage.lua.stack;
+		*top = lua_gettop(L);
+		return L;
 	}
 
 	/*
@@ -197,6 +206,7 @@ luaT_temp_luastate(int *coro_ref)
 	 * registry while it is in use.
 	 */
 	*coro_ref = luaL_ref(tarantool_L, LUA_REGISTRYINDEX);
+	*top = -1;
 	return L;
 }
 
@@ -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);
 }
 
@@ -521,11 +527,12 @@ static int
 luaL_merge_source_buffer_fetch(struct merge_source_buffer *source)
 {
 	int coro_ref = LUA_REFNIL;
-	struct lua_State *L = luaT_temp_luastate(&coro_ref);
+	int top = -1;
+	struct lua_State *L = luaT_temp_luastate(&coro_ref, &top);
 	if (L == NULL)
 		return -1;
 	int rc = luaL_merge_source_buffer_fetch_impl(L, source);
-	luaT_release_temp_luastate(coro_ref);
+	luaT_release_temp_luastate(L, coro_ref, top);
 	return rc;
 }
 
@@ -799,11 +806,12 @@ luaL_merge_source_table_next(struct merge_source *base,
 			     struct tuple **out)
 {
 	int coro_ref = LUA_REFNIL;
-	struct lua_State *L = luaT_temp_luastate(&coro_ref);
+	int top = -1;
+	struct lua_State *L = luaT_temp_luastate(&coro_ref, &top);
 	if (L == NULL)
 		return -1;
 	int rc = luaL_merge_source_table_next_impl(L, base, format, out);
-	luaT_release_temp_luastate(coro_ref);
+	luaT_release_temp_luastate(L, coro_ref, top);
 	return rc;
 }
 
@@ -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 "
 			 "return values", nresult);
 		return -1;
 	}
@@ -973,11 +981,12 @@ luaL_merge_source_tuple_next(struct merge_source *base,
 			     struct tuple **out)
 {
 	int coro_ref = LUA_REFNIL;
-	struct lua_State *L = luaT_temp_luastate(&coro_ref);
+	int top = -1;
+	struct lua_State *L = luaT_temp_luastate(&coro_ref, &top);
 	if (L == NULL)
 		return -1;
 	int rc = luaL_merge_source_tuple_next_impl(L, base, format, out);
-	luaT_release_temp_luastate(coro_ref);
+	luaT_release_temp_luastate(L, coro_ref, top);
 	return rc;
 }
 
diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index 697d1b21d..63e5b5015 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -65,6 +65,7 @@ add_custom_target(test-force
 add_subdirectory(app)
 add_subdirectory(app-tap)
 add_subdirectory(box)
+add_subdirectory(box-tap)
 add_subdirectory(unit)
 add_subdirectory(${PROJECT_SOURCE_DIR}/third_party/luajit/test/gh-4427-ffi-sandwich ${PROJECT_BINARY_DIR}/third_party/luajit/test/gh-4427-ffi-sandwich)
 add_subdirectory(${PROJECT_SOURCE_DIR}/third_party/luajit/test/lj-flush-on-trace ${PROJECT_BINARY_DIR}/third_party/luajit/test/lj-flush-on-trace)
diff --git a/test/box-tap/CMakeLists.txt b/test/box-tap/CMakeLists.txt
new file mode 100644
index 000000000..a39898bd8
--- /dev/null
+++ b/test/box-tap/CMakeLists.txt
@@ -0,0 +1,4 @@
+# fiber.h requires tarantool_ev.h from third_party directory.
+include_directories(${CMAKE_SOURCE_DIR}/third_party)
+
+build_module(check_merge_source check_merge_source.c)
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 @@
+#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(),
+			    * struct tuple (opaque)
+			    */
+
+/**
+ * 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.
+ */
+
+/**
+ * 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.
+ *
+ * The function is to be called from Lua. Lua API is the
+ * following:
+ *
+ * Parameters:
+ *
+ * - merge_source   A merge source object to call
+ *                  merge_source_next() on it.
+ *
+ * Return values:
+ *
+ * - is_next_ok     Whether the call is successful.
+ * - err_msg        Error message from the call or nil.
+ * - is_stack_even  Whether the fiber-local Lua stack is
+ *                  even after the call.
+ */
+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);
+
+	int top = lua_gettop(temporary_L);
+	int rc = merge_source_next(source, NULL, &tuple);
+	(void) tuple;
+	bool is_stack_even = lua_gettop(temporary_L) == top;
+	struct error *e = diag_last_error(diag_get());
+
+	lua_pushboolean(L, rc == 0);
+	if (rc == 0)
+		lua_pushnil(L);
+	else
+		lua_pushstring(L, e->errmsg);
+	lua_pushboolean(L, is_stack_even);
+	return 3;
+}
+
+/**
+ * Register the module.
+ */
+LUA_API int
+luaopen_check_merge_source(struct lua_State *L)
+{
+	static const struct luaL_Reg meta[] = {
+		{"call_next", lbox_check_merge_source_call_next},
+		{NULL, NULL}
+	};
+	luaL_register(L, "merge_source", meta);
+	return 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..9fbb0919a
--- /dev/null
+++ b/test/box-tap/gh-4954-merger-via-c.test.lua
@@ -0,0 +1,251 @@
+#!/usr/bin/env tarantool
+
+--
+-- gh-4954: The fiber-local Lua stack should be even after
+-- merge_source_next() call from C code.
+--
+-- See test/box-tap/check_merge_source.c for more information.
+--
+
+local fio = require('fio')
+
+-- Use BUILDDIR passed from test-run or cwd when run w/o
+-- test-run to find test/box-tap/check_merge_source.{so,dylib}.
+local build_path = os.getenv('BUILDDIR') or '.'
+package.cpath = fio.pathjoin(build_path, 'test/box-tap/?.so'   ) .. ';' ..
+                fio.pathjoin(build_path, 'test/box-tap/?.dylib') .. ';' ..
+                package.cpath
+
+local buffer = require('buffer')
+local msgpack = require('msgpack')
+local merger = require('merger')
+local tap = require('tap')
+local check_merge_source = require('check_merge_source')
+
+-- {{{ Lua iterator generator functions
+
+local function triplet()
+    return 1, 2, 3
+end
+
+local function wrong_type()
+    return 1, 2
+end
+
+local function no_chunks()
+    return nil
+end
+
+local function bad_buffer()
+    local buf = buffer.ibuf()
+    msgpack.encode({foo = 'bar'}, buf)
+    return 1, buf
+end
+
+-- FIXME: Enable after gh-5048: ('non-array tuple in a buffer
+-- leads to assertion fail').
+--[[
+local function bad_tuple_in_buffer()
+    local tuple = 1
+    local buf = buffer.ibuf()
+    msgpack.encode({tuple}, buf)
+    return 1, buf
+end
+]]--
+
+local function empty_buffer(_, state)
+    if state ~= nil then
+        return nil
+    end
+    local buf = buffer.ibuf()
+    return 1, buf
+end
+
+local function no_tuples_buffer(_, state)
+    if state ~= nil then
+        return nil
+    end
+    local buf = buffer.ibuf()
+    msgpack.encode({}, buf)
+    return 1, buf
+end
+
+local function good_buffer(_, state)
+    if state ~= nil then
+        return nil
+    end
+    local buf = buffer.ibuf()
+    local tuple = {1, 2, 3}
+    msgpack.encode({tuple}, buf)
+    return 1, buf
+end
+
+local function bad_tuple_in_table()
+    local tuple = 1
+    return 1, {tuple}
+end
+
+local function empty_table(_, state)
+    if state ~= nil then
+        return nil
+    end
+    return 1, {}
+end
+
+local function good_table(_, state)
+    if state ~= nil then
+        return nil
+    end
+    local tuple = {1, 2, 3}
+    return 1, {tuple}
+end
+
+local function bad_tuple()
+    local tuple = 1
+    return 1, tuple
+end
+
+local function good_tuple(_, state)
+    if state ~= nil then
+        return nil
+    end
+    local tuple = {1, 2, 3}
+    return 1, tuple
+end
+
+-- }}}
+
+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').
+    --[[
+    {
+        '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$',
+    },
+    ]]--
+    {
+        'buffer source, no buffers',
+        source_new = merger.new_buffer_source,
+        source_gen = no_chunks,
+    },
+    {
+        'buffer source, empty buffer',
+        source_new = merger.new_buffer_source,
+        source_gen = empty_buffer,
+    },
+    {
+        'buffer source, no tuples buffer',
+        source_new = merger.new_buffer_source,
+        source_gen = no_tuples_buffer,
+    },
+    {
+        'buffer source, good buffer',
+        source_new = merger.new_buffer_source,
+        source_gen = good_buffer,
+    },
+    {
+        'table source, bad gen function',
+        source_new = merger.new_table_source,
+        source_gen = triplet,
+        exp_err = '^Expected <state>, <table>, got 3 return values$',
+    },
+    {
+        'table source, bad gen result',
+        source_new = merger.new_table_source,
+        source_gen = wrong_type,
+        exp_err = '^Expected <state>, <table>$',
+    },
+    {
+        'table source, bad tuple in table',
+        source_new = merger.new_table_source,
+        source_gen = bad_tuple_in_table,
+        exp_err = '^A tuple or a table expected, got number$',
+    },
+    {
+        'buffer source, no tables',
+        source_new = merger.new_table_source,
+        source_gen = no_chunks,
+    },
+    {
+        'table source, empty table',
+        source_new = merger.new_table_source,
+        source_gen = empty_table,
+    },
+    {
+        'table source, good table',
+        source_new = merger.new_table_source,
+        source_gen = good_table,
+    },
+    {
+        'tuple source, bad gen function',
+        source_new = merger.new_tuple_source,
+        source_gen = triplet,
+        exp_err = '^Expected <state>, <tuple>, got 3 return values$',
+    },
+    {
+        'tuple source, bad gen result',
+        source_new = merger.new_tuple_source,
+        source_gen = wrong_type,
+        exp_err = '^A tuple or a table expected, got number$',
+    },
+    {
+        'tuple source, bad tuple',
+        source_new = merger.new_tuple_source,
+        source_gen = bad_tuple,
+        exp_err = '^A tuple or a table expected, got number$',
+    },
+    {
+        'tuple source, no tuples',
+        source_new = merger.new_tuple_source,
+        source_gen = no_chunks,
+    },
+    {
+        'tuple source, good tuple',
+        source_new = merger.new_tuple_source,
+        source_gen = good_tuple,
+    },
+}
+
+local test = tap.test('gh-4954-merger-via-c')
+test:plan(#cases)
+
+for _, case in ipairs(cases) do
+    test:test(case[1], function(test)
+        test:plan(3)
+        local source = case.source_new(case.source_gen)
+        local is_next_ok, err_msg, is_stack_even =
+            check_merge_source.call_next(source)
+        if case.exp_err == nil then
+            test:ok(is_next_ok, 'merge_source_next() should succeed')
+            test:ok(err_msg == nil, 'no error message')
+        else
+            test:ok(not is_next_ok, 'merge_source_next() should fail')
+            test:ok(string.match(err_msg, case.exp_err), 'verify error message',
+                                 {err_msg = err_msg, exp_err = case.exp_err})
+        end
+        test:ok(is_stack_even, 'fiber-local Lua stack should be even')
+    end)
+end
+
+os.exit(test:check() and 0 or 1)
-- 
2.25.0

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

[Alexander Turenko]

> 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 @@
> +#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(),
> +			    * struct tuple (opaque)
> +			    */

Dropped those comments after offline discussion with teammates.

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

[Igor Munkin]

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

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

[Alexander Turenko]

> > 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/.

Cited from [1]:

 | "As presented" (verb) connotes deliberate placement. "As present"
 | (adjective) just means it's there.

'It is there' meanging fits better here, IMHO.

Isn't I miss something about grammar here?

[1]: https://www.instructionalsolutions.com/blog/bid/102954/Tables-in-Report-Writing-Presented-or-Present

> > 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/.

Thanks! Fixed.

> > @@ -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.

Are you propose the following?

 | if (top >= 0)
 |         lua_settop(L, top);
 | else
 |         luaL_unref(tarantool_L, LUA_REGISTRYINDEX, coro_ref);

When I look into the code locally, it does not seem to be logical: why
decision whether we should unreference a state should be made based on
`top` value?

Now the code seem to be logical for me: if `top` was saved, then we drop
it. If reference was saved, we unreference it.

Vlad even proposed to drop `top >= 0`, which works in fact, but Lua
Reference Manual does not guarantee it (it was discussed within this
mailing thread).

> > +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.

No-no, it is always so for a Lua born fiber. It seems I should reword
the comment in a more strict way and explain why I use <temporary_L>
explicitly despite the fact that it is always the same as <L>.

The new comment:

 | /*
 |  * Ensure that there is a reusable temporary Lua stack.
 |  *
 |  * Note: It is the same as <L> for a Lua born fiber (at
 |  * least at the moment of writing), but it is the
 |  * implementation detail and the test looks more clean
 |  * when we don't lean on this fact.
 |  */

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

[Igor Munkin]

Sasha,

Thanks for your comments, LGTM!

On 16.07.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/.
> 
> Cited from [1]:
> 
>  | "As presented" (verb) connotes deliberate placement. "As present"
>  | (adjective) just means it's there.
> 
> 'It is there' meanging fits better here, IMHO.
> 
> Isn't I miss something about grammar here?

I guess no, thanks for clarification!

> 
> [1]: https://www.instructionalsolutions.com/blog/bid/102954/Tables-in-Report-Writing-Presented-or-Present
> 

<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.
> 
> Are you propose the following?
> 
>  | if (top >= 0)
>  |         lua_settop(L, top);
>  | else
>  |         luaL_unref(tarantool_L, LUA_REGISTRYINDEX, coro_ref);

Kinda, but with <coro_ref> in condition:
| if (coro_ref == LUA_NOREF)
| 	lua_settop(L, top);
| else
| 	luaL_unref(tarantool_L, LUA_REGISTRYINDEX, coro_ref);

Feel free to ignore this comment if it doesn't looks logical to you.

> 

<snipped>

> 
> Vlad even proposed to drop `top >= 0`, which works in fact, but Lua
> Reference Manual does not guarantee it (it was discussed within this
> mailing thread).

I see Reference Manual allows any integer number to be passed as <idx>
but I agree that behaviour for negative values are not clear from it.
JFYI, LuaJIT works the same way here vanilla Lua 5.1 does. So I'm much
closer to Vlad's opinion here, but doesn't insist on such <lua_settop>
usage, since it might not be clear from Manual.

> 
> > > +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.
> 
> No-no, it is always so for a Lua born fiber. It seems I should reword
> the comment in a more strict way and explain why I use <temporary_L>
> explicitly despite the fact that it is always the same as <L>.
> 
> The new comment:
> 
>  | /*
>  |  * Ensure that there is a reusable temporary Lua stack.
>  |  *
>  |  * Note: It is the same as <L> for a Lua born fiber (at
>  |  * least at the moment of writing), but it is the
>  |  * implementation detail and the test looks more clean
>  |  * when we don't lean on this fact.
>  |  */

OK, now it's clear, thanks!

-- 
Best regards,
IM

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

[Alexander Turenko]

> > > > @@ -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.
> > 
> > Are you propose the following?
> > 
> >  | if (top >= 0)
> >  |         lua_settop(L, top);
> >  | else
> >  |         luaL_unref(tarantool_L, LUA_REGISTRYINDEX, coro_ref);
> 
> Kinda, but with <coro_ref> in condition:
> | if (coro_ref == LUA_NOREF)
> | 	lua_settop(L, top);
> | else
> | 	luaL_unref(tarantool_L, LUA_REGISTRYINDEX, coro_ref);
> 
> Feel free to ignore this comment if it doesn't looks logical to you.

To be honest, I would prefer to keep the current way:
luaT_temp_luastate() fills <top> and <coro_ref> and they separately
control whether lua_settop() and luaL_unref() will be called / will have
an effect in luaT_release_temp_luastate(). It is quite straightforward.

But I understand that when lua_settop() has the check, but luaL_unref()
has no, it looks a bit lopsided and it is quite natural to want to make
it visually balanced.

> > Vlad even proposed to drop `top >= 0`, which works in fact, but Lua
> > Reference Manual does not guarantee it (it was discussed within this
> > mailing thread).
> 
> I see Reference Manual allows any integer number to be passed as <idx>
> but I agree that behaviour for negative values are not clear from it.
> JFYI, LuaJIT works the same way here vanilla Lua 5.1 does. So I'm much
> closer to Vlad's opinion here, but doesn't insist on such <lua_settop>
> usage, since it might not be clear from Manual.

I'm quite sure we just have no such guarantee.

I disagree with the point that it is guaranteed, but in some unclear
way. No. I performed an attempt to find what exactly guarantees this
behaviour and found nothing. I described it in [1].

Since it unlikely will be changed, we can remove the check and leave a
comment to describe the behaviour we lean on. But the check just
shorter and cleaner, I don't see a reason to replace it with the
comment.

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-June/017750.html

[Tarantool-patches] [PATCH v2 3/3] lua: expose temporary Lua state for iproto calls

[Alexander Turenko]

There is code that may save some time and resources for creating a new
Lua state when it is present in the fiber storage of a current fiber.
There are not so much of them: running a Lua trigger and construction of
a next tuple in a merge source.

Before the patch, fiber->storage.lua.stack is filled only for the main
fiber and fibers created from Lua using fiber.create() or fiber.new(),
but not for background fibers (which serve binary protocol requests).

This patch fills fiber->storage.lua.stack for background fibers that
serve a Lua call or eval: we already have this state and nothing prevent
us from exposing it via the fiber storage.

Follows up #4954
---
 src/box/lua/call.c   | 27 +++++++++++++++++++++++++++
 src/lib/core/fiber.h | 14 ++++++++++----
 2 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/src/box/lua/call.c b/src/box/lua/call.c
index 6588ec2fa..ccdef6662 100644
--- a/src/box/lua/call.c
+++ b/src/box/lua/call.c
@@ -537,12 +537,39 @@ box_process_lua(lua_CFunction handler, struct execute_lua_ctx *ctx,
 	port_lua_create(ret, L);
 	((struct port_lua *) ret)->ref = coro_ref;
 
+	/*
+	 * A code that need a temporary fiber-local Lua state may
+	 * save some time and resources for creating a new state
+	 * and use this one.
+	 */
+	bool has_lua_stack = fiber()->storage.lua.stack != NULL;
+	if (!has_lua_stack)
+		fiber()->storage.lua.stack = L;
+
 	lua_pushcfunction(L, handler);
 	lua_pushlightuserdata(L, ctx);
 	if (luaT_call(L, 1, LUA_MULTRET) != 0) {
+		if (!has_lua_stack)
+			fiber()->storage.lua.stack = NULL;
 		port_lua_destroy(ret);
 		return -1;
 	}
+
+	/*
+	 * Since this field is optional we're not obligated to
+	 * keep it until the Lua state will be unreferenced in
+	 * port_lua_destroy().
+	 *
+	 * There is no much sense to keep it beyond the Lua call,
+	 * so let's zap now.
+	 *
+	 * But: keep the stack if it was present before the call,
+	 * because it would be counter-intuitive if the existing
+	 * state pointer would be zapped after this function call.
+	 */
+	if (!has_lua_stack)
+		fiber()->storage.lua.stack = NULL;
+
 	return 0;
 }
 
diff --git a/src/lib/core/fiber.h b/src/lib/core/fiber.h
index cd9346a55..2ff0b4009 100644
--- a/src/lib/core/fiber.h
+++ b/src/lib/core/fiber.h
@@ -495,12 +495,18 @@ struct fiber {
 		struct session *session;
 		struct credentials *credentials;
 		struct txn *txn;
-		/**
-		 * Lua stack and the optional
-		 * fiber.storage Lua reference.
-		 */
+		/** Fields related to Lua code execution. */
 		struct {
+			/**
+			 * Optional Lua state (may be NULL).
+			 * Useful as a temporary Lua state to save
+			 * time and resources on creating it.
+			 * Should not be used in other fibers.
+			 */
 			struct lua_State *stack;
+			/**
+			 * Optional fiber.storage Lua reference.
+			 */
 			int ref;
 		} lua;
 		/**
-- 
2.25.0

Re: [Tarantool-patches] [PATCH v2 3/3] lua: expose temporary Lua state for iproto calls

[Igor Munkin]

Sasha,

Thanks for the patch! It LGTM except the nits I mentioned below.

On 18.06.20, Alexander Turenko wrote:
> There is code that may save some time and resources for creating a new
> Lua state when it is present in the fiber storage of a current fiber.

Typo: s/present/presented/.

> There are not so much of them: running a Lua trigger and construction of
> a next tuple in a merge source.

<snipped>

> This patch fills fiber->storage.lua.stack for background fibers that
> serve a Lua call or eval: we already have this state and nothing prevent

Typo: s/prevent/prevents/.

> us from exposing it via the fiber storage.
> 
> Follows up #4954
> ---
>  src/box/lua/call.c   | 27 +++++++++++++++++++++++++++
>  src/lib/core/fiber.h | 14 ++++++++++----
>  2 files changed, 37 insertions(+), 4 deletions(-)
> 
> diff --git a/src/box/lua/call.c b/src/box/lua/call.c
> index 6588ec2fa..ccdef6662 100644
> --- a/src/box/lua/call.c
> +++ b/src/box/lua/call.c
> @@ -537,12 +537,39 @@ box_process_lua(lua_CFunction handler, struct execute_lua_ctx *ctx,
>  	port_lua_create(ret, L);
>  	((struct port_lua *) ret)->ref = coro_ref;
>  
> +	/*
> +	 * A code that need a temporary fiber-local Lua state may
> +	 * save some time and resources for creating a new state
> +	 * and use this one.
> +	 */

Could you please provide an example for the fiber calling this function
with non-NULL fiber-local Lua state? Are those conditions below are
strictly required by the current implementation?

> +	bool has_lua_stack = fiber()->storage.lua.stack != NULL;
> +	if (!has_lua_stack)
> +		fiber()->storage.lua.stack = L;
> +
>  	lua_pushcfunction(L, handler);
>  	lua_pushlightuserdata(L, ctx);
>  	if (luaT_call(L, 1, LUA_MULTRET) != 0) {
> +		if (!has_lua_stack)
> +			fiber()->storage.lua.stack = NULL;
>  		port_lua_destroy(ret);
>  		return -1;
>  	}
> +
> +	/*
> +	 * Since this field is optional we're not obligated to
> +	 * keep it until the Lua state will be unreferenced in
> +	 * port_lua_destroy().
> +	 *
> +	 * There is no much sense to keep it beyond the Lua call,
> +	 * so let's zap now.
> +	 *
> +	 * But: keep the stack if it was present before the call,
> +	 * because it would be counter-intuitive if the existing
> +	 * state pointer would be zapped after this function call.
> +	 */
> +	if (!has_lua_stack)
> +		fiber()->storage.lua.stack = NULL;
> +
>  	return 0;
>  }
>  

<snipped>

> -- 
> 2.25.0
> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH v2 3/3] lua: expose temporary Lua state for iproto calls

[Alexander Turenko]

> > There is code that may save some time and resources for creating a new
> > Lua state when it is present in the fiber storage of a current fiber.
> 
> Typo: s/present/presented/.

According to [1] the word may be considered as an adjective and so using
it as 'present' here is technically correct. This article gives the
following difference in meanging:

 | "As presented" (verb) connotes deliberate placement. "As present"
 | (adjective) just means it's there.

'It is there' is what I want to express here, so 'present' looks as the
better choice here.

Does not I miss something about grammar here?

[1]: https://www.instructionalsolutions.com/blog/bid/102954/Tables-in-Report-Writing-Presented-or-Present

> > This patch fills fiber->storage.lua.stack for background fibers that
> > serve a Lua call or eval: we already have this state and nothing prevent
> 
> Typo: s/prevent/prevents/.

Thanks! Fixed.

> > diff --git a/src/box/lua/call.c b/src/box/lua/call.c
> > index 6588ec2fa..ccdef6662 100644
> > --- a/src/box/lua/call.c
> > +++ b/src/box/lua/call.c
> > @@ -537,12 +537,39 @@ box_process_lua(lua_CFunction handler, struct execute_lua_ctx *ctx,
> >  	port_lua_create(ret, L);
> >  	((struct port_lua *) ret)->ref = coro_ref;
> >  
> > +	/*
> > +	 * A code that need a temporary fiber-local Lua state may
> > +	 * save some time and resources for creating a new state
> > +	 * and use this one.
> > +	 */
> 
> Could you please provide an example for the fiber calling this function
> with non-NULL fiber-local Lua state?

Sure.

 | tarantool> box.cfg{}
 | tarantool> echo = function(...) return ... end
 | tarantool> box.schema.func.create('echo')
 | tarantool> box.schema.func.call('echo', {1, 2, 3})

I added the assert and verified it just in case:

 | diff --git a/src/box/lua/call.c b/src/box/lua/call.c
 | index 0315e720c..0221ffd2d 100644
 | --- a/src/box/lua/call.c
 | +++ b/src/box/lua/call.c
 | @@ -561,6 +561,7 @@ box_process_lua(enum handlers handler, struct execute_lua_ctx *ctx,
 |          * and use this one.
 |          */
 |         bool has_lua_stack = fiber()->storage.lua.stack != NULL;
 | +       assert(fiber()->storage.lua.stack == NULL);
 |         if (!has_lua_stack)
 |                 fiber()->storage.lua.stack = L;

The assert fails after the steps above.

(But even if it would not be possible, I would write the code this way
to don't lean on not-so-obvious details that may be changed in a
future.)

> Are those conditions below are strictly required by the current
> implementation?

When the fiber-local Lua state is present it should not be changed or
zapped by the function. I don't know whether it would lead to some
negative behaviour changes, but it would be at least counter-intuitive.
There is the comment on the topic (I left it cited below).

> > +	 * But: keep the stack if it was present before the call,
> > +	 * because it would be counter-intuitive if the existing
> > +	 * state pointer would be zapped after this function call.
> > +	 */
> > +	if (!has_lua_stack)
> > +		fiber()->storage.lua.stack = NULL;

BTW, 'present' is here again. Don't know what form is better here, but
left it as is now.

Re: [Tarantool-patches] [PATCH v2 3/3] lua: expose temporary Lua state for iproto calls

[Igor Munkin]

Sasha,

Thanks for the comments, LGTM!

On 16.07.20, Alexander Turenko wrote:

<snipped>

> > > diff --git a/src/box/lua/call.c b/src/box/lua/call.c
> > > index 6588ec2fa..ccdef6662 100644
> > > --- a/src/box/lua/call.c
> > > +++ b/src/box/lua/call.c
> > > @@ -537,12 +537,39 @@ box_process_lua(lua_CFunction handler, struct execute_lua_ctx *ctx,
> > >  	port_lua_create(ret, L);
> > >  	((struct port_lua *) ret)->ref = coro_ref;
> > >  
> > > +	/*
> > > +	 * A code that need a temporary fiber-local Lua state may
> > > +	 * save some time and resources for creating a new state
> > > +	 * and use this one.
> > > +	 */
> > 
> > Could you please provide an example for the fiber calling this function
> > with non-NULL fiber-local Lua state?
> 
> Sure.
> 
>  | tarantool> box.cfg{}
>  | tarantool> echo = function(...) return ... end
>  | tarantool> box.schema.func.create('echo')
>  | tarantool> box.schema.func.call('echo', {1, 2, 3})

Well, I expected this one, thanks.

> 
> I added the assert and verified it just in case:
> 
>  | diff --git a/src/box/lua/call.c b/src/box/lua/call.c
>  | index 0315e720c..0221ffd2d 100644
>  | --- a/src/box/lua/call.c
>  | +++ b/src/box/lua/call.c
>  | @@ -561,6 +561,7 @@ box_process_lua(enum handlers handler, struct execute_lua_ctx *ctx,
>  |          * and use this one.
>  |          */
>  |         bool has_lua_stack = fiber()->storage.lua.stack != NULL;
>  | +       assert(fiber()->storage.lua.stack == NULL);
>  |         if (!has_lua_stack)
>  |                 fiber()->storage.lua.stack = L;
> 
> The assert fails after the steps above.
> 
> (But even if it would not be possible, I would write the code this way
> to don't lean on not-so-obvious details that may be changed in a
> future.)
> 
> > Are those conditions below are strictly required by the current
> > implementation?
> 
> When the fiber-local Lua state is present it should not be changed or
> zapped by the function. I don't know whether it would lead to some
> negative behaviour changes, but it would be at least counter-intuitive.
> There is the comment on the topic (I left it cited below).

My last sentence relates to the check whether fiber-local Lua state is
present. Long story short, I see no reasons to omit this field
initialization prior to <box_process_lua> call. Feel free to consider
this one as a side note with no changes required.

> 
> > > +	 * But: keep the stack if it was present before the call,
> > > +	 * because it would be counter-intuitive if the existing
> > > +	 * state pointer would be zapped after this function call.
> > > +	 */
> > > +	if (!has_lua_stack)
> > > +		fiber()->storage.lua.stack = NULL;
> 
> BTW, 'present' is here again. Don't know what form is better here, but
> left it as is now.

I'm OK with this wording after your clarifications.

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH v2 3/3] lua: expose temporary Lua state for iproto calls

[Alexander Turenko]

> > > Are those conditions below are strictly required by the current
> > > implementation?
> > 
> > When the fiber-local Lua state is present it should not be changed or
> > zapped by the function. I don't know whether it would lead to some
> > negative behaviour changes, but it would be at least counter-intuitive.
> > There is the comment on the topic (I left it cited below).
> 
> My last sentence relates to the check whether fiber-local Lua state is
> present. Long story short, I see no reasons to omit this field
> initialization prior to <box_process_lua> call. Feel free to consider
> this one as a side note with no changes required.

Igor clarified his idea in the offline discussion. Now box_process_lua()
always creates a new Lua thread to serve a call request, but it may
reuse the fiber-local Lua state when it exists. Similarly how
lbox_trigger_run() either use the existing state or create a new one
(see [1]).

The idea looks meaningful. I experimented around and got failed
sql/boolean.test.sql test. It seems I unable to provide a ready patch at
the time: at least I need to debug and polish it, think how to test it,
at max I should look on reusing a Lua state at whole and elaborate
possible approaches.

I think we can proceed with the current 'lua: expose temporary Lua state
for iproto calls' patch, which prevents twice creation of a Lua thread
for background fibers when merger is used or a trigger is run. And then
work on preventing twice creation of a thread for Lua-born fibers (say,
when <box.schema.func.call> is used). Usage of this function is quite
rare, I guess.

[1]: https://github.com/tarantool/tarantool/commit/7d0408486571c631e796a90a35adae41cfcd53c9

Re: [Tarantool-patches] [PATCH v2 0/3] Merger's NULL defererence

[Vladislav Shpilevoy]

LGTM.

Re: [Tarantool-patches] [PATCH v2 0/3] Merger's NULL defererence

[Alexander Turenko]

> Changelog entry
> ---------------
> 
> @ChangeLog
> 
> - merger: fix NULL pointer dereference when merger is called via the
>   binary protocol (say, via net.box) (gh-4954)
> 
> Links
> -----
> 
> https://github.com/tarantool/tarantool/issues/4954
> Totktonada/gh-4954-fix-merger-segfault-full-ci

Made all suggested review fixes (or discussed where I disagree). Thank
you, Vlad and Igor!

Pushed to master.

Pushed to 2.4. Removed box-tap/gh-4954-merger-via-c.test.lua from the
2nd commit, because it depends on exposing all symbols from the
executable (#2971).

Pushed to 2.3. Applied the same changes as for 2.4.

Updated all relevant release notes with the changelog entry.

Removed the Totktonada/gh-4954-fix-merger-segfault-full-ci branch.

WBR, Alexander Turenko.