[Tarantool-patches] [PATCH v3] net.box: allow to store user-defined fields in future object
Vladimir Davydov
vdavydov at tarantool.org
Mon Aug 16 11:27:05 MSK 2021
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 at 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()
More information about the Tarantool-patches
mailing list