[Tarantool-patches] [PATCH 1/2] box: make box.cfg idempotent function

[Maria Khaydich]

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