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