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

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

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