[Tarantool-patches] [PATCH v2 2/3] merger: clean fiber-local Lua stack after next()
Alexander Turenko
alexander.turenko at tarantool.org
Fri Jul 17 06:08:51 MSK 2020
> > > > @@ -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
More information about the Tarantool-patches
mailing list