<HTML><BODY><div><div>Thank you for the review! I reworked the patch:</div><hr><div><div>Saving box.execute method before explicitly configuring box<br>automatically invokes load_cfg() nonetheless. Further calls<br>to box.cfg{} caused its reconfiguration and replacement of<br>previously saved box.execute meaning it couldn't be used<br>again without leading to an error.</div><div> </div><div>The patch introduces a workaround making box.execute idempotent<br>and is heavily influenced by @Totktonada.</div><div> </div><div>Closes #4231<br>---<br>Issue:<br><a href="https://github.com/tarantool/tarantool/issues/4231">https://github.com/tarantool/tarantool/issues/4231</a> <br>Branch:<br><a href="https://github.com/tarantool/tarantool/compare/eljashm/gh-4231-box.execute-immutable-function">https://github.com/tarantool/tarantool/compare/eljashm/gh-4231-box.execute-immutable-function</a> </div><div> </div><div> extra/exports | 2 +<br> src/box/lua/load_cfg.lua | 42 ++++++++++++++++---<br> .../gh-4231-immutable-box-execute.test.lua | 28 +++++++++++++<br> 3 files changed, 67 insertions(+), 5 deletions(-)<br> create mode 100755 test/box-tap/gh-4231-immutable-box-execute.test.lua</div><div>diff --git a/extra/exports b/extra/exports<br>index 7b84a1452..8599614ee 100644<br>--- a/extra/exports<br>+++ b/extra/exports<br>@@ -118,6 +118,8 @@ swim_member_unref<br> swim_member_is_dropped<br> swim_member_is_payload_up_to_date<br> <br>+box_is_configured<br>+<br> # Module API<br> <br> _say<br>diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua<br>index 85617c8f0..dc4293bdd 100644<br>--- a/src/box/lua/load_cfg.lua<br>+++ b/src/box/lua/load_cfg.lua<br>@@ -6,6 +6,12 @@ local private = require('box.internal')<br> local urilib = require('uri')<br> local math = require('math')<br> local fiber = require('fiber')<br>+local ffi = require('ffi')<br>+<br>+ffi.cdef([[<br>+bool<br>+box_is_configured(void);<br>+]])<br> <br> -- Function decorator that is used to prevent box.cfg() from<br> -- being called concurrently by different fibers.<br>@@ -558,15 +564,41 @@ local function load_cfg(cfg)<br> end<br> box.cfg = locked(load_cfg)<br> <br>+local function box_is_configured()<br>+ return ffi.C.box_is_configured()<br>+end<br>+<br> --<br>--- This makes possible do box.execute without calling box.cfg<br>--- manually. The load_cfg call overwrites following table and<br>--- metatable.<br>+-- box.execute is <box_load_and_execute> when box is not loaded,<br>+-- <lbox_execute> otherwise. See also <box_configured> variable.<br>+-- <box_load_and_execute> loads box and calls <lbox_execute>.<br> --<br>-function box.execute(...)<br>- load_cfg()<br>+local box_load_and_execute<br>+box_load_and_execute = function(...)<br>+ -- Configure box if it is not configured, no-op otherwise (not<br>+ -- reconfiguration).<br>+ -- We should check whether box is configured, because a user<br>+ -- may save <box_load_and_execute> function before box loading<br>+ -- and call it afterwards.<br>+ if not box_is_configured() then<br>+ locked(function()<br>+ -- Re-check, because after releasing of the lock the<br>+ -- box state may be changed. We should call load_cfg()<br>+ -- only once.<br>+ if not box_is_configured() then<br>+ load_cfg()<br>+ end<br>+ end)()<br>+ end<br>+<br>+ -- At this point box should be configured and box.execute()<br>+ -- function should be replaced with lbox_execute().<br>+ assert(type(box.cfg) ~= 'function')<br>+ assert(box.execute ~= box_load_and_execute)<br>+<br> return box.execute(...)<br> end<br>+box.execute = box_load_and_execute<br> <br> -- gh-810:<br> -- hack luajit default cpath<br>diff --git a/test/box-tap/gh-4231-immutable-box-execute.test.lua b/test/box-tap/gh-4231-immutable-box-execute.test.lua<br>new file mode 100755<br>index 000000000..1544591bf<br>--- /dev/null<br>+++ b/test/box-tap/gh-4231-immutable-box-execute.test.lua<br>@@ -0,0 +1,28 @@<br>+#!/usr/bin/env tarantool<br>+local tap = require('tap')<br>+local test = tap.test('execute')<br>+test:plan(2)<br>+<br>+--<br>+-- gh-4231: box.execute should be an idempotent function<br>+-- meaning its effect should be the same if a user chooses<br>+-- to use it before explicit box.cfg invocation<br>+--<br>+<br>+local function execute_is_immutable(execute, cmd, msg)<br>+ local status, err = pcall(execute, cmd)<br>+ test:ok(status and type(err) == 'table', msg)<br>+end<br>+<br>+local box_execute_stub = box.execute<br>+-- explicit call to load_cfg<br>+box.cfg{}<br>+local box_execute_actual = box.execute<br>+<br>+execute_is_immutable(box_execute_stub,<br>+ "CREATE TABLE t1 (s1 INTEGER, PRIMARY KEY (s1));",<br>+ "box.execute stub works before box.cfg")<br>+execute_is_immutable(box_execute_actual, "DROP TABLE t1",<br>+ "box.execute works properly after box.cfg")<br>+<br>+os.exit(test:check() and 0 or 1)<br>-- <br>2.24.0</div></div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">Четверг, 20 февраля 2020, 20:51 +03:00 от Alexander Turenko <alexander.turenko@tarantool.org>:<br> <div id=""><div class="js-helper js-readmsg-msg"><style type="text/css"></style><div><div id="style_15822210631225461495_BODY">Sorry for the late response.<br><br>See comments below.<br><br>While looking into the patch, experimenting and commenting I made the<br>patch (it is based on your changes), so I'll paste it at end of the<br>email for the reference.<br><br>WBR, Alexander Turenko.<br><br>> commit 573318ea8749931741247f181333ab68b43a82c6<br>> Author: Maria <<a href="/compose?To=maria.khaydich@tarantool.org">maria.khaydich@tarantool.org</a>><br>> Date: Tue Oct 29 17:56:51 2019 +0300<br>><br>> box: box.execute should be immutable function<br><br>Now I recalled the right term: idempotent.<br><br>Nit: it would be good to use imperative mood in the commit header:<br><br> | Use the imperative mood in the subject line. A properly formed Git<br> | commit subject line should always be able to complete the following<br> | sentence: “If applied, this commit will /your subject line here/”.<br><br><a href="https://www.tarantool.io/en/doc/1.10/dev_guide/developer_guidelines/#how-to-write-a-commit-message" target="_blank">https://www.tarantool.io/en/doc/1.10/dev_guide/developer_guidelines/#how-to-write-a-commit-message</a><br><br>><br>> Using box.execute method before explicitly configuring box<br>> automatically invoked box.cfg nonetheless. Any further calls<br>> to box.cfg caused its reconfiguration which then led to an<br>> error when trying to use execute method again.<br><br>Sorry that I nitpick around wording again, but after the description I<br>would check something like the following:<br><br> | box.execute('SELECT 1') -- here box.cfg{} is called under hood<br> | box.execute('SELECT 1') -- everything looks ok<br><br>After that I would be confused, because everything works as expected.<br><br>I would say that there are two different functions<br><box_load_and_execute> (which is assigned to box.execute when box is not<br>configured) and <lbox_execute> (after box confguration) and would<br>describe the case using those terms. I guess it would easier to<br>understand.<br><br>See also the comment about the test at very end of the email.<br><br>><br>> The patch introduces a fix making box.execute immutable.<br>><br>> Closes #4231<br>><br>> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua<br>> index 85617c8f0..1f9b4392f 100644<br>> --- a/src/box/lua/load_cfg.lua<br>> +++ b/src/box/lua/load_cfg.lua<br>> @@ -517,10 +517,14 @@ setmetatable(box, {<br>> end<br>> })<br>><br>> +-- Flag needed to keep immutable functions after box reconfiguration<br><br>Nit: over 66 symbols.<br><br>> +local box_loaded = false<br>> +<br>> local function load_cfg(cfg)<br>> cfg = upgrade_cfg(cfg, translate_cfg)<br>> cfg = prepare_cfg(cfg, default_cfg, template_cfg, modify_cfg)<br>> apply_default_cfg(cfg, default_cfg);<br>> + box_loaded = true<br><br>After this we can failed to configure box, consider the case:<br><br> | tarantool> box.cfg{listen = 'incorrect'}<br> | ---<br> | - error: 'Incorrect value for option ''listen'': expected host:service or /unix.socket'<br> | ...<br> |<br> | tarantool> box.execute('SELECT 1')<br><br>We should set the flag only at successful box configuration. I would<br>also add such test case.<br><br>> -- Save new box.cfg<br>> box.cfg = cfg<br>> if not pcall(private.cfg_check) then<br>> @@ -564,7 +568,16 @@ box.cfg = locked(load_cfg)<br>> -- metatable.<br>> --<br>> function box.execute(...)<br>> - load_cfg()<br>> + --<br>> + -- Saving box.execute method before explicit calls to either<br>> + -- box.execute() or box.cfg{} leads to implicit invocation of<br>> + -- box.cfg nonetheless.<br><br>Technically saving of box.execute does not lead to invocation of<br>box.cfg. But anyway I would describe things in the way as proposed<br>above.<br><br>> + -- The flag box_loaded makes sure execute<br>> + -- is an immutable function meaning it won't be overwritten by<br>> + -- any following attempts to reconfigure box.<br>> + --<br><br>The problem is not that <lbox_execute> will be overwritten with some<br><box_another_execute>, but that the initial <box_load_and_execute> does<br>not work correctly after successful box configuration.<br><br>> + if not box_loaded then<br>> + load_cfg()<br>> + end<br><br>I considered using of `type(box.cfg) == 'function'` check as in<br>tarantoolctl, but in fact it is not realiable: if box was not configured<br>after box.cfg() due to an error (say, after `box.cfg{listen =<br>'invalid'}`) the type of box.cfg will be 'table' and reload_cfg() will<br>work on the next box.cfg() call. So we should use box_is_configured C<br>function:<br><br> | local ffi = require('ffi')<br> |<br> | ffi.cdef([[<br> | bool<br> | box_is_configured(void);<br> | ]])<br> |<br> | local function box_is_configured()<br> | return ffi.C.box_is_configured()<br> | end<br><br>This way requires to add the function into extra/exports.<br><br>BTW, it seems that it is possible that <box_load_and_execute> will be<br>called when `box.cfg{}` is already in progress in another fiber: this is<br>why all load_cfg() / reload_cfg() calls are decorated using `locked`<br>wrapper. It seems we should do the same in <box_load_and_execute>:<br><br> | function box.execute(...)<br> | if not box_is_configured() then<br> | locked(function()<br> | -- Re-check, because after the lock release the box<br> | -- state may be changed. We should call load_cfg()<br> | -- only once.<br> | if not box_is_configured() then<br> | load_cfg()<br> | end<br> | end)()<br> | end<br> | return box.execute(...)<br> | end<br><br>I experimented with this like so:<br><br> | $ ./src/tarantool<br> | tarantool> box_execute = box.execute<br> | tarantool> for i = 1, 10 do require('fiber').create(function() require('log').info(require('yaml').encode((box_execute('SELECT * FROM "_vindex"')))) end) end<br><br>While looking into the code I found one more case:<br><br> | tarantool> box_cfg = box.cfg<br> | ---<br> | ...<br> | tarantool> box_cfg()<br> | <...><br> | tarantool> box_cfg()<br> | ---<br> | - error: 'builtin/box/load_cfg.lua:19: builtin/box/load_cfg.lua:547: bad argument<br> | #1 to ''pairs'' (table expected, got nil)'<br> | ...<br><br>I think it does NOT mean that we should not fix box.execute() code,<br>because the expected behaviour is a bit different:<br><br>* box.execute() (more specifically <box_load_and_execute>) should<br> configure box if it is not configured or just execute otherwise (w/o<br> box reconfiguration).<br>* box.cfg() (more specifically <load_cfg>) should configure box if it is<br> not configured, otherwise **reconfigure** it.<br><br>So box.cfg() should call reload_cfg() if box is already configured. I<br>propose to fix it in a separate patch (but within the scope of the<br>issue).<br><br>> return box.execute(...)<br>> end<br>><br>> diff --git a/test/box-tap/gh-4231-immutable-box-execute.test.lua b/test/box-tap/gh-4231-immutable-box-execute.test.lua<br>> new file mode 100755<br>> index 000000000..5bdbec88f<br>> --- /dev/null<br>> +++ b/test/box-tap/gh-4231-immutable-box-execute.test.lua<br>> @@ -0,0 +1,33 @@<br>> +#!/usr/bin/env tarantool<br>> +local tap = require('tap')<br>> +local test = tap.test('execute')<br>> +test:plan(3)<br>> +<br>> +--<br>> +-- gh-4231: box.execute should be immutable function meaning it's<br>> +-- address doesn't change after first box.cfg implicit invocation<br><br>But it is not so even after the patch:<br><br> | tarantool> box.execute<br> | ---<br> | - 'function: 0x4074bc30'<br> | ...<br> | tarantool> box.execute('SELECT 1')<br> | <...><br> | tarantool> box.execute<br> | ---<br> | - 'function: 0x416b2600'<br> | ...<br><br>> +--<br>> +<br>> +local function execute_is_immutable(execute, cmd, msg)<br>> + local status, err = pcall(function()<br>> + execute(cmd)<br>> + end)<br><br>Nit: Can be written as:<br><br> | local status, err = pcall(execute, cmd)<br><br>> + test:ok(status and err == nil, msg)<br>> +end<br>> +<br>> +local box_execute_stub = box.execute<br>> +execute_is_immutable(box_execute_stub, "SELECT 1",<br>> + "execute does not work properly before box.cfg")<br><br>This will print:<br><br>ok - execute does not work properly before box.cfg<br><br>--that looks counter-intuitive for me.<br><br>BTW, it looks as the preparation code for the test rather than the test<br>itself. Personally I think that such code should do assert() rather than<br>produce 'ok/not ok' TAP13 output. This is not the strict requirement,<br>however.<br><br>Consider [1], from words 'I would split test preparation / clean up code<br>from actual test cases'.<br><br>[1]: <a href="https://lists.tarantool.org/pipermail/tarantool-patches/2020-January/013595.html" target="_blank">https://lists.tarantool.org/pipermail/tarantool-patches/2020-January/013595.html</a><br><br>> +<br>> +local box_execute_actual = box.execute<br>> +-- explicit call to load_cfg<br>> +box.cfg{}<br>> +<br>> +-- checking the function was not reconfigured, i.e. adress stays the same<br><br>Typo: adress -> address.<br><br>> +test:is(box_execute_actual, box.execute,<br>> + "execute is not the same after box.cfg")<br>> +execute_is_immutable(box_execute_actual,<br>> + "CREATE TABLE t1 (s1 INTEGER, PRIMARY KEY (s1));",<br>> + "execute does not work properly after box.cfg")<br>> +<br>> +os.exit(test:check() and 0 or 1)<br><br>The test case works properly (does not fail) on current master w/o the<br>fix. I guess you intend to test box_execute_stub after box.cfg().<br><br>BTW, I suggest to run a test on a tarantool version before a fix to<br>verify that the test actually able to catch the problem.<br><br>----<br><br>diff --git a/extra/exports b/extra/exports<br>index 7b84a1452..8599614ee 100644<br>--- a/extra/exports<br>+++ b/extra/exports<br>@@ -118,6 +118,8 @@ swim_member_unref<br> swim_member_is_dropped<br> swim_member_is_payload_up_to_date<br> <br>+box_is_configured<br>+<br> # Module API<br> <br> _say<br>diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua<br>index 1f9b4392f..dfff772d7 100644<br>--- a/src/box/lua/load_cfg.lua<br>+++ b/src/box/lua/load_cfg.lua<br>@@ -6,6 +6,12 @@ local private = require('box.internal')<br> local urilib = require('uri')<br> local math = require('math')<br> local fiber = require('fiber')<br>+local ffi = require('ffi')<br>+<br>+ffi.cdef([[<br>+ bool<br>+ box_is_configured(void);<br>+]])<br> <br> -- Function decorator that is used to prevent box.cfg() from<br> -- being called concurrently by different fibers.<br>@@ -517,14 +523,10 @@ setmetatable(box, {<br> end<br> })<br> <br>--- Flag needed to keep immutable functions after box reconfiguration<br>-local box_loaded = false<br>-<br> local function load_cfg(cfg)<br> cfg = upgrade_cfg(cfg, translate_cfg)<br> cfg = prepare_cfg(cfg, default_cfg, template_cfg, modify_cfg)<br> apply_default_cfg(cfg, default_cfg);<br>- box_loaded = true<br> -- Save new box.cfg<br> box.cfg = cfg<br> if not pcall(private.cfg_check) then<br>@@ -562,24 +564,43 @@ local function load_cfg(cfg)<br> end<br> box.cfg = locked(load_cfg)<br> <br>+local function box_is_configured()<br>+ return ffi.C.box_is_configured()<br>+end<br>+<br> --<br>--- This makes possible do box.execute without calling box.cfg<br>--- manually. The load_cfg call overwrites following table and<br>--- metatable.<br>+-- box.execute is <box_load_and_execute> when box is not loaded,<br>+-- <lbox_execute> otherwise. See also <box_configured> variable.<br> --<br>-function box.execute(...)<br>- --<br>- -- Saving box.execute method before explicit calls to either<br>- -- box.execute() or box.cfg{} leads to implicit invocation of<br>- -- box.cfg nonetheless. The flag box_loaded makes sure execute<br>- -- is an immutable function meaning it won't be overwritten by<br>- -- any following attempts to reconfigure box.<br>+-- <box_load_and_execute> loads box and calls <lbox_execute>.<br>+--<br>+local box_load_and_execute<br>+box_load_and_execute = function(...)<br>+ -- Configure box if it is not configured, no-op otherwise (not<br>+ -- reconfiguration).<br> --<br>- if not box_loaded then<br>- load_cfg()<br>+ -- We should check whether box is configured, because a user<br>+ -- may save <box_load_and_execute> function before box loading<br>+ -- and call it afterwards.<br>+ if not box_is_configured() then<br>+ locked(function()<br>+ -- Re-check, because after releasing of the lock the<br>+ -- box state may be changed. We should call load_cfg()<br>+ -- only once.<br>+ if not box_is_configured() then<br>+ load_cfg()<br>+ end<br>+ end)()<br> end<br>+<br>+ -- At this point box should be configured and box.execute()<br>+ -- function should be replaced with lbox_execute().<br>+ assert(type(box.cfg) ~= 'function')<br>+ assert(box.execute ~= box_load_and_execute)<br>+<br> return box.execute(...)<br> end<br>+box.execute = box_load_and_execute<br> <br> -- gh-810:<br> -- hack luajit default cpath</div></div></div></div></blockquote> <div> </div><div data-signature-widget="container"><div data-signature-widget="content"><div>--<br>Maria Khaydich</div></div></div><div> </div></div></BODY></HTML>