[Tarantool-patches] [PATCH] box.execute should be immutable function

[Tarantool-patches] [PATCH] box.execute should be immutable function

[Maria]

Using box.execute method before explicitly
configuring box automatically invoked box.cfg
nonetheless. Any further calls to the latter
caused its reconfiguration which could lead
to an error when trying to use the method.

Closes #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      |  4 +++-
 test/box-tap/execute.test.lua | 17 +++++++++++++++++
 2 files changed, 20 insertions(+), 1 deletion(-)
 create mode 100755 test/box-tap/execute.test.lua

diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
index e7f62cf4e..042edf913 100644
--- a/src/box/lua/load_cfg.lua
+++ b/src/box/lua/load_cfg.lua
@@ -528,7 +528,9 @@ box.cfg = locked(load_cfg)
 -- metatable.
 --
 function box.execute(...)
-    load_cfg()
+    if not box.cfg then
+        load_cfg()
+    end
     return box.execute(...)
 end
 
diff --git a/test/box-tap/execute.test.lua b/test/box-tap/execute.test.lua
new file mode 100755
index 000000000..301cf4a1c
--- /dev/null
+++ b/test/box-tap/execute.test.lua
@@ -0,0 +1,17 @@
+#!/usr/bin/env tarantool
+
+local tap = require('tap')
+local test = tap.test('execute')
+test:plan(1)
+
+local box_execute = box.execute
+box.cfg{}
+
+local status, err = pcall(
+    function() 
+        box_execute("CREATE TABLE t1 (s1 INTEGER, PRIMARY KEY (s1));") 
+    end)
+
+test:ok(status and err == nil, "box.execute after box.cfg")
+test:check()
+os.exit(0)
-- 
2.21.0 (Apple Git-122.2)

Re: [Tarantool-patches] [PATCH] box.execute should be immutable function

[Nikita Pettik]

On 14 Nov 14:50, Maria wrote:
> Using box.execute method before explicitly
> configuring box automatically invoked box.cfg
> nonetheless. Any further calls to the latter
> caused its reconfiguration which could lead
> to an error when trying to use the method.

Hello. I'm not going to comment patch iteself, just want to leave
a couple of general nits. They are just recommendations.

Feel free to use up to 72 characters while making up commit's body.

I'd fix commit subject:

box.execute should be immutable -> box: make box.execute() immutable despite box.cfg{}
(or sort of).

> Closes #4231
> Issue:
> https://github.com/tarantool/tarantool/issues/4231
> Branch:
> https://github.com/tarantool/tarantool/compare/eljashm/gh-4231-box.execute-immutable-function

Please, put links under --- delimiter so that it will be ignored
by git am.

> ---
>  src/box/lua/load_cfg.lua      |  4 +++-
>  test/box-tap/execute.test.lua | 17 +++++++++++++++++
>  2 files changed, 20 insertions(+), 1 deletion(-)
>  create mode 100755 test/box-tap/execute.test.lua
> 
> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
> index e7f62cf4e..042edf913 100644
> --- a/src/box/lua/load_cfg.lua
> +++ b/src/box/lua/load_cfg.lua
> @@ -528,7 +528,9 @@ box.cfg = locked(load_cfg)
>  -- metatable.
>  --
>  function box.execute(...)
> -    load_cfg()
> +    if not box.cfg then
> +        load_cfg()
> +    end
>      return box.execute(...)
>  end
>  
> diff --git a/test/box-tap/execute.test.lua b/test/box-tap/execute.test.lua
> new file mode 100755
> index 000000000..301cf4a1c
> --- /dev/null
> +++ b/test/box-tap/execute.test.lua

I'd better name this test ..box-tap/gh-4231-immutable-execute-func.test.lua
It is generally accepted naming way.

> @@ -0,0 +1,17 @@
> +#!/usr/bin/env tarantool
> +
> +local tap = require('tap')
> +local test = tap.test('execute')
> +test:plan(1)

Please, leave comment explaining what exactly is tested here.
For instance:

-- Make sure that box.execute() method does not change after
-- the very first box.cfg{} invokation.
--

> +
> +local box_execute = box.execute
> +box.cfg{}
> +
> +local status, err = pcall(
> +    function() 
> +        box_execute("CREATE TABLE t1 (s1 INTEGER, PRIMARY KEY (s1));") 
> +    end)
> +
> +test:ok(status and err == nil, "box.execute after box.cfg")

I guess this should be error message which is displayed in case
test fails. If so, I'd rather use message like:
"box.execute() is changed after box.cfg{} invokation"

Re: [Tarantool-patches] [PATCH] box.execute should be immutable function

[Igor Munkin]

Masha,

Thanks for the patch! I join to all remarks left by Nikita. Besides, I
added a couple new below related to the patch itself.

On 14.11.19, Maria wrote:
> Using box.execute method before explicitly
> configuring box automatically invoked box.cfg
> nonetheless. Any further calls to the latter
> caused its reconfiguration which could lead
> to an error when trying to use the method.
> 
> Closes #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      |  4 +++-
>  test/box-tap/execute.test.lua | 17 +++++++++++++++++
>  2 files changed, 20 insertions(+), 1 deletion(-)
>  create mode 100755 test/box-tap/execute.test.lua
> 
> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
> index e7f62cf4e..042edf913 100644
> --- a/src/box/lua/load_cfg.lua
> +++ b/src/box/lua/load_cfg.lua
> @@ -528,7 +528,9 @@ box.cfg = locked(load_cfg)
>  -- metatable.
>  --
>  function box.execute(...)
> -    load_cfg()

