Re: [Tarantool-patches] [PATCH] box: make box.execute() immutable

["Maria Khaydich" <maria.khaydich@tarantool.org>]

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


Thank you for the review! I reworked the patch:
----------------------------------------------------------------------
Saving box.execute method before explicitly configuring box
automatically invokes load_cfg() nonetheless. Further calls
to box.cfg{} caused its reconfiguration and replacement of
previously saved box.execute meaning it couldn't be used
again without leading to an error.
 
The patch introduces a workaround making box.execute idempotent
and is heavily influenced by @Totktonada.
 
Closes #4231
---
Issue:
https://github.com/tarantool/tarantool/issues/4231  
Branch:
https://github.com/tarantool/tarantool/compare/eljashm/gh-4231-box.execute-immutable-function  
 
 extra/exports                                 |  2 +
 src/box/lua/load_cfg.lua                      | 42 ++++++++++++++++---
 .../gh-4231-immutable-box-execute.test.lua    | 28 +++++++++++++
 3 files changed, 67 insertions(+), 5 deletions(-)
 create mode 100755 test/box-tap/gh-4231-immutable-box-execute.test.lua
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 85617c8f0..dc4293bdd 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.
@@ -558,15 +564,41 @@ 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.
+-- <box_load_and_execute> loads box and calls <lbox_execute>.
 --
-function box.execute(...)
-    load_cfg()
+local box_load_and_execute
+box_load_and_execute = function(...)
+    -- Configure box if it is not configured, no-op otherwise (not
+    -- reconfiguration).
+    -- 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
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..1544591bf
--- /dev/null
+++ b/test/box-tap/gh-4231-immutable-box-execute.test.lua
@@ -0,0 +1,28 @@
+#!/usr/bin/env tarantool
+local tap = require('tap')
+local test = tap.test('execute')
+test:plan(2)
+
+--
+-- 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
+--
+
+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
+box.cfg{}
+local box_execute_actual = box.execute
+
+execute_is_immutable(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",
+    "box.execute works properly after box.cfg")
+
+os.exit(test:check() and 0 or 1)
-- 
2.24.0 
>Четверг, 20 февраля 2020, 20:51 +03:00 от Alexander Turenko <alexander.turenko@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@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
 

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