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