From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id C9B9743D67A for ; Wed, 16 Oct 2019 00:16:51 +0300 (MSK) Date: Wed, 16 Oct 2019 00:16:03 +0300 From: Igor Munkin Message-ID: <20191015211601.GB13020@tarantool.org> References: <20191015155045.7002-1-i.kosarev@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20191015155045.7002-1-i.kosarev@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH] refactoring: wrap lua_newthread with luaT_cpcall List-Id: Tarantool development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ilya Kosarev Cc: tarantool-patches@freelists.org, tarantool-patches@dev.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