[tarantool-patches] Re: [PATCH v7 1/6] lua: remove exceptions from function luaL_tofield()

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the patch! See 4 comments below.

On 15/01/2019 19:10, imeevma@tarantool.org wrote:
> Before this commit, the luaL_tofield() function threw a Lua
> exception when an error occurred. This behavior has been changed
> in this commit, now it sets diag_set() and returns -1 in case of
> an error.
> 
> Needed for #3505
> ---
>   src/box/lua/call.c  |   9 +++--
>   src/box/lua/tuple.c |   3 +-
>   src/lua/msgpack.c   |  12 ++++--
>   src/lua/utils.c     | 105 ++++++++++++++++++++++++++++++----------------------
>   src/lua/utils.h     |   8 +++-
>   5 files changed, 83 insertions(+), 54 deletions(-)
> 
> diff --git a/src/box/lua/call.c b/src/box/lua/call.c
> index 52939ae..39df05d 100644
> --- a/src/box/lua/call.c
> +++ b/src/box/lua/call.c
> @@ -164,7 +164,8 @@ luamp_encode_call_16(lua_State *L, struct luaL_serializer *cfg,
>   		 */
>   		for (int i = 1; i <= nrets; ++i) {
>   			struct luaL_field field;
> -			luaL_tofield(L, cfg, i, &field);
> +			if (luaL_tofield(L, cfg, i, &field) < 0)
> +				luaT_error(L);

1. I know, it is weird, but usually we write return luaT_error/luaL_error().
Difference is in 'return' usage. Do not ask me why, I do not know. Maybe to
emphasize that this statement finishes the function. So please, write 'return'
where possible. Here and in other places.

>   			struct tuple *tuple;
>   			if (field.type == MP_EXT &&
>   			    (tuple = luaT_istuple(L, i)) != NULL) {
> diff --git a/src/lua/utils.c b/src/lua/utils.c
> index 978fe61..9a9fd3a 100644
> --- a/src/lua/utils.c
> +++ b/src/lua/utils.c
> @@ -37,6 +37,8 @@
>   #include <diag.h>
>   #include <fiber.h>
>   

2. Unnecessary empty line.

> +#include "box/error.h"
> +
>   int luaL_nil_ref = LUA_REFNIL;
>   int luaL_map_metatable_ref = LUA_REFNIL;
>   int luaL_array_metatable_ref = LUA_REFNIL;
> @@ -408,10 +411,11 @@ lua_field_inspect_table(struct lua_State *L, struct luaL_serializer *cfg,
>   		lua_call(L, 1, 1);
>   		/* replace obj with the unpacked value */
>   		lua_replace(L, idx);

3. In my opinion there is still a problem with lua_* methods,
throwing exceptions on OOM *and on a simple stack overflow*.
Even fixed GC will not fix the problem if all memory is
occupied by non-garbage memory. I adhere to my opinion that it
is better to use lua_cpcall. There is nothing bad with it. *_pcall
just remembers a couple of registers, it is not too expensive for
such a vast function. Also lua_cpcall causes much less diff.

We can do it, for example, by renaming luaL_tofield to luaL_tofield_xc
and introducing new luaL_tofield, calling luaL_tofield_xc via
lua_cpcall. Just like we used to work with C++ *_xc wrappers.

Please, ask again in the server team chat about the proposal above.
Do not forget about stack overflow error, which also is possible and
does not mean panic. Moreover, it is worth noting that diff is going
to be much less and simpler. If they decline the proposal, I give in.

> -		luaL_tofield(L, cfg, idx, field);
> -		return;
> +		return luaL_tofield(L, cfg, idx, field);
>   	} else if (!lua_isstring(L, -1)) {
> -		luaL_error(L, "invalid " LUAL_SERIALIZE  " value");
> +		diag_set(ClientError, ER_PROC_LUA,
> +			 "invalid " LUAL_SERIALIZE " value");
> +		return -1;
>   	}
>   
>   	type = lua_tostring(L, -1);
> @@ -525,94 +539,96 @@ luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg, int index,
>   			field->dval = num;
>   			CHECK_NUMBER(num);
>   		}
> -		return;
> +		break;

4. Why no to return 0? Anyway all your breaks lead to a simple 'return 0'.

> @@ -620,14 +636,15 @@ luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg, int index,
>   		field->sval.len = 0;
>   		if (lua_touserdata(L, index) == NULL) {
>   			field->type = MP_NIL;
> -			return;
> +			break;
>   		}
>   		/* Fall through */
>   	default:
>   		field->type = MP_EXT;
> -		return;
> +		break;
>   	}
>   #undef CHECK_NUMBER
> +	return 0;
>   }
>   
>   void