[Tarantool-patches] [PATCH v3] refactoring: wrap lua_newthread using luaT_cpcall

[Tarantool-patches] [PATCH v3] refactoring: wrap lua_newthread using luaT_cpcall

[Ilya Kosarev]

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

Changes in v3:
- remove extra luaT_pushthread function
- renamed lua_newthread_wrapper to luaT_newthread_wrapper due to namespaces agreement

 src/box/lua/call.c            | 12 +++++++++---
 src/lua/fiber.c               |  4 +++-
 src/lua/trigger.c             | 12 ++++--------
 src/lua/utils.h               | 30 ++++++++++++++++++++++++++++++
 third_party/lua-yaml/lyaml.cc |  5 ++++-
 5 files changed, 50 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..d9fb0704f 100644
--- a/src/lua/utils.h
+++ b/src/lua/utils.h
@@ -591,6 +591,36 @@ luaL_checkfinite(struct lua_State *L, struct luaL_serializer *cfg,
 		luaL_error(L, "number must not be NaN or Inf");
 }
 
+/**
+ * @brief A wrapper for lua_newthread() to pass it into luaT_cpcall
+ * @param L is a Lua State
+ * @sa lua_newthread()
+ */
+static inline int
+luaT_newthread_wrapper(lua_State *L)
+{
+	*(lua_State **)lua_touserdata(L, 1) = lua_newthread(L);
+	return 0;
+}
+
+/**
+ * @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, luaT_newthread_wrapper, &L1) != 0) {
+		return NULL;
+	}
+	assert(L1 != NULL);
+	setthreadV(L, L->top, L1);
+	incr_top(L);
+	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

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

[Igor Munkin]

Ilya,

Thanks, LGTM.

On 18.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
> 
> Changes in v3:
> - remove extra luaT_pushthread function
> - renamed lua_newthread_wrapper to luaT_newthread_wrapper due to namespaces agreement
> 
>  src/box/lua/call.c            | 12 +++++++++---
>  src/lua/fiber.c               |  4 +++-
>  src/lua/trigger.c             | 12 ++++--------
>  src/lua/utils.h               | 30 ++++++++++++++++++++++++++++++
>  third_party/lua-yaml/lyaml.cc |  5 ++++-
>  5 files changed, 50 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..d9fb0704f 100644
> --- a/src/lua/utils.h
> +++ b/src/lua/utils.h
> @@ -591,6 +591,36 @@ luaL_checkfinite(struct lua_State *L, struct luaL_serializer *cfg,
>  		luaL_error(L, "number must not be NaN or Inf");
>  }
>  
> +/**
> + * @brief A wrapper for lua_newthread() to pass it into luaT_cpcall
> + * @param L is a Lua State
> + * @sa lua_newthread()
> + */
> +static inline int
> +luaT_newthread_wrapper(lua_State *L)
> +{
> +	*(lua_State **)lua_touserdata(L, 1) = lua_newthread(L);
> +	return 0;
> +}
> +
> +/**
> + * @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, luaT_newthread_wrapper, &L1) != 0) {
> +		return NULL;
> +	}
> +	assert(L1 != NULL);
> +	setthreadV(L, L->top, L1);
> +	incr_top(L);
> +	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

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

[Sergey Ostanevich]

Hi!

LGTM, thank you!

Sergos


On 18 Oct 12:29, 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
> 
> Changes in v3:
> - remove extra luaT_pushthread function
> - renamed lua_newthread_wrapper to luaT_newthread_wrapper due to namespaces agreement
> 
>  src/box/lua/call.c            | 12 +++++++++---
>  src/lua/fiber.c               |  4 +++-
>  src/lua/trigger.c             | 12 ++++--------
>  src/lua/utils.h               | 30 ++++++++++++++++++++++++++++++
>  third_party/lua-yaml/lyaml.cc |  5 ++++-
>  5 files changed, 50 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..d9fb0704f 100644
> --- a/src/lua/utils.h
> +++ b/src/lua/utils.h
> @@ -591,6 +591,36 @@ luaL_checkfinite(struct lua_State *L, struct luaL_serializer *cfg,
>  		luaL_error(L, "number must not be NaN or Inf");
>  }
>  
> +/**
> + * @brief A wrapper for lua_newthread() to pass it into luaT_cpcall
> + * @param L is a Lua State
> + * @sa lua_newthread()
> + */
> +static inline int
> +luaT_newthread_wrapper(lua_State *L)
> +{
> +	*(lua_State **)lua_touserdata(L, 1) = lua_newthread(L);
> +	return 0;
> +}
> +
> +/**
> + * @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, luaT_newthread_wrapper, &L1) != 0) {
> +		return NULL;
> +	}
> +	assert(L1 != NULL);
> +	setthreadV(L, L->top, L1);
> +	incr_top(L);
> +	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
> 
> 

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

[Kirill Yukhin]

Hello,

On 18 окт 12:29, Ilya Kosarev wrote:
> Wrap throwing lua_newthread in luaT_newthread using luaT_cpcall
> to process arising error properly.
> 
> Closes #4556

I've checked your patch into 1.10, 2.1, 2.2 and master.

--
Regards, Kirill Yukhin