I had to read a lot of source code to dive into the problem to be fixed.
It's OK, but IMHO it would be great if you dropped a few words right
here about the bug and mentioned the related issue. This would
definitely simplify further maintenance.

> +    if not box.cfg then

This condition solves the reported problem (the test you provided within
this patch is OK), but consider the following:
| $ git lo -1
| ec09fcaac <snipped> box.execute should be immutable function
| $ ./tarantool -V
| Tarantool 2.3.0-161-gec09fcaac
| <snipped>
| $ echo 'box.execute("SELECT 1")' | timeout -s 9 3 ./tarantool
| Killed
Thus box.execute call prior to box.cfg invokation hangs Tarantool after
your patch. I'm totally not into SQL design but AFAIS such box.execute
invokation ought to call loag_cfg underneath. However it doesn't since
box.cfg is initialized with function several lines above, the check is
failed and Tarantool goes into deep recursion.

I guess you can introduce a local flag and use it as an upvalue in both
box.execute and load_cfg functions. The flag is to be initialized to
false, set in the load_cfg call and check within box.execute. However
feel free to provide your own solution.

> +        load_cfg()
> +    end
>      return box.execute(...)
>  end
>  
> diff --git a/test/box-tap/execute.test.lua b/test/box-tap/execute.test.lua
> new file mode 100755
> index 000000000..301cf4a1c
> --- /dev/null
> +++ b/test/box-tap/execute.test.lua
> @@ -0,0 +1,17 @@
> +#!/usr/bin/env tarantool
> +
> +local tap = require('tap')
> +local test = tap.test('execute')
> +test:plan(1)
> +
> +local box_execute = box.execute

Please consider to add the check related to the flaw found above.

> +box.cfg{}
> +
> +local status, err = pcall(
> +    function() 
> +        box_execute("CREATE TABLE t1 (s1 INTEGER, PRIMARY KEY (s1));") 

Minor: A trailing whitespace is left above.

> +    end)

There is a common code style for pcall an anonymous multiline Lua
function within box-tap (see [1] and [2]). Please adjust yours
considering it.

> +
> +test:ok(status and err == nil, "box.execute after box.cfg")

Please consider the comment regarding the test finalization[3].

> +test:check()
> +os.exit(0)
> -- 
> 2.21.0 (Apple Git-122.2)
> 

[1]: https://github.com/tarantool/tarantool/blob/master/test/box-tap/cfg.test.lua#L148
[2]: https://github.com/tarantool/tarantool/blob/master/test/box-tap/merger.test.lua#L573
[3]: https://github.com/tarantool/tarantool/issues/4655#issue-529430689

-- 
Best regards,
IM

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

[Maria Khaydich]

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


Igor, thank you for the review!
Proposed fixes are done:
 
 
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.
 
The patch introduces a fix making box.execute method immutable.
 
Closes #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                      | 15 +++++++++++-
 .../gh-4231_immutable_box.execute.test.lua    | 24 +++++++++++++++++++
 2 files changed, 38 insertions(+), 1 deletion(-)
 create mode 100644 test/box-tap/gh-4231_immutable_box.execute.test.lua
diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
index e7f62cf4e..ad416e578 100644
--- a/src/box/lua/load_cfg.lua
+++ b/src/box/lua/load_cfg.lua
@@ -481,10 +481,14 @@ 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
@@ -528,7 +532,16 @@ box.cfg = locked(load_cfg)
 -- metatable.
 --
 function box.execute(...)
-    load_cfg()
+    --
+    -- Calling box.execute before explicitly configuring box led to
+    -- automatic box.cfg invocation nonetheless. Following explicit
+    -- attempts to configure box led to its reconfiguratin and as a 
+    -- result - to an error when trying to use execute method again. 
+    -- The check makes sure box.execute is an immutable function.
+    --
+    if not box_loaded then
+        load_cfg()
+    end
     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 100644
index 000000000..e2469630e
--- /dev/null
+++ b/test/box-tap/gh-4231_immutable_box.execute.test.lua
@@ -0,0 +1,24 @@
+#!/usr/bin/env tarantool
+local tap = require('tap')
+local test = tap.test('execute')
+test:plan(2)
+
+--
+-- gh-4231: box.execute should be immutable function meaning it doesn't
+-- change after first box.cfg implicit invocation
+--
+
+local box_execute = box.execute
+local status, err = pcall(function()
+    box_execute("SELECT 1")
+end)
+test:ok(status and err == nil, "box.execute does not work properly before box.cfg")
+
+box.cfg{}
+
+local status2, err2 = pcall(function()
+    box_execute("CREATE TABLE t1 (s1 INTEGER, PRIMARY KEY (s1));")
+end)
+test:ok(status2 and err2 == nil, "box.execute is changed after box.cfg invocation")
+
+os.exit(test:check() and 0 or 1)
-- 
2.24.0
  
