[Tarantool-patches] [PATCH] box.execute should be immutable function
Igor Munkin
imun at tarantool.org
Tue Dec 17 17:39:40 MSK 2019
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
More information about the Tarantool-patches
mailing list