Re: [Tarantool-patches] [PATCH v2 06/15] WIP: module api/lua: add luaT_tuple_encode()

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

Thanks for the patch!

See 4 comments below.

On 11.10.2020 14:57, Alexander Turenko wrote:
> It is the same as luaT_tuple_new(), but returns raw MsgPack data (not
> <box_tuple_t>) allocated on the box region.
> 
> The reason to expose this function is to provide ability to use
> box_tuple_compare_with_key() function from an external module for a key
> passed as a Lua table. The compare function has the following signature:
> 
>  | API_EXPORT int
>  | box_tuple_compare_with_key(box_tuple_t *tuple_a, const char *key_b,
>  |                            box_key_def_t *key_def);
> 
> The second parameter is a key encoded as an MsgPack array, not a tuple
> structure. So luaT_tuple_new() is not applicable here (it is not
> worthful to create a tuple structure if we need just MsgPack data).
> 
> Some complexity was introduced to support encoding on the Lua shared
> buffer and the box region both. The motivation is the following:
> 
> - luaT_tuple_encode() is exposed with encoding to the box region,
>   because it is more usual to the module API. In particular a user of
>   the API able to control when the tuple data should be released.
> - Encoding to the Lua shared buffer is kept internally, because there is
>   no strong reason to change it to the box region for box.tuple.new().
> 
> Part of #5273
> 
> XXX: Try to get rid of luaT_mpstream_init_*() functions: call
> mpstream_init() outside of the protected section with changed error
> handler.
> ---
>  src/box/lua/tuple.c              |  99 ++++++++++++++++++++++----
>  src/box/lua/tuple.h              |  17 +++++
>  src/exports.h                    |   1 +
>  test/app-tap/module_api.c        | 115 +++++++++++++++++++++++++++++++
>  test/app-tap/module_api.test.lua |   2 +-
>  5 files changed, 220 insertions(+), 14 deletions(-)
> 
> diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
> index 18cfef979..8e2255a2b 100644
> --- a/src/box/lua/tuple.c
> +++ b/src/box/lua/tuple.c
> @@ -100,10 +100,31 @@ luaT_istuple(struct lua_State *L, int narg)
>  	return *(struct tuple **) data;
>  }
>  
> +/* {{{ Encode a Lua table as an MsgPack array */
> +
> +/*
> + * A lot of functions are there, however the task per se looks
> + * simple. Reasons are the following.
> + *
> + * 1. box.tuple.new() supports two parameters conventions.
> + *    <luaT_tuple_encode_values>() implements the old API.
> + * 2. Serializer from Lua to MsgPack may raise a Lua error,
> + *    so it should be run under pcall. The dangerous code is
> + *    encapsulated into <luaT_tuple_encode_table>().
> + * 3. In particular <mpstream_init>() may raise an error in case
> + *    of OOM, so it also is run under pcall.
> + * 4. box.tuple.new() and <luaT_tuple_new>() use shared Lua ibuf
> + *    under the hood (because there is no strong reason to change
> + *    it), while <luaT_tuple_encode>() uses the box region
> + *    (because it is usual for the module API).
> + */
> +
>  /**
>   * Encode a Lua values on a Lua stack as an MsgPack array.
>   *
>   * Raise a Lua error when encoding fails.
> + *
> + * Helper for <lbox_tuple_new>().

1. Should be a part of the previous patch?

>   */
>  static int
>  luaT_tuple_encode_values(struct lua_State *L)
> @@ -122,6 +143,22 @@ luaT_tuple_encode_values(struct lua_State *L)
>  	return 0;
>  }
>  
> +typedef void luaT_mpstream_init_f(struct mpstream *stream, struct lua_State *L);
> +
> +static void
> +luaT_mpstream_init_lua_ibuf(struct mpstream *stream, struct lua_State *L)
> +{
> +	mpstream_init(stream, tarantool_lua_ibuf, ibuf_reserve_cb,
> +		      ibuf_alloc_cb, luamp_error, L);
> +}
> +
> +static void
> +luaT_mpstream_init_box_region(struct mpstream *stream, struct lua_State *L)
> +{
> +	mpstream_init(stream, &fiber()->gc, region_reserve_cb, region_alloc_cb,
> +		      luamp_error, L);

2. If you throw after some allocations are done, will the region leak? I don't
see a truncate in case luaT_tuple_encode_helper fails.

> +}
> +
>  /**
>   * Encode a Lua table or a tuple as MsgPack.
>   *
> @@ -132,25 +169,26 @@ luaT_tuple_encode_values(struct lua_State *L)
>  static int
>  luaT_tuple_encode_table(struct lua_State *L)
>  {
> -	struct ibuf *buf = tarantool_lua_ibuf;
> -	ibuf_reset(buf);
>  	struct mpstream stream;
> -	mpstream_init(&stream, buf, ibuf_reserve_cb, ibuf_alloc_cb, luamp_error,
> -		      L);
> -	luamp_encode_tuple(L, &tuple_serializer, &stream, 1);
> +	luaT_mpstream_init_f *luaT_mpstream_init_f = lua_topointer(L, 1);
> +	luaT_mpstream_init_f(&stream, L);
> +	luamp_encode_tuple(L, &tuple_serializer, &stream, 2);
>  	mpstream_flush(&stream);
>  	return 0;
>  }
>  
> -static char *
> -luaT_tuple_encode_on_lua_ibuf(struct lua_State *L, int idx,
> -			      size_t *tuple_len_ptr)
> +/**
> + * Encode a Lua table / tuple to given mpstream.
> + */
> +static int
> +luaT_tuple_encode_helper(struct lua_State *L, int idx,
> +			 luaT_mpstream_init_f *luaT_mpstream_init_f)

3. Function name is usually a verb. Here I can propose
luaT_tuple_encode_on_mpstream.

>  {
>  	assert(idx != 0);
>  	if (!lua_istable(L, idx) && !luaT_istuple(L, idx)) {
>  		diag_set(IllegalParams, "A tuple or a table expected, got %s",
>  			 lua_typename(L, lua_type(L, idx)));
> -		return NULL;
> +		return -1;
>  	}
>  
>  	int top = lua_gettop(L);
> @@ -163,18 +201,53 @@ luaT_tuple_encode_on_lua_ibuf(struct lua_State *L, int idx,
>  	lua_rawgeti(L, LUA_REGISTRYINDEX, luaT_tuple_encode_table_ref);
>  	assert(lua_isfunction(L, -1));
>  
> +	lua_pushlightuserdata(L, luaT_mpstream_init_f);
>  	lua_pushvalue(L, idx);
>  
> -	int rc = luaT_call(L, 1, 0);
> +	int rc = luaT_call(L, 2, 0);
>  	lua_settop(L, top);
> -	if (rc != 0)
> +	return rc == 0 ? 0 : -1;

4. Why not simply 'return rc;'?