>Вторник, 17 декабря 2019, 17:41 +03:00 от Igor Munkin <imun@tarantool.org>:
> 
>Masha,
>
>Thanks for the patch! I join to all remarks left by Nikita. Besides, I
>added a couple new below related to the patch itself.
>
>On 14.11.19, Maria wrote:
>> Using box.execute method before explicitly
>> configuring box automatically invoked box.cfg
>> nonetheless. Any further calls to the latter
>> caused its reconfiguration which could lead
>> to an error when trying to use the method.
>>
>> Closes #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 | 4 +++-
>> test/box-tap/execute.test.lua | 17 +++++++++++++++++
>> 2 files changed, 20 insertions(+), 1 deletion(-)
>> create mode 100755 test/box-tap/execute.test.lua
>>
>> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
>> index e7f62cf4e..042edf913 100644
>> --- a/src/box/lua/load_cfg.lua
>> +++ b/src/box/lua/load_cfg.lua
>> @@ -528,7 +528,9 @@ box.cfg = locked(load_cfg)
>> -- metatable.
>> --
>> function box.execute(...)
>> - load_cfg()
>
>I had to read a lot of source code to dive into the problem to be fixed.
>It's OK, but IMHO it would be great if you dropped a few words right
>here about the bug and mentioned the related issue. This would
>definitely simplify further maintenance.
>
>> + if not box.cfg then
>
>This condition solves the reported problem (the test you provided within
>this patch is OK), but consider the following:
>| $ git lo -1
>| ec09fcaac <snipped> box.execute should be immutable function
>| $ ./tarantool -V
>| Tarantool 2.3.0-161-gec09fcaac
>| <snipped>
>| $ echo 'box.execute("SELECT 1")' | timeout -s 9 3 ./tarantool
>| Killed
>Thus box.execute call prior to box.cfg invokation hangs Tarantool after
>your patch. I'm totally not into SQL design but AFAIS such box.execute
>invokation ought to call loag_cfg underneath. However it doesn't since
>box.cfg is initialized with function several lines above, the check is
>failed and Tarantool goes into deep recursion.
>
>I guess you can introduce a local flag and use it as an upvalue in both
>box.execute and load_cfg functions. The flag is to be initialized to
>false, set in the load_cfg call and check within box.execute. However
>feel free to provide your own solution.
>
>> + load_cfg()
>> + end
>> return box.execute(...)
>> end
>>
>> diff --git a/test/box-tap/execute.test.lua b/test/box-tap/execute.test.lua
>> new file mode 100755
>> index 000000000..301cf4a1c
>> --- /dev/null
>> +++ b/test/box-tap/execute.test.lua
>> @@ -0,0 +1,17 @@
>> +#!/usr/bin/env tarantool
>> +
>> +local tap = require('tap')
>> +local test = tap.test('execute')
>> +test:plan(1)
>> +
>> +local box_execute = box.execute
>
>Please consider to add the check related to the flaw found above.
>
>> +box.cfg{}
>> +
>> +local status, err = pcall(
>> + function()
>> + box_execute("CREATE TABLE t1 (s1 INTEGER, PRIMARY KEY (s1));")
>
>Minor: A trailing whitespace is left above.
>
>> + end)
>
>There is a common code style for pcall an anonymous multiline Lua
>function within box-tap (see [1] and [2]). Please adjust yours
>considering it.
>
>> +
>> +test:ok(status and err == nil, "box.execute after box.cfg")
>
>Please consider the comment regarding the test finalization[3].
>
>> +test:check()
>> +os.exit(0)
>> --
>> 2.21.0 (Apple Git-122.2)
>>
>
>[1]:  https://github.com/tarantool/tarantool/blob/master/test/box-tap/cfg.test.lua#L148
>[2]:  https://github.com/tarantool/tarantool/blob/master/test/box-tap/merger.test.lua#L573
>[3]:  https://github.com/tarantool/tarantool/issues/4655#issue-529430689
>
>--
>Best regards,
>IM 
 
 
--
Maria Khaydich
 

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

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

[Igor Munkin]

Masha,

Thanks for the patch! It LGTM in general, but please consider a couple
polishing-aimed comments below.

On 24.12.19, Maria Khaydich wrote:
>    Igor, thank you for the review!
>    Proposed fixes are done:
> 

At first, please adjust the commit message subject considering our
contribution guide[1]. I see the component prefix is missing.

> 
>    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.
> 
>    The patch introduces a fix making box.execute method immutable.
> 
>    Closes #4231
>    ---
>    Issue:
>    https://github.com/tarantool/tarantool/issues/4231
>    Branch:
>    https://github.com/tarantool/tarantool/compare/eljashm/gh-4231-box.exec
>    ute-immutable-function
>     src/box/lua/load_cfg.lua                      | 15 +++++++++++-
>     .../gh-4231_immutable_box.execute.test.lua    | 24 +++++++++++++++++++
>     2 files changed, 38 insertions(+), 1 deletion(-)
>     create mode 100644 test/box-tap/gh-4231_immutable_box.execute.test.lua
>    diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
>    index e7f62cf4e..ad416e578 100644
>    --- a/src/box/lua/load_cfg.lua
>    +++ b/src/box/lua/load_cfg.lua
>    @@ -481,10 +481,14 @@ 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
>    @@ -528,7 +532,16 @@ box.cfg = locked(load_cfg)
>     -- metatable.
>     --
>     function box.execute(...)
>    -    load_cfg()
>    +    --
>    +    -- Calling box.execute before explicitly configuring box led to
>    +    -- automatic box.cfg invocation nonetheless. Following explicit
>    +    -- attempts to configure box led to its reconfiguratin and as a

Typo: s/reconfiguratin/reconfiguration/.

>    +    -- result - to an error when trying to use execute method again.
>    +    -- The check makes sure box.execute is an immutable function.

There is a trailing whitespace within this comment (I pulled your branch
and checked it). Please adjust it.

>    +    --
>    +    if not box_loaded then
>    +        load_cfg()
>    +    end
>         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 100644
>    index 000000000..e2469630e
>    --- /dev/null
>    +++ b/test/box-tap/gh-4231_immutable_box.execute.test.lua

As Nikita mentioned before (and we have the issue for it[2]), we're
going to use dashes instead of underscores. Please adjust the test chunk
name regarding this policy.

