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

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

[Vladimir Davydov via Tarantool-patches]

Before commit 954194a1ca5c ("net.box: rewrite request implementation in
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 an mhash, which is created lazily on the
first __newindex invocation. __index falls back on the metatable methods
if a field isn't found in the mhash.

Follow-up #6241
Closes #6306
---
https://github.com/tarantool/tarantool/tree/vdavydov/netbox-allow-to-store-user-data-in-future
https://github.com/tarantool/tarantool/issues/6241
https://github.com/tarantool/tarantool/issues/6306

As a result of this patch, async net.box.call performance degrades by
about 2-3% because of an extra C function call executed per each future
member access:

        | KRPS (PROC TIME) | KRPS (WALL TIME)
 before |     276 +- 9     |    448 +- 22
 after  |     270 +- 8     |    434 +- 21

Still, it's better than wrapping a future object into a table, which
degrades performance by 10%.

 src/box/lua/net_box.c                         |  91 ++++++++++++++++
 test/box/net.box_fiber-async_gh-3107.result   | 103 +++++++++++++++++-
 test/box/net.box_fiber-async_gh-3107.test.lua |  37 ++++++-
 3 files changed, 223 insertions(+), 8 deletions(-)

diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index 06e574cdf746..162ff6c82fb0 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -134,6 +134,14 @@ struct netbox_request {
 	 * the response hasn't been received yet.
 	 */
 	struct error *error;
+	/**
+	 * User-defined fields: field name -> Lua object reference.
+	 * We allow the user to attach extra information to a future object,
+	 * e.g. a reference to a connection or the invoked method name/args.
+	 * All the information is stored in this table, which is created
+	 * lazily, on the first __newindex invocation.
+	 */
+	struct mh_strnptr_t *index;
 };
 
 static const char netbox_registry_typename[] = "net.box.registry";
@@ -166,6 +174,15 @@ netbox_request_destroy(struct netbox_request *request)
 	luaL_unref(tarantool_L, LUA_REGISTRYINDEX, request->result_ref);
 	if (request->error != NULL)
 		error_unref(request->error);
+	if (request->index != NULL) {
+		struct mh_strnptr_t *h = request->index;
+		mh_int_t k;
+		mh_foreach(h, k) {
+			int ref = (intptr_t)mh_strnptr_node(h, k)->val;
+			luaL_unref(tarantool_L, LUA_REGISTRYINDEX, ref);
+		}
+		mh_strnptr_delete(h);
+	}
 }
 
 /**
@@ -1358,6 +1375,77 @@ 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);
+	size_t field_name_len;
+	const char *field_name = lua_tolstring(L, 2, &field_name_len);
+	if (field_name == NULL)
+		return luaL_error(L, "invalid index");
+	if (request->index != NULL) {
+		mh_int_t k = mh_strnptr_find_inp(request->index, field_name,
+						 field_name_len);
+		if (k != mh_end(request->index)) {
+			int field_value_ref = (intptr_t)mh_strnptr_node(
+				request->index, k)->val;
+			lua_rawgeti(L, LUA_REGISTRYINDEX, field_value_ref);
+			return 1;
+		}
+	}
+	/* Fall back on metatable methods. */
+	return luaL_getmetafield(L, 1, field_name) != LUA_TNIL ? 1 : 0;
+}
+
+static int
+luaT_netbox_request_newindex(struct lua_State *L)
+{
+	struct netbox_request *request = luaT_check_netbox_request(L, 1);
+	struct mh_strnptr_t *h = request->index;
+	size_t field_name_len;
+	const char *field_name = lua_tolstring(L, 2, &field_name_len);
+	if (field_name == NULL)
+		return luaL_error(L, "invalid index");
+	int field_value_ref = luaL_ref(L, LUA_REGISTRYINDEX);
+	if (field_value_ref == LUA_REFNIL) {
+		/* The field is set to nil. Delete it from the index. */
+		if (h == NULL)
+			return 0;
+		mh_int_t k = mh_strnptr_find_inp(h, field_name,
+						 field_name_len);
+		int ref = (intptr_t)mh_strnptr_node(h, k)->val;
+		luaL_unref(L, LUA_REGISTRYINDEX, ref);
+		mh_strnptr_del(h, k, NULL);
+		return 0;
+	}
+	if (h == NULL) {
+		/* Lazily create the index on the first invocation. */
+		h = request->index = mh_strnptr_new();
+		if (h == NULL) {
+			luaL_unref(L, LUA_REGISTRYINDEX, field_value_ref);
+			return luaL_error(L, "out of memory");
+		}
+	}
+	/* Insert a reference to the new value into the index. */
+	struct mh_strnptr_node_t node = {
+		.str = field_name,
+		.len = field_name_len,
+		.hash = mh_strn_hash(field_name, field_name_len),
+		.val = (void *)(intptr_t)field_value_ref,
+	};
+	struct mh_strnptr_node_t old_node = { NULL, 0, 0, NULL };
+	struct mh_strnptr_node_t *p_old_node = &old_node;
+	if (mh_strnptr_put(h, &node, &p_old_node,
+			   NULL) == mh_end(h)) {
+		luaL_unref(L, LUA_REGISTRYINDEX, field_value_ref);
+		return luaL_error(L, "out of memory");
+	}
+	/* Drop the reference to the overwritten value. */
+	if (p_old_node != NULL)
+		luaL_unref(L, LUA_REGISTRYINDEX, (intptr_t)old_node.val);
+	return 0;
+}
+
 /**
  * Returns true if the response was received for the given request.
  */
