Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v2] refactoring: wrap lua_newthread using luaT_cpcall

[Sergey Ostanevich <sergos@tarantool.org>]

Hi!

Please, create a GH for follow-ups:

- Rename lua_ and luaL_ interfaces in tarantool code base (utils.h,
  etc..) so that we will not interfere with luajit code base

I also agree with Igor that there's no need in luaT_pushthread() since
it has only one use and provides no more functionality than 2 lines of
code. 

With above fixed this patch is LGTM.

Regards,
Sergos


On 17 Oct 15:57, Igor Munkin wrote:
> CCed Sergos for proceeding with the review.
> 
> On 17.10.19, Igor Munkin wrote:
> > Ilya,
> > 
> > Thanks, the patch LGTM, however please consider my minor comments.
> > 
> > On 17.10.19, Ilya Kosarev wrote:
> > > Wrap throwing lua_newthread in luaT_newthread using luaT_cpcall
> > > to process arising error properly.
> > > 
> > > Closes #4556
> > > ---
> > > https://github.com/tarantool/tarantool/tree/i.kosarev/gh-4556-wrap-lua-newthread
> > > https://github.com/tarantool/tarantool/issues/4556
> > > 
> > > Changes in v2:
> > > - introduced luaT_newthread instead of repeated slocs
> > > - renamed luaL_pushthread to luaT_pushthread due to namespaces agreement
> > > 
> > >  src/box/lua/call.c            | 12 +++++++---
> > >  src/lua/fiber.c               |  4 +++-
> > >  src/lua/trigger.c             | 12 ++++------
> > >  src/lua/utils.h               | 41 +++++++++++++++++++++++++++++++++++
> > >  third_party/lua-yaml/lyaml.cc |  5 ++++-
> > >  5 files changed, 61 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/src/box/lua/call.c b/src/box/lua/call.c
> > > index 631003c84..51cad286a 100644
> > > --- a/src/box/lua/call.c
> > > +++ b/src/box/lua/call.c
> > > @@ -527,7 +527,9 @@ static inline int
> > >  box_process_lua(lua_CFunction handler, struct execute_lua_ctx *ctx,
> > >  		struct port *ret)
> > >  {
> > > -	lua_State *L = lua_newthread(tarantool_L);
> > > +	lua_State *L = luaT_newthread(tarantool_L);
> > > +	if (L == NULL)
> > > +		return -1;
> > >  	int coro_ref = luaL_ref(tarantool_L, LUA_REGISTRYINDEX);
> > >  	port_lua_create(ret, L);
> > >  	((struct port_lua *) ret)->ref = coro_ref;
> > > @@ -654,7 +656,9 @@ 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);
> > > +	lua_State *coro_L = luaT_newthread(tarantool_L);
> > > +	if (coro_L == NULL)
> > > +		return -1;
> > >  	if (!func->base.def->is_sandboxed) {
> > >  		/*
> > >  		 * Keep the original env to apply to a non-sandboxed
> > > @@ -811,7 +815,9 @@ lbox_func_call(struct lua_State *L)
> > >  	 * before the function call to pass it into the
> > >  	 * pcall-sandboxed tarantool_L handler.
> > >  	 */
> > > -	lua_State *args_L = lua_newthread(tarantool_L);
> > > +	lua_State *args_L = luaT_newthread(tarantool_L);
> > > +	if (args_L == NULL)
> > > +		return luaT_error(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..124908a05 100644
> > > --- a/src/lua/fiber.c
> > > +++ b/src/lua/fiber.c
> > > @@ -388,7 +388,9 @@ 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);
> > > +	lua_State *child_L = luaT_newthread(L);
> > > +	if (child_L == NULL)
> > > +		luaT_error(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..6df048a8d 100644
> > > --- a/src/lua/trigger.c
> > > +++ b/src/lua/trigger.c
> > > @@ -73,17 +73,13 @@ 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.
> > > -	 *
> > > -	 * 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;
> > > +	lua_State *L;
> > >  	int coro_ref;
> > >  	if (fiber()->storage.lua.stack == NULL) {
> > > -		L = lua_newthread(tarantool_L);
> > > +		L = luaT_newthread(tarantool_L);
> > > +		if (L == NULL)
> > > +			diag_raise();
> > >  		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..f82b30fbc 100644
> > > --- a/src/lua/utils.h
> > > +++ b/src/lua/utils.h
> > > @@ -591,6 +591,47 @@ luaL_checkfinite(struct lua_State *L, struct luaL_serializer *cfg,
> > >  		luaL_error(L, "number must not be NaN or Inf");
> > >  }
> > >  
> > 
> > Note: the comment about lua_* namespace also relates to the wrapper
> > below. I guess we need some follow-up activity for it.
> > 
> > > +/**
> > > + * @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)
> > > +{
> > > +	*(lua_State **)lua_touserdata(L, 1) = lua_newthread(L);
> > > +	return 0;
> > > +}
> > > +
> > 
> > Minor: I see no reason to create a separate function for pushing one
> > lua_State onto the anothers' stack. Feel free to ignore or to discuss it
> > with the next reviewer.
> > 
> > > +/**
> > > + * @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
> > > +luaT_pushthread(lua_State *L, lua_State *L1)
> > > +{
> > > +	setthreadV(L, L->top, L1);
> > > +	incr_top(L);
> > > +}
> > > +
> > > +/**
> > > + * @brief Safe wrapper for lua_newthread()
> > > + * @param L is a Lua State
> > > + * @sa lua_newthread()
> > > + */
> > > +static inline lua_State *
> > > +luaT_newthread(lua_State *L)
> > > +{
> > > +	lua_State *L1 = NULL;
> > > +	if (luaT_cpcall(L, lua_newthread_wrapper, &L1) != 0) {
> > > +		return NULL;
> > > +	}
> > > +	assert(L1 != NULL);
> > > +	luaT_pushthread(L, L1);
> > > +	return L1;
> > > +}
> > > +
> > >  /**
> > >   * Check if a value on @a L stack by index @a idx is an ibuf
> > >   * object. Both 'struct ibuf' and 'struct ibuf *' are accepted.
> > > diff --git a/third_party/lua-yaml/lyaml.cc b/third_party/lua-yaml/lyaml.cc
> > > index 7485341fa..af4f2f5d5 100644
> > > --- a/third_party/lua-yaml/lyaml.cc
> > > +++ b/third_party/lua-yaml/lyaml.cc
> > > @@ -789,7 +789,10 @@ 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 = luaT_newthread(L);
> > > +   if (dumper.outputL == NULL) {
> > > +      return luaL_error(L, OOM_ERRMSG);
> > > +   }
> > >     luaL_buffinit(dumper.outputL, &dumper.yamlbuf);
> > >  
> > >     if (!yaml_emitter_initialize(&dumper.emitter))
> > > -- 
> > > 2.17.1
> > > 
> > 
> > -- 
> > Best regards,
> > IM
> 
> -- 
> Best regards,
> IM
>