>    @@ -0,0 +1,24 @@
>    +#!/usr/bin/env tarantool
>    +local tap = require('tap')
>    +local test = tap.test('execute')
>    +test:plan(2)
>    +
>    +--
>    +--  gh-4231:  box.execute  should  be  immutable  function  meaning it
>    doesn't
>    +-- change after first box.cfg implicit invocation
>    +--
>    +
>    +local box_execute = box.execute
>    +local status, err = pcall(function()
>    +    box_execute("SELECT 1")
>    +end)
>    +test:ok(status  and  err  ==  nil, "box.execute does not work properly
>    before box.cfg")
>    +

load_cfg has been already called here, so you can check that box.execute
function prior to this call is the same as the one after it (as Nikita
suggested before).

>    +box.cfg{}
>    +
>    +local status2, err2 = pcall(function()
>    +    box_execute("CREATE TABLE t1 (s1 INTEGER, PRIMARY KEY (s1));")
>    +end)
>    +test:ok(status2 and err2 == nil, "box.execute is changed after box.cfg
>    invocation")

Minor: I guess you can move these lines to a separate function, since
they are quite similar to the ones prior to box.cfg call.

>    +
>    +os.exit(test:check() and 0 or 1)
>    --
>    2.24.0
> 

<snipped>

[1]: https://www.tarantool.io/en/doc/2.2/dev_guide/developer_guidelines/
[2]: https://github.com/tarantool/doc/issues/1004

-- 
Best regards,
IM

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

[Alexander Turenko]

>  function box.execute(...)
> -    load_cfg()
> +    --
> +    -- Calling box.execute before explicitly configuring box led to
> +    -- automatic box.cfg invocation nonetheless. Following explicit
> +    -- attempts to configure box led to its reconfiguratin and as a 
> +    -- result - to an error when trying to use execute method again. 
> +    -- The check makes sure box.execute is an immutable function.
> +    --

I would describe all this hell with registering lbox_execute as
box.execute, moving it to box_cfg_guard_whitelist, adding this
box.execute function, then replacing back with lbox_execute at
load_cfg().

This description would make clear why everything is fine if we just call
box.execute(), but extra flag is necessary if a user saved box.execute
before first box.execute() or box.cfg() call.

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

[Maria Khaydich]

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


Thank you for the review.
 
I’ve changed the comment describing box.execute method to explain why an extra flag is needed.
And also added polishing fixes proposed by Igor.
 
Result:
 
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.
 
The patch introduces a fix making box.execute immutable.
 
Closes #4231
---
Branch:
https://github.com/tarantool/tarantool/compare/eljashm/gh-4231-box.execute-immutable-function
Issue:
https://github.com/tarantool/tarantool/issues/4231

 src/box/lua/load_cfg.lua                      | 15 ++++++++-
 .../gh-4231-immutable-box-execute.test.lua    | 32 +++++++++++++++++++
 2 files changed, 46 insertions(+), 1 deletion(-)
 create mode 100755 test/box-tap/gh-4231-immutable-box-execute.test.lua
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
+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
@@ -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. The flag box_loaded makes sure execute
+    -- is an immutable function meaning it won't be overwritten by
+    -- any following attempts to reconfigure box.
+    --
+    if not box_loaded then
+        load_cfg()
+    end
     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..d8940a0b2
--- /dev/null
+++ b/test/box-tap/gh-4231-immutable-box-execute.test.lua
@@ -0,0 +1,32 @@
+#!/usr/bin/env tarantool
+local tap = require('tap')
+local test = tap.test('execute')
+test:plan(4)
+
+--
+-- gh-4231: box.execute should be immutable function meaning it's
+-- address doesn't change after first box.cfg implicit invocation
+--
+
+local function execute_is_immutable(execute, cmd, msg)
+    local status, err = pcall(function()
+        execute(cmd)
+    end)
+    test:ok(status and err == nil, msg)
+end
+
+local box_execute_stub = box.execute
+test:is(box_execute_stub, box.execute, "execute is not the same before box.cfg")
+execute_is_immutable(box_execute_stub, "SELECT 1",
+    "execute does not work properly before box.cfg")
+
+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
+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)
-- 
2.24.0  
>Четверг, 26 декабря 2019, 17:08 +03:00 от Alexander Turenko < alexander.turenko@tarantool.org >:
>  
>>  function box.execute(...)
>> -    load_cfg()
>> +    --
>> +    -- Calling box.execute before explicitly configuring box led to
>> +    -- automatic box.cfg invocation nonetheless. Following explicit
>> +    -- attempts to configure box led to its reconfiguratin and as a 
>> +    -- result - to an error when trying to use execute method again. 
>> +    -- The check makes sure box.execute is an immutable function.
>> +    --
>I would describe all this hell with registering lbox_execute as
>box.execute, moving it to box_cfg_guard_whitelist, adding this
>box.execute function, then replacing back with lbox_execute at
>load_cfg().
>
>This description would make clear why everything is fine if we just call
>box.execute(), but extra flag is necessary if a user saved box.execute
>before first box.execute() or box.cfg() call. 
 
 
--
Maria Khaydich
 

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

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

[Igor Munkin]

Masha,

Thanks, the patch LGTM, except a couple minor comments below. Please
consider them.

Sasha, please proceed with the patch.

On 13.01.20, Maria Khaydich wrote:
>    Thank you for the review.
> 
>    I’ve changed the comment describing box.execute method to explain why
>    an extra flag is needed.
>    And also added polishing fixes proposed by Igor.
> 
>    Result:
> 
>    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.
> 
>    The patch introduces a fix making box.execute immutable.
> 
>    Closes #4231
>    ---
>    Branch:
>    [1]https://github.com/tarantool/tarantool/compare/eljashm/gh-4231-box.e
>    xecute-immutable-function
>    Issue:
>    [2]https://github.com/tarantool/tarantool/issues/4231
>     src/box/lua/load_cfg.lua                      | 15 ++++++++-
>     .../gh-4231-immutable-box-execute.test.lua    | 32 +++++++++++++++++++
>     2 files changed, 46 insertions(+), 1 deletion(-)
>     create mode 100755 test/box-tap/gh-4231-immutable-box-execute.test.lua
>    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
>    +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
>    @@ -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. The flag box_loaded makes sure execute
>    +    -- is an immutable function meaning it won't be overwritten by
>    +    -- any following attempts to reconfigure box.
>    +    --
>    +    if not box_loaded then
>    +        load_cfg()
>    +    end
>         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..d8940a0b2
>    --- /dev/null
>    +++ b/test/box-tap/gh-4231-immutable-box-execute.test.lua
>    @@ -0,0 +1,32 @@
>    +#!/usr/bin/env tarantool
>    +local tap = require('tap')
>    +local test = tap.test('execute')
>    +test:plan(4)
>    +
>    +--
>    +-- gh-4231: box.execute should be immutable function meaning it's
>    +-- address doesn't change after first box.cfg implicit invocation
>    +--
>    +
>    +local function execute_is_immutable(execute, cmd, msg)
>    +    local status, err = pcall(function()
>    +        execute(cmd)
>    +    end)
>    +    test:ok(status and err == nil, msg)
>    +end
>    +
>    +local box_execute_stub = box.execute
>    +test:is(box_execute_stub, box.execute, "execute is not the same before
>    box.cfg")

Minor: this test is a trivial and excess one. I suggest to drop it.

>    +execute_is_immutable(box_execute_stub, "SELECT 1",
>    +    "execute does not work properly before box.cfg")
>    +
>    +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
>    +test:is(box_execute_actual,  box.execute,  "execute  is  not  the same
>    after box.cfg")

Minor: out of 80 chars

>    +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)
>    --
>    2.24.0
> 
>      Четверг, 26 декабря 2019, 17:08 +03:00 от Alexander Turenko
>      <alexander.turenko@tarantool.org>:
> 
>    >  function box.execute(...)
>    > -    load_cfg()
>    > +    --
>    > +    -- Calling box.execute before explicitly configuring box led to
>    > +    -- automatic box.cfg invocation nonetheless. Following explicit
>    > +    -- attempts to configure box led to its reconfiguratin and as a
>    >  +      --  result  -  to  an error when trying to use execute method
>    again.
>    > +    -- The check makes sure box.execute is an immutable function.
>    > +    --
>    I would describe all this hell with registering lbox_execute as
>    box.execute, moving it to box_cfg_guard_whitelist, adding this
>    box.execute function, then replacing back with lbox_execute at
>    load_cfg().
>    This  description  would  make  clear why everything is fine if we just
>    call
>    box.execute(), but extra flag is necessary if a user saved box.execute
>    before first box.execute() or box.cfg() call.
> 
> 
> 
>    --
>    Maria Khaydich
> 
> References
> 
>    1. https://github.com/tarantool/tarantool/compare/eljashm/gh-4231-box.execute-immutable-function
>    2. https://github.com/tarantool/tarantool/issues/4231

-- 
Best regards,
IM

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

[Maria Khaydich]

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


Fixed minor comments.
 
Sasha, you can proceed with the review. 
  
>Понедельник, 13 января 2020, 18:50 +03:00 от Igor Munkin <imun@tarantool.org>:
> 
>Masha,
>
>Thanks, the patch LGTM, except a couple minor comments below. Please
>consider them.
>
>Sasha, please proceed with the patch.
>
>On 13.01.20, Maria Khaydich wrote:
>> Thank you for the review.
>>
>> I’ve changed the comment describing box.execute method to explain why
>> an extra flag is needed.
>> And also added polishing fixes proposed by Igor.
>>
>> Result:
>>
>> 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.
>>
>> The patch introduces a fix making box.execute immutable.
>>
>> Closes #4231
>> ---
>> Branch:
>> [1] https://github.com/tarantool/tarantool/compare/eljashm/gh-4231-box.e
>> xecute-immutable-function
>> Issue:
>> [2] https://github.com/tarantool/tarantool/issues/4231
>> src/box/lua/load_cfg.lua | 15 ++++++++-
>> .../gh-4231-immutable-box-execute.test.lua | 32 +++++++++++++++++++
>> 2 files changed, 46 insertions(+), 1 deletion(-)
>> create mode 100755 test/box-tap/gh-4231-immutable-box-execute.test.lua
>> 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
>> +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
>> @@ -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. The flag box_loaded makes sure execute
>> + -- is an immutable function meaning it won't be overwritten by
>> + -- any following attempts to reconfigure box.
>> + --
>> + if not box_loaded then
>> + load_cfg()
>> + end
>> 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..d8940a0b2
>> --- /dev/null
>> +++ b/test/box-tap/gh-4231-immutable-box-execute.test.lua
>> @@ -0,0 +1,32 @@
>> +#!/usr/bin/env tarantool
>> +local tap = require('tap')
>> +local test = tap.test('execute')
>> +test:plan(4)
>> +
>> +--
>> +-- gh-4231: box.execute should be immutable function meaning it's
>> +-- address doesn't change after first box.cfg implicit invocation
>> +--
>> +
>> +local function execute_is_immutable(execute, cmd, msg)
>> + local status, err = pcall(function()
>> + execute(cmd)
>> + end)
>> + test:ok(status and err == nil, msg)
>> +end
>> +
>> +local box_execute_stub = box.execute
>> +test:is(box_execute_stub, box.execute, "execute is not the same before
>> box.cfg")
>
>Minor: this test is a trivial and excess one. I suggest to drop it.
>
>> +execute_is_immutable(box_execute_stub, "SELECT 1",
>> + "execute does not work properly before box.cfg")
>> +
>> +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
>> +test:is(box_execute_actual, box.execute, "execute is not the same
>> after box.cfg")
>
>Minor: out of 80 chars
>
>> +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)
>> --
>> 2.24.0
>>
>> Четверг, 26 декабря 2019, 17:08 +03:00 от Alexander Turenko
>> < alexander.turenko@tarantool.org >:
>>
>> > function box.execute(...)
>> > - load_cfg()
>> > + --
>> > + -- Calling box.execute before explicitly configuring box led to
>> > + -- automatic box.cfg invocation nonetheless. Following explicit
>> > + -- attempts to configure box led to its reconfiguratin and as a
>> > + -- result - to an error when trying to use execute method
>> again.
>> > + -- The check makes sure box.execute is an immutable function.
>> > + --
>> I would describe all this hell with registering lbox_execute as
>> box.execute, moving it to box_cfg_guard_whitelist, adding this
>> box.execute function, then replacing back with lbox_execute at
>> load_cfg().
>> This description would make clear why everything is fine if we just
>> call
>> box.execute(), but extra flag is necessary if a user saved box.execute
>> before first box.execute() or box.cfg() call.
>>
>>
>>
>> --
>> Maria Khaydich
>>
>> References
>>
>> 1.  https://github.com/tarantool/tarantool/compare/eljashm/gh-4231-box.execute-immutable-function
>> 2.  https://github.com/tarantool/tarantool/issues/4231
>
>--
>Best regards,
>IM 
 
 
--
Maria Khaydich
 

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

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

[Alexander Turenko]

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

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

[Igor Munkin]

Sasha,

On 20.02.20, Alexander Turenko wrote:
> 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.
> 

<snipped>

> 
> 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:
> 

Nice. I overlooked it, but you found such a great solution here.

>  | local ffi = require('ffi')
>  |
>  | ffi.cdef([[
>  |     bool
>  |     box_is_configured(void);
>  | ]])
>  |
>  | local function box_is_configured()
>  |     return ffi.C.box_is_configured()
>  | end
> 

<snipped>

-- 
Best regards,
IM

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

[Maria Khaydich]

[-- 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 --]

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

[Maria Khaydich]

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


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@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: 19132 bytes --]

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

[Konstantin Osipov]

* Maria Khaydich <maria.khaydich@tarantool.org> [20/03/11 19:02]:
> 
> 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.

Did you benchmark it?

>  
> 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  

-- 
Konstantin Osipov, Moscow, Russia

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

[Maria Khaydich]

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


  
>Четверг, 12 марта 2020, 16:29 +03:00 от Konstantin Osipov <kostja.osipov@gmail.com>:
> 
>* Maria Khaydich < maria.khaydich@tarantool.org > [20/03/11 19:02]:
>>
>> 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.
>
>Did you benchmark it?
 
Do you think we need to? There’s basically one extra condition in the patch. I don’t see how it might degrade performance.
 
>
>>  
>> 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  
>
>--
>Konstantin Osipov, Moscow, Russia 
 
 
--
Maria Khaydich
 

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

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

[Konstantin Osipov]

* Maria Khaydich <maria.khaydich@tarantool.org> [20/03/12 22:26]:
> >> 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.
> >
> >Did you benchmark it?
>  
> Do you think we need to? There’s basically one extra condition
> in the patch. I don’t see how it might degrade performance.

It's not a one more condition, it's one more FFI C call.

-- 
Konstantin Osipov, Moscow, Russia

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

[Igor Munkin]

Masha,

Thanks for the patch! I left a couple of comments below, please consider
them.

Side note: Sasha, there is much work done to solve the issue but we have
a ticket[1] requesting to drop the "feature". Could you please write a
rationale to proceed with this fix, instead of removing the implicit
box.cfg call from of box.execute?

On 11.03.20, Maria Khaydich wrote:
> 
> 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

<snipped>

> 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

<snipped>

> @@ -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

It's better to use a sole function here instead of creating a new one
for every call.

> +        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

Minor: test chunk name is left unchanged since the previous version and
doesn't respect the commit message wording.

> 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)

Typo: s/immutable/idempotent/.

> +    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 

<snipped>

>  
>  
> --
> Maria Khaydich
>  

[1]: https://github.com/tarantool/tarantool/issues/4726

-- 
Best regards,
IM

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

[Igor Munkin]

Masha,

Thanks for the patch! Please consider my comments below.

On 11.03.20, Maria Khaydich wrote:
> 
> 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
> +

Minor: Since box_is_configured is introduced within this patchset it
could be placed properly in the first patch of the series. Feel free to
ignore.

>  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);

