From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id B9C556EC40; Sat, 14 Aug 2021 14:40:45 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org B9C556EC40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1628941245; bh=u3Q5jBrFBdwKUb5hSWRtTSgmtDasGTs+lhRdK7YJ6aY=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=mP3cXmU9v5zCRleu9GKgZApvdkJwbS1/9j1Bc38pzH37zhdG7ECZt7So0zw0kTD4A ajDE/ca5ZxZPhjjHTcLIKazN9c2k3NZ2fmPwJwwX3XaBSkpB7RUUfhBJU8YwNHE8Ec 4/umcXNxRPYwCdwLqjg++2o1o2I8wqZeFhWTF8zU= Received: from smtpng1.i.mail.ru (smtpng1.i.mail.ru [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 90EA56EC40 for ; Sat, 14 Aug 2021 14:40:42 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 90EA56EC40 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1mEs1o-000274-Kr; Sat, 14 Aug 2021 14:40:41 +0300 Date: Sat, 14 Aug 2021 14:17:00 +0300 To: Vladimir Davydov Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org, yaroslav.dynnikov@tarantool.org Message-ID: <20210814111659.GQ27855@tarantool.org> References: <20210813095700.7vumduzrevgmcuyi@esperanza> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: X-Clacks-Overhead: GNU Terry Pratchett User-Agent: Mutt/1.10.1 (2018-07-13) X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD92087353F0EC44DD906AB4890CDABF0C5CB76CEE71D3E4007182A05F538085040A0040142B306389D4598646B9D83F93389B2B1BBC684DC291896FFC1B72D8FCD X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7B7733D0215A2F71AEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006374B6C65B7367884A58638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D85389A9823E22910EBD6C65C4EACFF816117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCF1175FABE1C0F9B6A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18C26CFBAC0749D213D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B6753C3A5E0A5AB5B7089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A566D507031A0BC0AF1E3225B6ABBEBCB493765D8603B576DFD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA752DA3D96DA0CEF5C48E8E86DC7131B365E7726E8460B7C23C X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34D8FD11F74BDD6E8DE808EFE122CB7D4E15E51CBD2EC00719614C33BE9C332CBC686F217BEA7644D31D7E09C32AA3244C7986CDFB0AB8C290C41D0596D66133613A76366E8A9DE7CA83B48618A63566E0 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj57i8v0hCSFIX5ITcMTiViQ== X-Mailru-Sender: 689FA8AB762F7393C37E3C1AEC41BA5D726FE6EF8BF7E05F48608B969F0A9097A7C8D0F45F857DBFE9F1EFEE2F478337FB559BB5D741EB964C8C2C849690F8E70A04DAD6CC59E33667EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v3] net.box: allow to store user-defined fields in future object X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Igor Munkin via Tarantool-patches Reply-To: Igor Munkin Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "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 > @@ -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 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 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. > */ > 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() > -- > 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