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

[Igor Munkin]

Masha,

Thanks for the patch! I left a couple of comments below, please consider
them.

Side note: Sasha, there is much work done to solve the issue but we have
a ticket[1] requesting to drop the "feature". Could you please write a
rationale to proceed with this fix, instead of removing the implicit
box.cfg call from of box.execute?

On 11.03.20, Maria Khaydich wrote:
> 
> Thank you for the review! I reworked the patch:
> ----------------------------------------------------------------------
> Saving box.execute method before explicitly configuring box
> automatically invokes load_cfg() nonetheless. Further calls
> to box.cfg{} caused its reconfiguration and replacement of
> previously saved box.execute meaning it couldn't be used
> again without leading to an error.
>  
> The patch introduces a workaround making box.execute idempotent
> and is heavily influenced by @Totktonada.
>  
> Closes #4231
> ---
> Issue:
> https://github.com/tarantool/tarantool/issues/4231  
> Branch:
> https://github.com/tarantool/tarantool/compare/eljashm/gh-4231-box.execute-immutable-function  
>  
>  extra/exports                                 |  2 +
>  src/box/lua/load_cfg.lua                      | 42 ++++++++++++++++---
>  .../gh-4231-immutable-box-execute.test.lua    | 28 +++++++++++++
>  3 files changed, 67 insertions(+), 5 deletions(-)
>  create mode 100755 test/box-tap/gh-4231-immutable-box-execute.test.lua

<snipped>

> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
> index 85617c8f0..dc4293bdd 100644
> --- a/src/box/lua/load_cfg.lua
> +++ b/src/box/lua/load_cfg.lua

<snipped>

> @@ -558,15 +564,41 @@ local function load_cfg(cfg)
>  end
>  box.cfg = locked(load_cfg)
>  
> +local function box_is_configured()
> +    return ffi.C.box_is_configured()
> +end
> +
>  --
> --- This makes possible do box.execute without calling box.cfg
> --- manually. The load_cfg call overwrites following table and
> --- metatable.
> +-- box.execute is <box_load_and_execute> when box is not loaded,
> +-- <lbox_execute> otherwise. See also <box_configured> variable.
> +-- <box_load_and_execute> loads box and calls <lbox_execute>.
>  --
> -function box.execute(...)
> -    load_cfg()
> +local box_load_and_execute
> +box_load_and_execute = function(...)
> +    -- Configure box if it is not configured, no-op otherwise (not
> +    -- reconfiguration).
> +    -- We should check whether box is configured, because a user
> +    -- may save <box_load_and_execute> function before box loading
> +    -- and call it afterwards.
> +    if not box_is_configured() then

It's better to use a sole function here instead of creating a new one
for every call.

> +        locked(function()
> +            -- Re-check, because after releasing of the lock the
> +            -- box state may be changed. We should call load_cfg()
> +            -- only once.
> +            if not box_is_configured() then
> +                load_cfg()
> +            end
> +        end)()
> +    end
> +
> +    -- At this point box should be configured and box.execute()
> +    -- function should be replaced with lbox_execute().
> +    assert(type(box.cfg) ~= 'function')
> +    assert(box.execute ~= box_load_and_execute)
> +
>      return box.execute(...)
>  end
> +box.execute = box_load_and_execute
>  
>  -- gh-810:
>  -- hack luajit default cpath
> diff --git a/test/box-tap/gh-4231-immutable-box-execute.test.lua b/test/box-tap/gh-4231-immutable-box-execute.test.lua

Minor: test chunk name is left unchanged since the previous version and
doesn't respect the commit message wording.

> new file mode 100755
> index 000000000..1544591bf
> --- /dev/null
> +++ b/test/box-tap/gh-4231-immutable-box-execute.test.lua
> @@ -0,0 +1,28 @@
> +#!/usr/bin/env tarantool
> +local tap = require('tap')
> +local test = tap.test('execute')
> +test:plan(2)
> +
> +--
> +-- gh-4231: box.execute should be an idempotent function
> +-- meaning its effect should be the same if a user chooses
> +-- to use it before explicit box.cfg invocation
> +--
> +
> +local function execute_is_immutable(execute, cmd, msg)

Typo: s/immutable/idempotent/.

> +    local status, err = pcall(execute, cmd)
> +    test:ok(status and type(err) == 'table', msg)
> +end
> +
> +local box_execute_stub = box.execute
> +-- explicit call to load_cfg
> +box.cfg{}
> +local box_execute_actual = box.execute
> +
> +execute_is_immutable(box_execute_stub,
> +    "CREATE TABLE t1 (s1 INTEGER, PRIMARY KEY (s1));",
> +    "box.execute stub works before box.cfg")
> +execute_is_immutable(box_execute_actual, "DROP TABLE t1",
> +    "box.execute works properly after box.cfg")
> +
> +os.exit(test:check() and 0 or 1)
> -- 
> 2.24.0 

<snipped>

>  
>  
> --
> Maria Khaydich
>  

[1]: https://github.com/tarantool/tarantool/issues/4726

-- 
Best regards,
IM