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

[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