Re: [Tarantool-patches] [PATCH v3] net.box: allow to store user-defined fields in future object

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Vova,

Thanks for the fixes! LGTM, with some nits below.

On 13.08.21, Vladimir Davydov wrote:
> Before commit 954194a1ca5c ("net.box: rewrite request implementation in

Typo: AFAICS full hash has to be used to mention particular commits. I
failed to find this rule in the dev guide[1], but I've found that Cyrill
has recently added the example to Code review procedure in our Wiki[2]
(however, the command doesn't yield the full hash).

> C"), net.box future was a plain Lua table so that the caller could
> attach extra information to it. Now it isn't true anymore - a future is
> a userdata object, and it doesn't have indexing methods.
> 
> For backward compatibility, let's add __index and __newindex fields and
> store user-defined fields in a Lua table, which is created lazily on the
> first __newindex invocation. __index falls back on the metatable methods
> if a field isn't found in the table.
> 
> Follow-up #6241
> Closes #6306
> ---
> https://github.com/tarantool/tarantool/issues/6306
> https://github.com/tarantool/tarantool/tree/vdavydov/netbox-allow-to-store-user-data-in-future-using-lua-table
> 
> Changes in v3:
>  - Use Lua table instead of mhash for storing user data, as suggested
>    by imun@.
> 
> v2: https://lists.tarantool.org/pipermail/tarantool-patches/2021-August/025402.html
> 
>  src/box/lua/net_box.c                         |  51 ++++++++
>  test/box/net.box_fiber-async_gh-3107.result   | 109 +++++++++++++++++-
>  test/box/net.box_fiber-async_gh-3107.test.lua |  39 ++++++-
>  3 files changed, 191 insertions(+), 8 deletions(-)
> 
> diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
> index 229dec590cf9..078bbfb07b18 100644
> --- a/src/box/lua/net_box.c
> +++ b/src/box/lua/net_box.c

<snipped>

> @@ -1420,6 +1430,44 @@ luaT_netbox_request_gc(struct lua_State *L)
>  	return 0;
>  }
>  
> +static int
> +luaT_netbox_request_index(struct lua_State *L)
> +{
> +	struct netbox_request *request = luaT_check_netbox_request(L, 1);
> +	if (request->index_ref != LUA_NOREF) {
> +		lua_rawgeti(L, LUA_REGISTRYINDEX, request->index_ref);
> +		/* Push the key (2nd argument) to the top. */

Minor: It's worth to mention, why you don't use <lua_insert> here
instead (like you do in __newindex). The reason is to save the given key
for the fall back path. You can also add an assert, that coroutine stack
size at the end of this <if> block is the same as at its beginning.

> +		lua_pushvalue(L, 2);
> +		lua_rawget(L, -2);
> +		if (lua_type(L, -1) != LUA_TNIL)
> +			return 1;
> +		/* Pop nil and the index table. */
> +		lua_pop(L, 2);
> +	}
> +	/* Fall back on metatable methods. */
> +	lua_getmetatable(L, 1);
> +	/* Move the metatable before the key (2nd argument). */
> +	lua_insert(L, 2);
> +	lua_rawget(L, 2);
> +	return 1;
> +}
> +
> +static int
> +luaT_netbox_request_newindex(struct lua_State *L)
> +{
> +	struct netbox_request *request = luaT_check_netbox_request(L, 1);
> +	if (request->index_ref == LUA_NOREF) {

Minor: Looks like this condition is met only once. Will this place
perform better with UNLIKELY attribute?

> +		/* Lazily create the index table on the first invocation. */
> +		lua_newtable(L);
> +		request->index_ref = luaL_ref(L, LUA_REGISTRYINDEX);
> +	}
> +	lua_rawgeti(L, LUA_REGISTRYINDEX, request->index_ref);
> +	/* Move the index table before the key (2nd argument). */
> +	lua_insert(L, 2);
> +	lua_rawset(L, 2);
> +	return 0;
> +}
> +
>  /**
>   * Returns true if the response was received for the given request.
>   */

<snipped>

> diff --git a/test/box/net.box_fiber-async_gh-3107.result b/test/box/net.box_fiber-async_gh-3107.result
> index aaaca351a579..60033059335b 100644
> --- a/test/box/net.box_fiber-async_gh-3107.result
> +++ b/test/box/net.box_fiber-async_gh-3107.result
> @@ -10,10 +10,7 @@ net = require('net.box')
>  cond = nil
>  ---
>  ...
> -box.schema.func.create('long_function')
> ----
> -...
> -box.schema.user.grant('guest', 'execute', 'function', 'long_function')
> +box.schema.user.grant('guest', 'execute', 'universe')
>  ---
>  ...
>  function long_function(...) cond = fiber.cond() cond:wait() return ... end
> @@ -104,7 +101,109 @@ err:find('Usage') ~= nil
>  ---
>  - true
>  ...
> -box.schema.func.drop('long_function')
> +-- Storing user data in future object.
> +future = c:eval('return 123', {}, {is_async = true})
> +---
> +...
> +future.abc -- nil
> +---
> +- null
> +...
> +future.abc = nil
> +---
> +...
> +future.abc -- nil
> +---
> +- null
> +...
> +future.abc = 'abc'
> +---
> +...
> +future.abc -- abc
> +---
> +- abc
> +...
> +future.abc = nil
> +---
> +...
> +future.abc -- nil
> +---
> +- null
> +...
> +future.abc = nil

Looks like a typo: future.abc already equals to nil above.

> +---
> +...
> +future.abc -- nil
> +---
> +- null
> +...
> +future:wait_result() -- 123
> +---
> +- [123]
> +...
> +-- Garbage collection of stored user data.
> +future = c:eval('return 123', {}, {is_async = true})
> +---
> +...
> +future.data1 = {1}
> +---
> +...
> +future.data2 = {2}
> +---
> +...
> +future.data3 = {3}
> +---
> +...
> +gc = setmetatable({}, {__mode = 'v'})
> +---
> +...
> +gc.data1 = future.data1
> +---
> +...
> +gc.data2 = future.data2
> +---
> +...
> +gc.data3 = future.data3
> +---
> +...
> +future.data1 = nil
> +---
> +...
> +_ = collectgarbage('collect')
> +---
> +...
> +gc.data1 == nil
> +---
> +- true
> +...
> +future.data2 = 123
> +---
> +...
> +_ = collectgarbage('collect')
> +---
> +...
> +gc.data2 == nil
> +---
> +- true
> +...
> +future:wait_result() -- 123
> +---
> +- [123]
> +...
> +future = nil
> +---
> +...
> +_ = collectgarbage('collect')
> +---
> +...
> +_ = collectgarbage('collect')
> +---
> +...
> +gc.data3 == nil
> +---
> +- true
> +...
> +box.schema.user.revoke('guest', 'execute', 'universe')
>  ---
>  ...
>  c:close()

<snipped>

> -- 
> 2.25.1
> 

[1]: https://www.tarantool.io/en/doc/latest/dev_guide/developer_guidelines/
[2]: https://github.com/tarantool/tarantool/wiki/Code-review-procedure#commit-message

-- 
Best regards,
IM