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

[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