Re: [Tarantool-patches] [PATCH] box: remove context from stack

[Alexander Turenko via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

On Tue, Jun 09, 2020 at 01:35:18PM +0300, Maria Khaydich wrote:
> 
> I suppose no tests are needed since this one is pretty straightforward.
> ----------------------------------------------------------------------
>  
> Lua stack was broken because we forgot to clear the context
> in case of an error when return value of called function was
> not serializable.
>  
> Closes #4617
> ---
> Branch:
> https://github.com/tarantool/tarantool/compare/eljashm/gh-4617-broken-lua-stack  
> Issue:
> https://github.com/tarantool/tarantool/issues/4617  
>  
> @ChangeLog
> Remove context from lua stack in case of an error.

Here we describe changes for a user (in the way a user may understand
it). For complex bugs we can briefly describe a situation, when it
occurs.

It is now in the changelogs/ directory.

>  
>  src/box/lua/call.c | 1 +
>  1 file changed, 1 insertion(+)
> diff --git a/src/box/lua/call.c b/src/box/lua/call.c
> index 6588ec2fa..7ab49983d 100644
> --- a/src/box/lua/call.c
> +++ b/src/box/lua/call.c
> @@ -436,6 +436,7 @@ port_lua_do_dump(struct port *base, struct mpstream *stream,
>      int top = lua_gettop(L);
>      if (lua_cpcall(L, handler, &ctx) != 0) {
>          luaT_toerror(port->L);
> +        lua_pop(L, 1); 

There are several doubts and thoughts around it:

1. lua_cpcall() always removes arguments from the Lua stack (in case of
   an error too). How we can observe this ctx userdata on tarantool_L
   without yield in encode_lua_call()?
2. luaT_toerror() leans on luaT_tolstring(), so it duplicates a string
   with the error message. Should not we pop two elements in the case? I
   would just call lua_settop().
3. The clash is due to tarantool_L usage (see 1.10.1-48-g18d87adf3). Is
   there any real reason why we can't use port->L here?

CCed Igor and Sergey to think around together.

>          return -1;
>      }
>      lua_settop(L, top);
> -- 
> 2.24.0