<snipped>

> diff --git a/test/box-tap/gh-4231-immutable-box-execute.test.lua b/test/box-tap/gh-4231-immutable-box-execute.test.lua

Minor: test chunk name is left unchanged since the previous version and
doesn't respect the commit message wording.

> 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)

Minor: IMHO these changes are better to be moved to the first patch of
the series. Feel free to ignore.

> +    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   

<snipped>

>  
>  
> --
> Maria Khaydich
>  

-- 
Best regards,
IM

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

[Igor Munkin]

Kostja,

On 12.03.20, Konstantin Osipov wrote:
> * Maria Khaydich <maria.khaydich@tarantool.org> [20/03/12 22:26]:
> > >> 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.
> > >
> > >Did you benchmark it?
> >  
> > Do you think we need to? There’s basically one extra condition
> > in the patch. I don’t see how it might degrade performance.
> 
> It's not a one more condition, it's one more FFI C call.

I guess the problem have to be fixed anyway.

However you might suggest another fix for the issue? There are several
other ways to indicate whether box is configured, e.g. introduce the
specific value to the box table. What do you think?

I see for now box.cfg call as not the one performance critical, but I
might be missing something you see.

It would be great if you detailed a bit your proposal regarding the fix
and its benchmarks.

> 
> -- 
> Konstantin Osipov, Moscow, Russia

