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

[Alexander Turenko <alexander.turenko@tarantool.org>]

Thanks for the careful review!

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

I'm not sure we can lean on this. See, Lua 5.1 Reference Manual [1] states the
following about lua_settop():

 | Accepts any acceptable index, or 0, and sets the stack top to this index.

And defines an acceptable index as follows:

 | More formally, we define an acceptable index as follows:
 |
 |     (index < 0 && abs(index) <= top) ||
 |     (index > 0 && index <= stackspace)

However LuaJIT has the following condition:

 | LUA_API void lua_settop(lua_State *L, int idx)
 | {
 |   if (idx >= 0) {
 |     <...>
 |   } else {
 |     api_check(L, -(idx+1) <= (L->top - L->base));
 |     L->top += idx+1;  /* Shrinks top (idx < 0). */
 |   }
 | }

The api_check() call allows a negative index to point to a stack slot
below the current top by 1. It looks as the implementation detail.

I was wonder from where the condition comes. It seems, LuaJIT borrows it
from PUC-Rio Lua 5.1 and it was introduced in [2] right with this extra
+1.

NB: Widely used index2addr() LuaJIT internal function has the following
check for a negative index:

 | api_check(L, idx != 0 && -idx <= L->top - L->base);

And index2addr() is used in lua_toboolean(), which accepts an acceptable
index according to the manual.

I would better follow the API rather then lean on implementation
details and provide a comment with clarification why it works.

[1]: https://www.lua.org/manual/5.1/manual.html
[2]: https://github.com/lua/lua/commit/255052b6c6a1b109c93b104c03de8968b56dcd5a

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

box-tap/gh-4954-merger-via-c.test.lua has three 'bad gen function' test
cases and all expected errors in them are structured in the same way. It
would be strange to verify an that an error message contains a typo
(especially, when only one of them has it).

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

I actively use them during the initial development: when I remove some
experimental code, I verify whether I should remove some header.

There is nothing bad if it becomes a bit outdated: some of headers used
more, some becomes unused. If one want to clean them up, those comments
will give an idea how to obtain possibly unused headers using simple
pattern matching. Then the list of possibly unused headers may be
verified by removing them and try to compile.

I find it convenient sometimes, so I would prefer to leave the comments
(if you don't strongly disagree).

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

It sounds reasonable: I cannot use merge source methods without at least
opaque <struct tuple> definition. Removed the declaration and added
'struct tuple (opaque)' to the '#include "box/merger.h"' comment.

Also fixed comment style:

 #include "box/merger.h"    /* struct merge_source,
-                             merge_source_next() */
+                           * merge_source_next(),
+                           * struct tuple (opaque)
+                           */

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

The function has the Lua API and I want to describe it in Lua terms: it
accepts a merge source and returns three values. However I agree that I
should not use Doxygen markup for this sake, because it will contradict
with actual C definition.

Since we have no formal agreement how to document Lua and Lua/C
functions (and tooling around it), it seems I should not use any special
markup and just describe parameters and return values in a free form.

I wrote it this way:

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

Note: Vlad points me a couple of examples that describe Lua API in
Lua/C terms:

 | /**
 |  * Push a message using a protocol, depending on a session type.
 |  * @param L Lua state. First argument on the stack is data to
 |  *        push.
 |  * @retval 1 Success, true is pushed.
 |  * @retval 2 Error. Nil and error object are pushed.
 |  */
 | static int
 | lbox_session_push(struct lua_State *L)

 | /**
 |  * Create a new SWIM instance. SWIM is not created via FFI,
 |  * because this operation yields.
 |  * @retval 1 Success. A SWIM instance pointer is on the stack.
 |  * @retval 2 Error. Nil and an error object are pushed.
 |  */
 | static int
 | lua_swim_new(struct lua_State *L)

I'm not very comfortable with this way, the reasons are the following:

- Just not so intuitive as a description in usual Lua terms.
- Usually amount of returned values is not most important information,
  but, say, whether a first one is nil is important. We should point a
  user a way to use the API right in the API description: success case
  is when the first retval is not nil in the first place, not when a
  function returns one value.
- It requires some knowledge about Lua/C API and so it is not acceptable
  as a source to generate user-visible API documentation for public Lua
  methods.
- It will change if a function will be reimplemented using pure Lua /
  LuaJIT FFI despite that an actual API (a way to call and use) will not
  be changed.

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

Okay, fixed.

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

It is one of test cases for gh-4954, but it is blocked by gh-5048. I'll
enable it here with gh-5048 and will add a specific test case either to
box-tap/merger.test.lua (because it already has truncated buffer cases)
or as a separate file (not sure what is actually better, but a formal
process requires the latter).

The gh-5048 case will not check stack size (just error message), call a
Lua/C procedure and so: it will be easier to understand and to verify it
separately from gh-4954.

I'll send a fix when this patchset will land to master to don't forget
to uncomment the case here.

----

During self-review of the changes found and fixed two typos:

diff --git a/test/box-tap/gh-4954-merger-via-c.test.lua b/test/box-tap/gh-4954-merger-via-c.test.lua
index 963b5825a..9fbb0919a 100755
--- a/test/box-tap/gh-4954-merger-via-c.test.lua
+++ b/test/box-tap/gh-4954-merger-via-c.test.lua
@@ -4,13 +4,13 @@
 -- gh-4954: The fiber-local Lua stack should be even after
 -- merge_source_next() call from C code.
 --
--- See test/box-tap/merge_source.c for more information.
+-- 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/merge_source.{so,dylib}.
+-- 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') .. ';' ..

And also commented the unused function to make luacheck happy (and
simplify luacheck integration patchset rebasing if it'll land into
master after this patchset):

diff --git a/test/box-tap/gh-4954-merger-via-c.test.lua b/test/box-tap/gh-4954-merger-via-c.test.lua
index 59ff731a5..9fbb0919a 100755
--- a/test/box-tap/gh-4954-merger-via-c.test.lua
+++ b/test/box-tap/gh-4954-merger-via-c.test.lua
@@ -42,12 +42,16 @@ local function bad_buffer()
     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