Re: [Tarantool-patches] [PATCH] refactoring: wrap lua_newthread with luaT_cpcall

[Igor Munkin <imun@tarantool.org>]

Ilya,

Thanks for you patch. Please consider my comments below.

On 15.10.19, Ilya Kosarev wrote:
> Wrap throwing lua_newthread with luaT_cpcall to process arising
> error properly.
> 
> Closes #4556
> ---
> https://github.com/tarantool/tarantool/tree/gh-4556-wrap-lua-newthread
> https://github.com/tarantool/tarantool/issues/4556
> 
>  src/box/lua/call.c            | 18 +++++++++++++++---
>  src/lua/fiber.c               |  6 +++++-
>  src/lua/trigger.c             | 13 +++++--------
>  src/lua/utils.h               | 24 ++++++++++++++++++++++++
>  third_party/lua-yaml/lyaml.cc |  6 +++++-
>  5 files changed, 54 insertions(+), 13 deletions(-)
> 
> diff --git a/src/box/lua/call.c b/src/box/lua/call.c
> index 631003c84..ecc85f7c2 100644
> --- a/src/box/lua/call.c
> +++ b/src/box/lua/call.c
> @@ -527,7 +527,11 @@ static inline int
>  box_process_lua(lua_CFunction handler, struct execute_lua_ctx *ctx,
>  		struct port *ret)
>  {
> -	lua_State *L = lua_newthread(tarantool_L);

> +	struct lua_State *L = NULL;

I see the only way for L to be NULL is when luaT_cpcall fails, thereby
the second check seems to be excess. However, feel free to add an assert
for it here.

> +	if (luaT_cpcall(tarantool_L, lua_newthread_wrapper, &L) != 0 || L == NULL) {
> +		return -1;
> +	}
> +	luaL_pushthread(tarantool_L, L);
>  	int coro_ref = luaL_ref(tarantool_L, LUA_REGISTRYINDEX);
>  	port_lua_create(ret, L);
>  	((struct port_lua *) ret)->ref = coro_ref;
> @@ -654,7 +658,11 @@ func_persistent_lua_load(struct func_lua *func)
>  	 * an arbitrary user-defined code
>  	 * (e.g. body = 'fiber.yield()').
>  	 */
> -	struct lua_State *coro_L = lua_newthread(tarantool_L);

Think about replacing the following five lines with a function
implementing the semantics described below. Let's name it luaT_newthread
and it
* Takes a pointer to lua_State
* Tries to safely create a new lua_State via lua_cpcall (exactly the way
  you've already implemented)
* Returns NULL If lua_newthread call fails with the LUA_ERRMEM error
* Pushes the newly created coroutine onto the stack of the given
  lua_State and returns it to its caller.

Whether luaT_newthread call fails you can also either return -1 or call
luaT_error within the caller frame.

> +	struct lua_State *coro_L = NULL;
> +	if (luaT_cpcall(tarantool_L, lua_newthread_wrapper, &coro_L) != 0 || coro_L == NULL) {
> +		return -1;
> +	}
> +	luaL_pushthread(tarantool_L, coro_L);
>  	if (!func->base.def->is_sandboxed) {
>  		/*
>  		 * Keep the original env to apply to a non-sandboxed
> @@ -811,7 +819,11 @@ lbox_func_call(struct lua_State *L)
>  	 * before the function call to pass it into the
>  	 * pcall-sandboxed tarantool_L handler.
>  	 */

Minor: lua_State opaque type is declared within LJ header (i.e. lua.h).
If struct lua_State usage instead doesn't respect some Tarantool style
guides it's preferable and common to use just lua_State.

> -	lua_State *args_L = lua_newthread(tarantool_L);
> +	struct lua_State *args_L = NULL;
> +	if (luaT_cpcall(tarantool_L, lua_newthread_wrapper, &args_L) != 0 || args_L == NULL) {
> +		return luaT_error(L);
> +	}
> +	luaL_pushthread(tarantool_L, args_L);
>  	int coro_ref = luaL_ref(tarantool_L, LUA_REGISTRYINDEX);
>  	lua_xmove(L, args_L, lua_gettop(L) - 1);
>  	struct port args;
> diff --git a/src/lua/fiber.c b/src/lua/fiber.c
> index 336be60a2..f71ea2a72 100644
> --- a/src/lua/fiber.c
> +++ b/src/lua/fiber.c
> @@ -388,7 +388,11 @@ lua_fiber_run_f(MAYBE_UNUSED va_list ap)
>  static struct fiber *
>  fiber_create(struct lua_State *L)
>  {
> -	struct lua_State *child_L = lua_newthread(L);
> +	struct lua_State *child_L = NULL;
> +	if (luaT_cpcall(L, lua_newthread_wrapper, &child_L) != 0 || child_L == NULL) {
> +		luaT_error(L);
> +	}
> +	luaL_pushthread(L, child_L);
>  	int coro_ref = luaL_ref(L, LUA_REGISTRYINDEX);
>  
>  	struct fiber *f = fiber_new("lua", lua_fiber_run_f);
> diff --git a/src/lua/trigger.c b/src/lua/trigger.c
> index 4803e85c5..bf3fce234 100644
> --- a/src/lua/trigger.c
> +++ b/src/lua/trigger.c
> @@ -73,17 +73,14 @@ lbox_trigger_run(struct trigger *ptr, void *event)
>  	 * trigger yields, so when it's time to clean
>  	 * up the coro, we wouldn't know which stack position
>  	 * it is on.

Note: What is the main stopper to introduce proposed API in LuaJIT
within this changeset? It will reduce the usage of internal LJ methods
in Tarantool code base.

> -	 *
> -	 * XXX: lua_newthread() may throw if out of memory,
> -	 * this needs to be wrapped with lua_pcall() as well.
> -	 * Don't, since it's a stupid overhead on every trigger
> -	 * invocation, and in future we plan to hack into Lua
> -	 * C API to fix this.
>  	 */
> -	struct lua_State *L;
> +	struct lua_State *L = NULL;
>  	int coro_ref;
>  	if (fiber()->storage.lua.stack == NULL) {
> -		L = lua_newthread(tarantool_L);
> +		if (luaT_cpcall(tarantool_L, lua_newthread_wrapper, &L) != 0 || L == NULL) {
> +			diag_raise();
> +		}
> +		luaL_pushthread(tarantool_L, L);
>  		coro_ref = luaL_ref(tarantool_L, LUA_REGISTRYINDEX);
>  	} else {
>  		L = fiber()->storage.lua.stack;
> diff --git a/src/lua/utils.h b/src/lua/utils.h
> index f8c34545a..cf4d6d326 100644
> --- a/src/lua/utils.h
> +++ b/src/lua/utils.h
> @@ -417,6 +417,30 @@ luaL_checkfield(struct lua_State *L, struct luaL_serializer *cfg, int idx,
>  	luaL_convertfield(L, cfg, idx, field);
>  }
>  

I'm definitely against spoiling Lua standart namespaces (lua_* and
luaL_*) and suggest to stop it since this patch. Let's discuss which
prefix is the preferable one for internal auxillary functions. I guess
you can use luaT_* here as widely used within Tarantool sources.

> +/**
> + * @brief A wrapper for lua_newthread() to pass it into luaT_cpcall
> + * @param L is a Lua State
> + * @sa lua_newthread()
> + */
> +static inline int
> +lua_newthread_wrapper(lua_State *L)
> +{
> +	*(struct lua_State**)lua_touserdata(L, 1) = lua_newthread(L);
> +	return 0;
> +}
> +
> +/**
> + * @brief Push L1 thread onto the L stack
> + * @param L is a Lua State whose stack we are pushing onto
> + * @param L1 is a Lua State whose thread we are pushing
> + */
> +static inline void
> +luaL_pushthread(lua_State *L, lua_State *L1)
> +{
> +	setthreadV(L, L->top, L1);
> +	incr_top(L);
> +}
> +
>  void
>  luaL_register_type(struct lua_State *L, const char *type_name,
>  		   const struct luaL_Reg *methods);
> diff --git a/third_party/lua-yaml/lyaml.cc b/third_party/lua-yaml/lyaml.cc
> index 7485341fa..78b68e260 100644
> --- a/third_party/lua-yaml/lyaml.cc
> +++ b/third_party/lua-yaml/lyaml.cc
> @@ -789,7 +789,11 @@ lua_yaml_encode(lua_State *L, struct luaL_serializer *serializer,
>     dumper.cfg = serializer;
>     dumper.error = 0;
>     /* create thread to use for YAML buffer */
> -   dumper.outputL = lua_newthread(L);
> +   dumper.outputL = NULL;
> +   if (luaT_cpcall(L, lua_newthread_wrapper, &dumper.outputL) != 0 || dumper.outputL == NULL) {
> +      return luaL_error(L, OOM_ERRMSG);
> +   }
> +   luaL_pushthread(L, dumper.outputL);
>     luaL_buffinit(dumper.outputL, &dumper.yamlbuf);
>  
>     if (!yaml_emitter_initialize(&dumper.emitter))
> -- 
> 2.17.1
> 

-- 
Best regards,
IM