-- 
Best regards,
IM

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

[Konstantin Osipov]

* Igor Munkin <imun@tarantool.org> [20/03/19 10:08]:
> I guess the problem have to be fixed anyway.
> 
> However you might suggest another fix for the issue? There are several
> other ways to indicate whether box is configured, e.g. introduce the
> specific value to the box table. What do you think?

Why not set a lua variable *from* C instead of calling from Lua
*into* C each time?

I mean, this is an obvious optimization, but it is only worth it
if there is a measurable slowdown (which I suspect there is, at
least a couple of %, but even a couple of % IMHO justify it).

> I see for now box.cfg call as not the one performance critical, but I
> might be missing something you see.
> 
> It would be great if you detailed a bit your proposal regarding the fix
> and its benchmarks.

-- 
Konstantin Osipov, Moscow, Russia

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

[Igor Munkin]

Kostja,

On 19.03.20, Konstantin Osipov wrote:
> * Igor Munkin <imun@tarantool.org> [20/03/19 10:08]:
> > I guess the problem have to be fixed anyway.
> > 
> > However you might suggest another fix for the issue? There are several
> > other ways to indicate whether box is configured, e.g. introduce the
> > specific value to the box table. What do you think?
> 
> Why not set a lua variable *from* C instead of calling from Lua
> *into* C each time?