@@ -1602,6 +1690,7 @@ netbox_make_request(struct lua_State *L, int idx,
 	fiber_cond_create(&request->cond);
 	request->result_ref = LUA_NOREF;
 	request->error = NULL;
+	request->index = NULL;
 	if (netbox_request_register(request, registry) != 0) {
 		netbox_request_destroy(request);
 		luaT_error(L);
@@ -1988,6 +2077,8 @@ luaopen_net_box(struct lua_State *L)
 
 	static const struct luaL_Reg netbox_request_meta[] = {
 		{ "__gc",           luaT_netbox_request_gc },
+		{ "__index",        luaT_netbox_request_index },
+		{ "__newindex",     luaT_netbox_request_newindex },
 		{ "is_ready",       luaT_netbox_request_is_ready },
 		{ "result",         luaT_netbox_request_result },
 		{ "wait_result",    luaT_netbox_request_wait_result },
diff --git a/test/box/net.box_fiber-async_gh-3107.result b/test/box/net.box_fiber-async_gh-3107.result
index aaaca351a579..e44db726c8cb 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,103 @@ 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 = 'abc'
+---
+...
+future.abc -- abc
+---
+- abc
+...
+future.abc = nil
+---
+...
+future.abc -- nil
+---
+- null
+...
+future[{}] -- error
+---
+- error: '[string "return future[{}] -- error "]:1: invalid index'
+...
+future[{}] = 'abc' -- error
+---
+- error: '[string "future[{}] = ''abc'' -- error "]:1: invalid index'
+...
+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()
diff --git a/test/box/net.box_fiber-async_gh-3107.test.lua b/test/box/net.box_fiber-async_gh-3107.test.lua
index d23f368cbce4..5571aeb935e0 100644
--- a/test/box/net.box_fiber-async_gh-3107.test.lua
+++ b/test/box/net.box_fiber-async_gh-3107.test.lua
@@ -5,8 +5,7 @@ net = require('net.box')
 -- gh-3107: fiber-async netbox.
 --
 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
 function finalize_long() while not cond do fiber.sleep(0.01) end cond:signal() cond = nil end
 s = box.schema.create_space('test')
@@ -36,7 +35,39 @@ err:find('Usage') ~= nil
 _, err = pcall(future.wait_result, future, '100')
 err:find('Usage') ~= nil
 
-box.schema.func.drop('long_function')
+-- Storing user data in future object.
+future = c:eval('return 123', {}, {is_async = true})
+future.abc -- nil
+future.abc = 'abc'
+future.abc -- abc
+future.abc = nil
+future.abc -- nil
+future[{}] -- error
+future[{}] = 'abc' -- error
+future:wait_result() -- 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
+future.data2 = 123
+_ = collectgarbage('collect')
+gc.data2 == nil
+future:wait_result() -- 123
+future = nil
+_ = collectgarbage('collect')
+_ = collectgarbage('collect')
+gc.data3 == nil
+
+box.schema.user.revoke('guest', 'execute', 'universe')
 
 c:close()
 s:drop()
-- 
2.25.1

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

[Cyrill Gorcunov via Tarantool-patches]

On Tue, Aug 10, 2021 at 07:53:54PM +0300, Vladimir Davydov via Tarantool-patches wrote:
> +
> +static int
> +luaT_netbox_request_newindex(struct lua_State *L)
> +{
> +	struct netbox_request *request = luaT_check_netbox_request(L, 1);
> +	struct mh_strnptr_t *h = request->index;
> +	size_t field_name_len;
> +	const char *field_name = lua_tolstring(L, 2, &field_name_len);
> +	if (field_name == NULL)
> +		return luaL_error(L, "invalid index");
> +	int field_value_ref = luaL_ref(L, LUA_REGISTRYINDEX);
> +	if (field_value_ref == LUA_REFNIL) {
> +		/* The field is set to nil. Delete it from the index. */
> +		if (h == NULL)
> +			return 0;
> +		mh_int_t k = mh_strnptr_find_inp(h, field_name,
> +						 field_name_len);

We're guaranteed that field_name present in a hash and never
ever return nil, right? Sorry I don't understand this code just
found this moment suspicious.

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

[Vladimir Davydov via Tarantool-patches]

On Tue, Aug 10, 2021 at 08:15:27PM +0300, Cyrill Gorcunov via Tarantool-patches wrote:
> On Tue, Aug 10, 2021 at 07:53:54PM +0300, Vladimir Davydov via Tarantool-patches wrote:
> > +
> > +static int
> > +luaT_netbox_request_newindex(struct lua_State *L)
> > +{
> > +	struct netbox_request *request = luaT_check_netbox_request(L, 1);
> > +	struct mh_strnptr_t *h = request->index;
> > +	size_t field_name_len;
> > +	const char *field_name = lua_tolstring(L, 2, &field_name_len);
> > +	if (field_name == NULL)
> > +		return luaL_error(L, "invalid index");
> > +	int field_value_ref = luaL_ref(L, LUA_REGISTRYINDEX);
> > +	if (field_value_ref == LUA_REFNIL) {
> > +		/* The field is set to nil. Delete it from the index. */
> > +		if (h == NULL)
> > +			return 0;
> > +		mh_int_t k = mh_strnptr_find_inp(h, field_name,
> > +						 field_name_len);
> 
> We're guaranteed that field_name present in a hash and never
> ever return nil, right?

Of course, we are not. Fixed. Thanks!

Incremental diff:
--
diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index 162ff6c82fb0..6ab0f71cdbe9 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -1413,9 +1413,11 @@ luaT_netbox_request_newindex(struct lua_State *L)
 			return 0;
 		mh_int_t k = mh_strnptr_find_inp(h, field_name,
 						 field_name_len);
-		int ref = (intptr_t)mh_strnptr_node(h, k)->val;
-		luaL_unref(L, LUA_REGISTRYINDEX, ref);
-		mh_strnptr_del(h, k, NULL);
+		if (k != mh_end(h)) {
+			int ref = (intptr_t)mh_strnptr_node(h, k)->val;
+			luaL_unref(L, LUA_REGISTRYINDEX, ref);
+			mh_strnptr_del(h, k, NULL);
+		}
 		return 0;
 	}
 	if (h == NULL) {
diff --git a/test/box/net.box_fiber-async_gh-3107.result b/test/box/net.box_fiber-async_gh-3107.result
index e44db726c8cb..1c48a115e6b5 100644
--- a/test/box/net.box_fiber-async_gh-3107.result
+++ b/test/box/net.box_fiber-async_gh-3107.result
@@ -109,6 +109,13 @@ future.abc -- nil
 ---
 - null
 ...
+future.abc = nil
+---
+...
+future.abc -- nil
+---
+- null
+...
 future.abc = 'abc'
 ---
 ...
@@ -123,6 +130,13 @@ future.abc -- nil
 ---
 - null
 ...
+future.abc = nil
+---
+...
+future.abc -- nil
+---
+- null
+...
 future[{}] -- error
 ---
 - error: '[string "return future[{}] -- error "]:1: invalid index'
diff --git a/test/box/net.box_fiber-async_gh-3107.test.lua b/test/box/net.box_fiber-async_gh-3107.test.lua
index 5571aeb935e0..e08bad056cba 100644
--- a/test/box/net.box_fiber-async_gh-3107.test.lua
+++ b/test/box/net.box_fiber-async_gh-3107.test.lua
@@ -38,10 +38,14 @@ err:find('Usage') ~= nil
 -- Storing user data in future object.
 future = c:eval('return 123', {}, {is_async = true})
 future.abc -- nil
+future.abc = nil
+future.abc -- nil
 future.abc = 'abc'
 future.abc -- abc
 future.abc = nil
 future.abc -- nil
+future.abc = nil
+future.abc -- nil
 future[{}] -- error
 future[{}] = 'abc' -- error
 future:wait_result() -- 123

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

[Vladimir Davydov via Tarantool-patches]

Before commit 954194a1ca5c ("net.box: rewrite request implementation in
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 an mhash, which is created lazily on the
first __newindex invocation. __index falls back on the metatable methods
if a field isn't found in the mhash.

Follow-up #6241
Closes #6306
---
Changes in v2:
 - When a key is deleted from the hash (by setting its value to nil),
   check that it's actually present in the hash.

v1: https://lists.tarantool.org/pipermail/tarantool-patches/2021-August/025346.html

 src/box/lua/net_box.c                         |  93 ++++++++++++++
 test/box/net.box_fiber-async_gh-3107.result   | 117 +++++++++++++++++-
 test/box/net.box_fiber-async_gh-3107.test.lua |  41 +++++-
 3 files changed, 243 insertions(+), 8 deletions(-)

diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index 06e574cdf746..6ab0f71cdbe9 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -134,6 +134,14 @@ struct netbox_request {
 	 * the response hasn't been received yet.
 	 */
 	struct error *error;
+	/**
+	 * User-defined fields: field name -> Lua object reference.
+	 * We allow the user to attach extra information to a future object,
+	 * e.g. a reference to a connection or the invoked method name/args.
+	 * All the information is stored in this table, which is created
+	 * lazily, on the first __newindex invocation.
+	 */
+	struct mh_strnptr_t *index;
 };
 
 static const char netbox_registry_typename[] = "net.box.registry";
@@ -166,6 +174,15 @@ netbox_request_destroy(struct netbox_request *request)
 	luaL_unref(tarantool_L, LUA_REGISTRYINDEX, request->result_ref);
 	if (request->error != NULL)
 		error_unref(request->error);
+	if (request->index != NULL) {
+		struct mh_strnptr_t *h = request->index;
+		mh_int_t k;
+		mh_foreach(h, k) {
+			int ref = (intptr_t)mh_strnptr_node(h, k)->val;
+			luaL_unref(tarantool_L, LUA_REGISTRYINDEX, ref);
+		}
+		mh_strnptr_delete(h);
+	}
 }
 
 /**
@@ -1358,6 +1375,79 @@ 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);
+	size_t field_name_len;
+	const char *field_name = lua_tolstring(L, 2, &field_name_len);
+	if (field_name == NULL)
+		return luaL_error(L, "invalid index");
+	if (request->index != NULL) {
+		mh_int_t k = mh_strnptr_find_inp(request->index, field_name,
+						 field_name_len);
+		if (k != mh_end(request->index)) {
+			int field_value_ref = (intptr_t)mh_strnptr_node(
+				request->index, k)->val;
+			lua_rawgeti(L, LUA_REGISTRYINDEX, field_value_ref);
+			return 1;
+		}
+	}
+	/* Fall back on metatable methods. */
+	return luaL_getmetafield(L, 1, field_name) != LUA_TNIL ? 1 : 0;
+}
+
+static int
+luaT_netbox_request_newindex(struct lua_State *L)
+{
+	struct netbox_request *request = luaT_check_netbox_request(L, 1);
+	struct mh_strnptr_t *h = request->index;
+	size_t field_name_len;
+	const char *field_name = lua_tolstring(L, 2, &field_name_len);
+	if (field_name == NULL)
+		return luaL_error(L, "invalid index");
+	int field_value_ref = luaL_ref(L, LUA_REGISTRYINDEX);
+	if (field_value_ref == LUA_REFNIL) {
+		/* The field is set to nil. Delete it from the index. */
+		if (h == NULL)
+			return 0;
+		mh_int_t k = mh_strnptr_find_inp(h, field_name,
+						 field_name_len);
+		if (k != mh_end(h)) {
+			int ref = (intptr_t)mh_strnptr_node(h, k)->val;
+			luaL_unref(L, LUA_REGISTRYINDEX, ref);
+			mh_strnptr_del(h, k, NULL);
+		}
+		return 0;
+	}
+	if (h == NULL) {
+		/* Lazily create the index on the first invocation. */
+		h = request->index = mh_strnptr_new();
+		if (h == NULL) {
+			luaL_unref(L, LUA_REGISTRYINDEX, field_value_ref);
+			return luaL_error(L, "out of memory");
+		}
+	}
+	/* Insert a reference to the new value into the index. */
+	struct mh_strnptr_node_t node = {
+		.str = field_name,
+		.len = field_name_len,
+		.hash = mh_strn_hash(field_name, field_name_len),
+		.val = (void *)(intptr_t)field_value_ref,
+	};
+	struct mh_strnptr_node_t old_node = { NULL, 0, 0, NULL };
+	struct mh_strnptr_node_t *p_old_node = &old_node;
+	if (mh_strnptr_put(h, &node, &p_old_node,
+			   NULL) == mh_end(h)) {
+		luaL_unref(L, LUA_REGISTRYINDEX, field_value_ref);
+		return luaL_error(L, "out of memory");
+	}
+	/* Drop the reference to the overwritten value. */
+	if (p_old_node != NULL)
+		luaL_unref(L, LUA_REGISTRYINDEX, (intptr_t)old_node.val);
+	return 0;
+}
+
 /**
  * Returns true if the response was received for the given request.
  */
@@ -1602,6 +1692,7 @@ netbox_make_request(struct lua_State *L, int idx,
 	fiber_cond_create(&request->cond);
 	request->result_ref = LUA_NOREF;
 	request->error = NULL;
+	request->index = NULL;
 	if (netbox_request_register(request, registry) != 0) {
 		netbox_request_destroy(request);
 		luaT_error(L);
@@ -1988,6 +2079,8 @@ luaopen_net_box(struct lua_State *L)
 
 	static const struct luaL_Reg netbox_request_meta[] = {
 		{ "__gc",           luaT_netbox_request_gc },
+		{ "__index",        luaT_netbox_request_index },
+		{ "__newindex",     luaT_netbox_request_newindex },
 		{ "is_ready",       luaT_netbox_request_is_ready },
 		{ "result",         luaT_netbox_request_result },
 		{ "wait_result",    luaT_netbox_request_wait_result },
diff --git a/test/box/net.box_fiber-async_gh-3107.result b/test/box/net.box_fiber-async_gh-3107.result
index aaaca351a579..1c48a115e6b5 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,117 @@ 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
+---
+...
+future.abc -- nil
+---
+- null
+...
+future[{}] -- error
+---
+- error: '[string "return future[{}] -- error "]:1: invalid index'
+...
+future[{}] = 'abc' -- error
+---
+- error: '[string "future[{}] = ''abc'' -- error "]:1: invalid index'
+...
+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()
diff --git a/test/box/net.box_fiber-async_gh-3107.test.lua b/test/box/net.box_fiber-async_gh-3107.test.lua
index d23f368cbce4..e08bad056cba 100644
--- a/test/box/net.box_fiber-async_gh-3107.test.lua
+++ b/test/box/net.box_fiber-async_gh-3107.test.lua
@@ -5,8 +5,7 @@ net = require('net.box')
 -- gh-3107: fiber-async netbox.
 --
 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
 function finalize_long() while not cond do fiber.sleep(0.01) end cond:signal() cond = nil end
 s = box.schema.create_space('test')
@@ -36,7 +35,43 @@ err:find('Usage') ~= nil
 _, err = pcall(future.wait_result, future, '100')
 err:find('Usage') ~= nil
 
-box.schema.func.drop('long_function')
+-- Storing user data in future object.
+future = c:eval('return 123', {}, {is_async = true})
+future.abc -- nil
+future.abc = nil
+future.abc -- nil
+future.abc = 'abc'
+future.abc -- abc
+future.abc = nil
+future.abc -- nil
+future.abc = nil
+future.abc -- nil
+future[{}] -- error
+future[{}] = 'abc' -- error
+future:wait_result() -- 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
+future.data2 = 123
+_ = collectgarbage('collect')
+gc.data2 == nil
+future:wait_result() -- 123
+future = nil
+_ = collectgarbage('collect')
+_ = collectgarbage('collect')
+gc.data3 == nil
+
+box.schema.user.revoke('guest', 'execute', 'universe')
 
 c:close()
 s:drop()
-- 
2.25.1

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

[Vladislav Shpilevoy via Tarantool-patches]

Hi! Thanks for the patch!

See 3 comments below.

> diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
> index 06e574cdf746..162ff6c82fb0 100644
> --- a/src/box/lua/net_box.c
> +++ b/src/box/lua/net_box.c
<...>

> +
> +static int
> +luaT_netbox_request_newindex(struct lua_State *L)
> +{
> +	struct netbox_request *request = luaT_check_netbox_request(L, 1);
> +	struct mh_strnptr_t *h = request->index;
> +	size_t field_name_len;
> +	const char *field_name = lua_tolstring(L, 2, &field_name_len);
> +	if (field_name == NULL)
> +		return luaL_error(L, "invalid index");
> +	int field_value_ref = luaL_ref(L, LUA_REGISTRYINDEX);
> +	if (field_value_ref == LUA_REFNIL) {
> +		/* The field is set to nil. Delete it from the index. */

1. But it is not deleted. It is just ignored, which is fine, but
the comment is wrong then.

> +		if (h == NULL)
> +			return 0;
> +		mh_int_t k = mh_strnptr_find_inp(h, field_name,
> +						 field_name_len);
> +		int ref = (intptr_t)mh_strnptr_node(h, k)->val;
> +		luaL_unref(L, LUA_REGISTRYINDEX, ref);
> +		mh_strnptr_del(h, k, NULL);
> +		return 0;
> +	}
> +	if (h == NULL) {
> +		/* Lazily create the index on the first invocation. */
> +		h = request->index = mh_strnptr_new();
> +		if (h == NULL) {
> +			luaL_unref(L, LUA_REGISTRYINDEX, field_value_ref);
> +			return luaL_error(L, "out of memory");
> +		}
> +	}
> +	/* Insert a reference to the new value into the index. */
> +	struct mh_strnptr_node_t node = {
> +		.str = field_name,
> +		.len = field_name_len,
> +		.hash = mh_strn_hash(field_name, field_name_len),

2. lua_hashstring() might be faster. You could reuse already
calculated hash value from Lua. Although for that 'field_name'
must be of exactly string type (lua_tolstring() works not only for
strings). So if you would make a more precise type check, you
could avoid hash calculation completely. Or use lua_hash() for
non-string values' strings. (But I might be wrong about how
lua_hashstring() works for non-string values, you would need to
double-check).

> +		.val = (void *)(intptr_t)field_value_ref,

3. Can you simply cast to void * right away? Why do you need the
intermediate intptr_t cast?

> +	};
> +	struct mh_strnptr_node_t old_node = { NULL, 0, 0, NULL };
> +	struct mh_strnptr_node_t *p_old_node = &old_node;
> +	if (mh_strnptr_put(h, &node, &p_old_node,
> +			   NULL) == mh_end(h)) {
> +		luaL_unref(L, LUA_REGISTRYINDEX, field_value_ref);
> +		return luaL_error(L, "out of memory");
> +	}
> +	/* Drop the reference to the overwritten value. */
> +	if (p_old_node != NULL)
> +		luaL_unref(L, LUA_REGISTRYINDEX, (intptr_t)old_node.val);
> +	return 0;
> +}

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

[Vladislav Shpilevoy via Tarantool-patches]

On 12.08.2021 21:02, Vladislav Shpilevoy via Tarantool-patches wrote:
> Hi! Thanks for the patch!
> 
> See 3 comments below.
> 
>> diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
>> index 06e574cdf746..162ff6c82fb0 100644
>> --- a/src/box/lua/net_box.c
>> +++ b/src/box/lua/net_box.c
> <...>
> 
>> +
>> +static int
>> +luaT_netbox_request_newindex(struct lua_State *L)
>> +{
>> +	struct netbox_request *request = luaT_check_netbox_request(L, 1);
>> +	struct mh_strnptr_t *h = request->index;
>> +	size_t field_name_len;
>> +	const char *field_name = lua_tolstring(L, 2, &field_name_len);
>> +	if (field_name == NULL)
>> +		return luaL_error(L, "invalid index");
>> +	int field_value_ref = luaL_ref(L, LUA_REGISTRYINDEX);
>> +	if (field_value_ref == LUA_REFNIL) {
>> +		/* The field is set to nil. Delete it from the index. */
> 
> 1. But it is not deleted. It is just ignored, which is fine, but
> the comment is wrong then.

Sorry, nevermind. I looked at h == NULL below and misinterpreted it
in a hurry.

>> +		if (h == NULL)
>> +			return 0;

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

[Igor Munkin via Tarantool-patches]

Vova,

Thanks for the patch! I am curious why Lua tables are not chosen for
storing users data? It looks more natural (kinda netbox request storage
that would be similar to the fiber's one), and easier to implement. Of
course the only thing bothering me about Lua tables usage is Lua GC
pressure. However:
* Whether there are *too* much entries, TABOV might occur for REGISTRY
  table and keeping many tables (especially short living ones) at once
  is also not such good option.
* In case there are *just* much entries, I perhaps that REGISTRY
  performance is better that spawning lots of tables. Anyway, I haven't
  look on the mhash implementation, so I have no estimations about its
  perfomance. I see they are created and destroyed manually, but Lua
  tables are within the GC reign, so some time is spent on their
  traversal. At the same time, REGISTRY traversal is atomic, so
  inflating it also affects the time spent on a particular incremental
  GC step. Too much theory, BTW. Benchmarks are required for this case.
* If this is more an exceptional option (that's I doubt considering
  Yaroslav complains regarding this functionality breakage), then any of
  the option is fine IMHO, so the most convenient is preferable.

It would be great to see the comparison for your current implementation
and the one using Lua tables. The most interesting workload is a bunch
of short-term requests, I guess.

-- 
Best regards,
IM

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

[Vladimir Davydov via Tarantool-patches]

On Thu, Aug 12, 2021 at 09:13:56PM +0300, Igor Munkin wrote:
> Vova,
> 
> Thanks for the patch! I am curious why Lua tables are not chosen for
> storing users data? It looks more natural (kinda netbox request storage
> that would be similar to the fiber's one), and easier to implement. Of
> course the only thing bothering me about Lua tables usage is Lua GC
> pressure. However:
> * Whether there are *too* much entries, TABOV might occur for REGISTRY
>   table and keeping many tables (especially short living ones) at once
>   is also not such good option.
> * In case there are *just* much entries, I perhaps that REGISTRY
>   performance is better that spawning lots of tables. Anyway, I haven't
>   look on the mhash implementation, so I have no estimations about its
>   perfomance. I see they are created and destroyed manually, but Lua
>   tables are within the GC reign, so some time is spent on their
>   traversal. At the same time, REGISTRY traversal is atomic, so
>   inflating it also affects the time spent on a particular incremental
>   GC step. Too much theory, BTW. Benchmarks are required for this case.
> * If this is more an exceptional option (that's I doubt considering
>   Yaroslav complains regarding this functionality breakage), then any of
>   the option is fine IMHO, so the most convenient is preferable.
> 
> It would be great to see the comparison for your current implementation
> and the one using Lua tables. The most interesting workload is a bunch
> of short-term requests, I guess.

According to my measurements, Lua tables perform slightly better (+1%)
than mhash. The code looks simpler as well so I'll go with Lua tables.
Will send v3 in reply to this email. Thanks!

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

[Vladimir Davydov via Tarantool-patches]

Before commit 954194a1ca5c ("net.box: rewrite request implementation in
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
@@ -124,6 +124,15 @@ struct netbox_request {
 	/** Lua references to on_push trigger and its context. */
 	int on_push_ref;
 	int on_push_ctx_ref;
+	/**
+	 * Lua reference to a table with user-defined fields.
+	 * We allow the user to attach extra information to a future object,
+	 * e.g. a reference to a connection or the invoked method name/args.
+	 * All the information is stored in this table, which is created
+	 * lazily, on the first __newindex invocation. Until then, it's
+	 * LUA_NOREF.
+	 */
+	int index_ref;
 	/**
 	 * Lua reference to the request result or LUA_NOREF if the response
 	 * hasn't been received yet. If the response was decoded to a
@@ -167,6 +176,7 @@ netbox_request_destroy(struct netbox_request *request)
 	luaL_unref(tarantool_L, LUA_REGISTRYINDEX, request->on_push_ref);
 	luaL_unref(tarantool_L, LUA_REGISTRYINDEX, request->on_push_ctx_ref);
 	luaL_unref(tarantool_L, LUA_REGISTRYINDEX, request->result_ref);
+	luaL_unref(tarantool_L, LUA_REGISTRYINDEX, request->index_ref);
 	if (request->error != NULL)
 		error_unref(request->error);
 }
@@ -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. */
+		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) {
+		/* 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.
  */
@@ -1664,6 +1712,7 @@ netbox_make_request(struct lua_State *L, int idx,
 		request->format = tuple_format_runtime;
 	tuple_format_ref(request->format);
 	fiber_cond_create(&request->cond);
+	request->index_ref = LUA_NOREF;
 	request->result_ref = LUA_NOREF;
 	request->error = NULL;
 	if (netbox_request_register(request, registry) != 0) {
@@ -2052,6 +2101,8 @@ luaopen_net_box(struct lua_State *L)
 
 	static const struct luaL_Reg netbox_request_meta[] = {
 		{ "__gc",           luaT_netbox_request_gc },
+		{ "__index",        luaT_netbox_request_index },
+		{ "__newindex",     luaT_netbox_request_newindex },
 		{ "is_ready",       luaT_netbox_request_is_ready },
 		{ "result",         luaT_netbox_request_result },
 		{ "wait_result",    luaT_netbox_request_wait_result },
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
+---
+...
+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()
diff --git a/test/box/net.box_fiber-async_gh-3107.test.lua b/test/box/net.box_fiber-async_gh-3107.test.lua
index d23f368cbce4..474dfaa4e35b 100644
--- a/test/box/net.box_fiber-async_gh-3107.test.lua
+++ b/test/box/net.box_fiber-async_gh-3107.test.lua
@@ -5,8 +5,7 @@ net = require('net.box')
 -- gh-3107: fiber-async netbox.
 --
 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
 function finalize_long() while not cond do fiber.sleep(0.01) end cond:signal() cond = nil end
 s = box.schema.create_space('test')
@@ -36,7 +35,41 @@ err:find('Usage') ~= nil
 _, err = pcall(future.wait_result, future, '100')
 err:find('Usage') ~= nil
 
-box.schema.func.drop('long_function')
+-- Storing user data in future object.
+future = c:eval('return 123', {}, {is_async = true})
+future.abc -- nil
+future.abc = nil
+future.abc -- nil
+future.abc = 'abc'
+future.abc -- abc
+future.abc = nil
+future.abc -- nil
+future.abc = nil
+future.abc -- nil
+future:wait_result() -- 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
+future.data2 = 123
+_ = collectgarbage('collect')
+gc.data2 == nil
+future:wait_result() -- 123
+future = nil
+_ = collectgarbage('collect')
+_ = collectgarbage('collect')
+gc.data3 == nil
+
+box.schema.user.revoke('guest', 'execute', 'universe')
 
 c:close()
 s:drop()
-- 
2.25.1

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

[Vladimir Davydov via Tarantool-patches]

On Thu, Aug 12, 2021 at 09:02:46PM +0300, Vladislav Shpilevoy wrote:
> > +
> > +static int
> > +luaT_netbox_request_newindex(struct lua_State *L)
> > +{
> > +	struct netbox_request *request = luaT_check_netbox_request(L, 1);
> > +	struct mh_strnptr_t *h = request->index;
> > +	size_t field_name_len;
> > +	const char *field_name = lua_tolstring(L, 2, &field_name_len);
> > +	if (field_name == NULL)
> > +		return luaL_error(L, "invalid index");
> > +	int field_value_ref = luaL_ref(L, LUA_REGISTRYINDEX);
> > +	if (field_value_ref == LUA_REFNIL) {
> > +		/* The field is set to nil. Delete it from the index. */
> 
> 1. But it is not deleted. It is just ignored, which is fine, but
> the comment is wrong then.
> 
> > +		if (h == NULL)
> > +			return 0;
> > +		mh_int_t k = mh_strnptr_find_inp(h, field_name,
> > +						 field_name_len);
> > +		int ref = (intptr_t)mh_strnptr_node(h, k)->val;
> > +		luaL_unref(L, LUA_REGISTRYINDEX, ref);
> > +		mh_strnptr_del(h, k, NULL);
> > +		return 0;
> > +	}
> > +	if (h == NULL) {
> > +		/* Lazily create the index on the first invocation. */
> > +		h = request->index = mh_strnptr_new();
> > +		if (h == NULL) {
> > +			luaL_unref(L, LUA_REGISTRYINDEX, field_value_ref);
> > +			return luaL_error(L, "out of memory");
> > +		}
> > +	}
> > +	/* Insert a reference to the new value into the index. */
> > +	struct mh_strnptr_node_t node = {
> > +		.str = field_name,
> > +		.len = field_name_len,
> > +		.hash = mh_strn_hash(field_name, field_name_len),
> 
> 2. lua_hashstring() might be faster. You could reuse already
> calculated hash value from Lua. Although for that 'field_name'
> must be of exactly string type (lua_tolstring() works not only for
> strings). So if you would make a more precise type check, you
> could avoid hash calculation completely. Or use lua_hash() for
> non-string values' strings. (But I might be wrong about how
> lua_hashstring() works for non-string values, you would need to
> double-check).
> 
> > +		.val = (void *)(intptr_t)field_value_ref,
> 
> 3. Can you simply cast to void * right away? Why do you need the
> intermediate intptr_t cast?

I reworked the patch to use a Lua table instead of mhash, as per
the suggestion by imun@, so these comments are not relevant anymore.
Please see v3:

https://lists.tarantool.org/pipermail/tarantool-patches/2021-August/025458.html

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

[Igor Munkin via Tarantool-patches]

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

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

[Vladimir Davydov via Tarantool-patches]

On Sat, Aug 14, 2021 at 02:17:00PM +0300, Igor Munkin wrote:
> 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).

Fixed.

> > +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.

Added a comment.

> > +		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?

Checked. Using UNLIKELY here in __index and/or __newindex makes no
difference. Leaving as is.

> > +future.abc = nil
> > +---
> > +...
> > +future.abc -- nil
> > +---
> > +- null
> > +...
> > +future.abc = nil
> 
> Looks like a typo: future.abc already equals to nil above.

It's actually not a typo. I wanted to ensure that deleting a key that
has no value works fine. Added clarifying comments.
--
From db18219e23d9d42c56125ec45af28876017e38f0 Mon Sep 17 00:00:00 2001
From: Vladimir Davydov <vdavydov@tarantool.org>
Date: Tue, 10 Aug 2021 14:38:33 +0300
Subject: [PATCH] net.box: allow to store user-defined fields in future object

Before commit 954194a1ca5c2cd1826a8a689663c827313dbbef ("net.box:
rewrite request implementation in 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

diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index 229dec590cf9..5d3d7826270d 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -124,6 +124,15 @@ struct netbox_request {
 	/** Lua references to on_push trigger and its context. */
 	int on_push_ref;
 	int on_push_ctx_ref;
+	/**
+	 * Lua reference to a table with user-defined fields.
+	 * We allow the user to attach extra information to a future object,
+	 * e.g. a reference to a connection or the invoked method name/args.
+	 * All the information is stored in this table, which is created
+	 * lazily, on the first __newindex invocation. Until then, it's
+	 * LUA_NOREF.
+	 */
+	int index_ref;
 	/**
 	 * Lua reference to the request result or LUA_NOREF if the response
 	 * hasn't been received yet. If the response was decoded to a
@@ -167,6 +176,7 @@ netbox_request_destroy(struct netbox_request *request)
 	luaL_unref(tarantool_L, LUA_REGISTRYINDEX, request->on_push_ref);
 	luaL_unref(tarantool_L, LUA_REGISTRYINDEX, request->on_push_ctx_ref);
 	luaL_unref(tarantool_L, LUA_REGISTRYINDEX, request->result_ref);
+	luaL_unref(tarantool_L, LUA_REGISTRYINDEX, request->index_ref);
 	if (request->error != NULL)
 		error_unref(request->error);
 }
@@ -1420,6 +1430,48 @@ 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);
+		/*
+		 * Copy the key (2nd argument) to the top. Note, we don't move
+		 * it with lua_insert, like we do in __newindex, because we want
+		 * to save it for the fallback path below.
+		 */
+		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) {
+		/* 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.
  */
@@ -1664,6 +1716,7 @@ netbox_make_request(struct lua_State *L, int idx,
 		request->format = tuple_format_runtime;
 	tuple_format_ref(request->format);
 	fiber_cond_create(&request->cond);
+	request->index_ref = LUA_NOREF;
 	request->result_ref = LUA_NOREF;
 	request->error = NULL;
 	if (netbox_request_register(request, registry) != 0) {
@@ -2052,6 +2105,8 @@ luaopen_net_box(struct lua_State *L)
 
 	static const struct luaL_Reg netbox_request_meta[] = {
 		{ "__gc",           luaT_netbox_request_gc },
+		{ "__index",        luaT_netbox_request_index },
+		{ "__newindex",     luaT_netbox_request_newindex },
 		{ "is_ready",       luaT_netbox_request_is_ready },
 		{ "result",         luaT_netbox_request_result },
 		{ "wait_result",    luaT_netbox_request_wait_result },
diff --git a/test/box/net.box_fiber-async_gh-3107.result b/test/box/net.box_fiber-async_gh-3107.result
index aaaca351a579..4ff25ed3817c 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    -- delete a key that has never been set
+---
+...
+future.abc -- nil
+---
+- null
+...
+future.abc = 'abc'  -- set a value for a key
+---
+...
+future.abc -- abc
+---
+- abc
+...
+future.abc = nil    -- delete a value for a key
+---
+...
+future.abc -- nil
+---
+- null
+...
+future.abc = nil    -- delete a key with no value
+---
+...
+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()
diff --git a/test/box/net.box_fiber-async_gh-3107.test.lua b/test/box/net.box_fiber-async_gh-3107.test.lua
index d23f368cbce4..7b07ed2def14 100644
--- a/test/box/net.box_fiber-async_gh-3107.test.lua
+++ b/test/box/net.box_fiber-async_gh-3107.test.lua
@@ -5,8 +5,7 @@ net = require('net.box')
 -- gh-3107: fiber-async netbox.
 --
 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
 function finalize_long() while not cond do fiber.sleep(0.01) end cond:signal() cond = nil end
 s = box.schema.create_space('test')
@@ -36,7 +35,41 @@ err:find('Usage') ~= nil
 _, err = pcall(future.wait_result, future, '100')
 err:find('Usage') ~= nil
 
-box.schema.func.drop('long_function')
+-- Storing user data in future object.
+future = c:eval('return 123', {}, {is_async = true})
+future.abc -- nil
+future.abc = nil    -- delete a key that has never been set
+future.abc -- nil
+future.abc = 'abc'  -- set a value for a key
+future.abc -- abc
+future.abc = nil    -- delete a value for a key
+future.abc -- nil
+future.abc = nil    -- delete a key with no value
+future.abc -- nil
+future:wait_result() -- 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
+future.data2 = 123
+_ = collectgarbage('collect')
+gc.data2 == nil
+future:wait_result() -- 123
+future = nil
+_ = collectgarbage('collect')
+_ = collectgarbage('collect')
+gc.data3 == nil
+
+box.schema.user.revoke('guest', 'execute', 'universe')
 
 c:close()
 s:drop()

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

[Igor Munkin via Tarantool-patches]

Vova,

Thanks for the fixes!

On 16.08.21, Vladimir Davydov wrote:
> On Sat, Aug 14, 2021 at 02:17:00PM +0300, Igor Munkin wrote:
> > On 13.08.21, Vladimir Davydov wrote:

<snipped>

> > > +future.abc = nil
> > > +---
> > > +...
> > > +future.abc -- nil
> > > +---
> > > +- null
> > > +...
> > > +future.abc = nil
> > 
> > Looks like a typo: future.abc already equals to nil above.
> 
> It's actually not a typo. I wanted to ensure that deleting a key that
> has no value works fine. Added clarifying comments.

Now I get it, thanks! Feel free to proceed with the patch then.

> --

<snipped>

-- 
Best regards,
IM

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

[Vitaliia Ioffe via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 841 bytes --]


Hi all, 
 
QA LGTM
 
--
Vitaliia Ioffe
 
  
>Понедельник, 16 августа 2021, 12:20 +03:00 от Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>:
> 
>Vova,
>
>Thanks for the fixes!
>
>On 16.08.21, Vladimir Davydov wrote:
>> On Sat, Aug 14, 2021 at 02:17:00PM +0300, Igor Munkin wrote:
>> > On 13.08.21, Vladimir Davydov wrote:
>
><snipped>
>
>> > > +future.abc = nil
>> > > +---
>> > > +...
>> > > +future.abc -- nil
>> > > +---
>> > > +- null
>> > > +...
>> > > +future.abc = nil
>> >
>> > Looks like a typo: future.abc already equals to nil above.
>>
>> It's actually not a typo. I wanted to ensure that deleting a key that
>> has no value works fine. Added clarifying comments.
>
>Now I get it, thanks! Feel free to proceed with the patch then.
>
>> --
>
><snipped>
>
>--
>Best regards,
>IM
 

[-- Attachment #2: Type: text/html, Size: 1469 bytes --]

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

[Vladimir Davydov via Tarantool-patches]

Pushed to master.