[Tarantool-patches] [PATCH v2 1/1] box: export box_session_push to the public C API

[Igor Munkin]

Vlad,

Thanks for the fixes! I dumped the main points we discussed and left one
comment below. Otherwise LGTM.

On 08.04.20, Vladislav Shpilevoy wrote:
> Hi! Thanks for the review!

<snipped>

> 
> > Furthermore, it doesn't respect the lua_CFunction
> > signature and lua_* prefix is definitely a misguiding one here. Let's
> > try to use another prefix here.
> 
> This is because it is not a lua_CFunction. It is not called
> anywhere via lua_call/pcall/cpcall. But I got the point.

Yes, I was wrong here.

> 

<snipped>

> > 
> > Moved our discussion from the previous thread here:
> > 
> > ================================================================================
> > 
> >> I was afraid that in case of an OOM we will leave garbage in
> >> tarantool_L.
> >> But if you say we can use tarantool_L safely, then ok, I am not against
> >> that. It is just one another reason to start panic when the heap of Lua
> >> are out of memory, because otherwise data on tarantool_L will leak.
> > 
> > When OOM occurs both host and guest stacks are unwinded to the closest
> > protected frame (i.e. the one created via <lua_pcall> below).
> 
> It will be unwinded, but only in case we got to the call. What if one
> of lua_pushlightuserdata() would throw an OOM? Nothing will be removed,
> because this function (port_dump_plain) does not work on tarantool_L. It
> may be a pure C stack not intersecting with Lua anyhow. We use tarantool_L
> as some kind of external buffer and a last resort to reach Lua objects from
> pure C context.
> 
> So if a throw happens before lua_pcall(), we will leave something on
> tarantool_L. Right?

OK, now I see. I dumped the questions we discussed:
* How unwinder behaves when LUA_ERRMEM occurs within lua_*/luaL_*
  call (how the personality routine is chosen, how the guest and host
  stacks are unwound)?
* Is it safe to use main coroutine as an auxillary lua_State?
* Does OOM error be pushed on the guest stack when the error occurs?

I hope I miss nothing significant.

As we agreed it's my AR to precisely check the cases above. For now I
guess we can live with tarantool_L here, since the code is much clearer
and with less memory usage. Furthermore if one of the given cases shows
that such main coroutine usage is an unsafe on then we ought to
reimplement *all* such places where tarantool_L is misused.

> 
> > Guest stack base is restored to the corresponding slot, slots above the top
> > one won't be marked and GC objects will be collected sometime later. If
> > there are no unmanaged memory allocations underneath then no leaks
> > occur.
> > 
> >>
> >> On the other hand, the new thread will also leak, so probably this is
> >> a lose-lose situation, but with tarantool_L it is a little simpler.
> > 
> > I might be missing something, but I see no memory leak since you create
> > a protected frame via <lua_pcall>.
> 
> It may not manage to get to it and fail during pushing the argument.
> 
> > ================================================================================
> > 
> >> +	char *result = NULL;
> >> +	int top = lua_gettop(L);
> >> +	(void) top;
> >> +	lua_pushlightuserdata(L, &result);
> >> +	lua_pushlightuserdata(L, base);
> >> +	lua_pushlightuserdata(L, size);
> >> +	lua_pushcclosure(L, lua_port_msgpack_dump_plain, 3);
> >> +	if (lua_pcall(L, 0, 0, 0) != 0) {
> > 
> > If <lua_port_msgpack_dump_plain> returns nothing via guest stack, you
> > can use more convenient <lua_cpcall>[1] here. It requires almost no
> > auxillary actions: just fill a structure to be passed via the void
> > pointer argument to the function.
> 
> Now we are talking :) Seems like this thing will protect from any
> leaks 99%, right? (Except in case of a fail it may be not able to
> push error object on the stack, is it possible?).
> 
> ====================

<snipped>

> diff --git a/src/box/lua/console.c b/src/box/lua/console.c
> index 886120eac..ff2db5832 100644
> --- a/src/box/lua/console.c
> +++ b/src/box/lua/console.c

<snipped>

> @@ -461,11 +465,10 @@ port_msgpack_dump_plain_via_lua(struct lua_State *L)
>  	 */
>  	luamp_decode(L, luaL_msgpack_default, &data);
>  	data = console_dump_plain(L, size);
> -	if (data == NULL) {
> -		*result = NULL;
> -		return 0;
> -	}
> -	*result = port_mspack_set_plain((struct port *)port, data, *size);
> +	if (data == NULL)
> +		assert(port->plain == NULL);
> +	else
> +		port_mspack_set_plain((struct port *)port, data, *size);

Typo: s/port_mspack_set_plain/port_msgpack_set_plain/g.

The result value is not checked, but <port_msgpack_set_plain> returns -1
when OOM occurs.

>  	return 0;
>   }
>  

<snipped>

-- 
Best regards,
IM