I guess we talk about the same thing.

> 
> I mean, this is an obvious optimization, but it is only worth it
> if there is a measurable slowdown (which I suspect there is, at
> least a couple of %, but even a couple of % IMHO justify it).

I'm OK with your proposal (but I still don't see the box.cfg as a
performance bottleneck).

Sasha, any thoughts?

> 
> > I see for now box.cfg call as not the one performance critical, but I
> > might be missing something you see.
> > 
> > It would be great if you detailed a bit your proposal regarding the fix
> > and its benchmarks.
> 
> -- 
> Konstantin Osipov, Moscow, Russia

-- 
Best regards,
IM

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

[Konstantin Osipov]

* Igor Munkin <imun@tarantool.org> [20/03/19 12:17]:
> > I mean, this is an obvious optimization, but it is only worth it
> > if there is a measurable slowdown (which I suspect there is, at
> > least a couple of %, but even a couple of % IMHO justify it).
> 
> I'm OK with your proposal (but I still don't see the box.cfg as a
> performance bottleneck).
> 
> Sasha, any thoughts?

box.cfg{} is *not* the bottleneck.
box.execute() is, it is having a call to ffi c function now.


-- 
Konstantin Osipov, Moscow, Russia

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

[Igor Munkin]

Kostja,

On 19.03.20, Konstantin Osipov wrote:
> * Igor Munkin <imun@tarantool.org> [20/03/19 12:17]:
> > > I mean, this is an obvious optimization, but it is only worth it
> > > if there is a measurable slowdown (which I suspect there is, at
> > > least a couple of %, but even a couple of % IMHO justify it).
> > 
> > I'm OK with your proposal (but I still don't see the box.cfg as a
> > performance bottleneck).
> > 
> > Sasha, any thoughts?
> 
> box.cfg{} is *not* the bottleneck.
> box.execute() is, it is having a call to ffi c function now.

I was misled, since you commented the patch with box.cfg related
changes. Now the performance issue is clear, thanks.

> 
> 
> -- 
> Konstantin Osipov, Moscow, Russia

-- 
Best regards,
IM

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

[Alexander Turenko]

> Side note: Sasha, there is much work done to solve the issue but we have
> a ticket[1] requesting to drop the "feature". Could you please write a
> rationale to proceed with this fix, instead of removing the implicit
> box.cfg call from of box.execute?
> 
> [1]: https://github.com/tarantool/tarantool/issues/4726

There is no decision about #4726 and now we have the flaw in the
feature: it should be fixed so.

WBR, Alexander Turenko.

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

[Alexander Turenko]

> > However you might suggest another fix for the issue? There are several
> > other ways to indicate whether box is configured, e.g. introduce the
> > specific value to the box table. What do you think?
> 
> Why not set a lua variable *from* C instead of calling from Lua
> *into* C each time?

This way it will be slower when called from C. Since we unable to
'unconfigure' box I would just cache C's value in Lua.

C's box_is_configured() is used only in box.session.su() and its
performance is not so critical as box.execute(). However I don't see a
reason to slowdown C's box_is_configured()() if we can avoid it: it may
be important if it will be called from some other code.

I don't sure, but storing database settings and state in Lua looks a bit
lopsided approach for me: even now we have three languages: C, Lua and
SQL. Some caching, hovewer, is okay.

> I mean, this is an obvious optimization, but it is only worth it
> if there is a measurable slowdown (which I suspect there is, at
> least a couple of %, but even a couple of % IMHO justify it).

I failed to obtain stable results and maybe there is a difference, but I
don't see it now.

