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

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

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