https://gist.github.com/Totktonada/407855389ed4da93bf0175cf8a11c7b9

I made those measurements on fixed CPU frequency and 10 times run each
implementation 10 times: 300 runs at whole. I see that results becomes
worse for all implementations from run to run. Looks strange.

WBR, Alexander Turenko.

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

[Konstantin Osipov]

* Alexander Turenko <alexander.turenko@tarantool.org> [20/05/06 14:17]:
> https://gist.github.com/Totktonada/407855389ed4da93bf0175cf8a11c7b9
> 
> I made those measurements on fixed CPU frequency and 10 times run each
> implementation 10 times: 300 runs at whole. I see that results becomes
> worse for all implementations from run to run. Looks strange.

Try with jit off?


-- 
Konstantin Osipov, Moscow, Russia

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

[Alexander Turenko]

On Wed, May 06, 2020 at 02:49:21PM +0300, Konstantin Osipov wrote:
> * Alexander Turenko <alexander.turenko@tarantool.org> [20/05/06 14:17]:
> > https://gist.github.com/Totktonada/407855389ed4da93bf0175cf8a11c7b9
> > 
> > I made those measurements on fixed CPU frequency and 10 times run each
> > implementation 10 times: 300 runs at whole. I see that results becomes
> > worse for all implementations from run to run. Looks strange.
> 
> Try with jit off?

Are there any production setup with jit.off()? This measurement would be
artificial I think.

Anyway, I collected the results and they are quite noisy again (~10%
variation). I would not say anything based on them. But in average ffi
call gives -0.5% (worse) and ffi call with caching -1.0% from the
baseline (master). Maybe it is just noise.

WBR, Alexander Turenko.

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

[Konstantin Osipov]

* Alexander Turenko <alexander.turenko@tarantool.org> [20/05/06 15:58]:
> On Wed, May 06, 2020 at 02:49:21PM +0300, Konstantin Osipov wrote:
> > * Alexander Turenko <alexander.turenko@tarantool.org> [20/05/06 14:17]:
> > > https://gist.github.com/Totktonada/407855389ed4da93bf0175cf8a11c7b9
> > > 
> > > I made those measurements on fixed CPU frequency and 10 times run each
> > > implementation 10 times: 300 runs at whole. I see that results becomes
> > > worse for all implementations from run to run. Looks strange.
> > 
> > Try with jit off?
> 
> Are there any production setup with jit.off()? This measurement would be
> artificial I think.

Why do you think you can assume that this trace will be jited in production?

> Anyway, I collected the results and they are quite noisy again (~10%
> variation). I would not say anything based on them. But in average ffi
> call gives -0.5% (worse) and ffi call with caching -1.0% from the
> baseline (master). Maybe it is just noise.
> 
> WBR, Alexander Turenko.

-- 
Konstantin Osipov, Moscow, Russia
https://scylladb.com

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

[Alexander Turenko]

On Wed, May 06, 2020 at 04:02:09PM +0300, Konstantin Osipov wrote:
> * Alexander Turenko <alexander.turenko@tarantool.org> [20/05/06 15:58]:
> > On Wed, May 06, 2020 at 02:49:21PM +0300, Konstantin Osipov wrote:
> > > * Alexander Turenko <alexander.turenko@tarantool.org> [20/05/06 14:17]:
> > > > https://gist.github.com/Totktonada/407855389ed4da93bf0175cf8a11c7b9
> > > > 
> > > > I made those measurements on fixed CPU frequency and 10 times run each
> > > > implementation 10 times: 300 runs at whole. I see that results becomes
> > > > worse for all implementations from run to run. Looks strange.
> > > 
> > > Try with jit off?
> > 
> > Are there any production setup with jit.off()? This measurement would be
> > artificial I think.
> 
> Why do you think you can assume that this trace will be jited in production?

Okay, good point.

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

[Alexander Turenko]

> >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

I was not right here: `type(box.cfg)` is changed only at successful box
configuration, however it is performed before full box loading that is
not suitable for box.execute().

We can set a module local `box_is_configured` variable at end of box
configuration to eliminate the ffi call. I'll update and resend the
patchset soon.

> >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

I'll add such test case.

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

[Alexander Turenko]

I updated the patchset and placed it on the
Totktonada/gh-4231-box-execute-idempotence branch. I'll send it soon.

Aside of changes around Igor's review I made the following changes:

* Rewrote w/o ffi call, using a module local variable.
* Fixed <load_cfg> to actually reconfigure box
  (reload_cfg(cfg) -> reload_cfg(box.cfg, cfg)).
* Replace box.execute with lbox_execute at end of box loading.
* Added a test for <box_load_and_execute>() and box.execute() at box
  loading (in parallel in separate fibers).
* Polished tests: remove execute_is_immutable() function, splitted
  box.execute and box.cfg tests.
* Clarified dynamic box option set functions contract.

> > +local function box_is_configured()
> > +    return ffi.C.box_is_configured()
> > +end
> > +
> 
> Minor: Since box_is_configured is introduced within this patchset it
> could be placed properly in the first patch of the series. Feel free to
> ignore.

Fixed.

> > diff --git a/test/box-tap/gh-4231-immutable-box-execute.test.lua b/test/box-tap/gh-4231-immutable-box-execute.test.lua
> 
> Minor: test chunk name is left unchanged since the previous version and
> doesn't respect the commit message wording.

Renamed to gh-4231-box-execute-idempotence.test.lua.

> > --- 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)
> 
> Minor: IMHO these changes are better to be moved to the first patch of
> the series. Feel free to ignore.

Fixed.