Masha,
Thanks, the patch LGTM, except a couple minor comments below. Please
consider them.
Sasha, please proceed with the patch.
On 13.01.20, Maria Khaydich wrote:
> Thank you for the review.
>
> I’ve changed the comment describing box.execute method to explain why
> an extra flag is needed.
> And also added polishing fixes proposed by Igor.
>
> Result:
>
> 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 immutable.
>
> Closes #4231
> ---
> Branch:
> [1]
https://github.com/tarantool/tarantool/compare/eljashm/gh-4231-box.e
> xecute-immutable-function
> Issue:
> [2]
https://github.com/tarantool/tarantool/issues/4231
> src/box/lua/load_cfg.lua | 15 ++++++++-
> .../gh-4231-immutable-box-execute.test.lua | 32 +++++++++++++++++++
> 2 files changed, 46 insertions(+), 1 deletion(-)
> create mode 100755 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 85617c8f0..1f9b4392f 100644
> --- a/src/box/lua/load_cfg.lua
> +++ b/src/box/lua/load_cfg.lua
> @@ -517,10 +517,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
> @@ -564,7 +568,16 @@ box.cfg = locked(load_cfg)
> -- metatable.
> --
> function box.execute(...)
> - load_cfg()
> + --
> + -- Saving box.execute method before explicit calls to either
> + -- box.execute() or box.cfg{} leads to implicit invocation of
> + -- box.cfg nonetheless. The flag box_loaded makes sure execute
> + -- is an immutable function meaning it won't be overwritten by
> + -- any following attempts to reconfigure box.
> + --
> + 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 100755
> index 000000000..d8940a0b2
> --- /dev/null
> +++ b/test/box-tap/gh-4231-immutable-box-execute.test.lua
> @@ -0,0 +1,32 @@
> +#!/usr/bin/env tarantool
> +local tap = require('tap')
> +local test = tap.test('execute')
> +test:plan(4)
> +
> +--
> +-- gh-4231: box.execute should be immutable function meaning it's
> +-- address doesn't change after first box.cfg implicit invocation
> +--
> +
> +local function execute_is_immutable(execute, cmd, msg)
> + local status, err = pcall(function()
> + execute(cmd)
> + end)
> + test:ok(status and err == nil, msg)
> +end
> +
> +local box_execute_stub = box.execute
> +test:is(box_execute_stub, box.execute, "execute is not the same before
> box.cfg")
Minor: this test is a trivial and excess one. I suggest to drop it.
> +execute_is_immutable(box_execute_stub, "SELECT 1",
> + "execute does not work properly before box.cfg")
> +
> +local box_execute_actual = box.execute
> +-- explicit call to load_cfg
> +box.cfg{}
> +-- checking the function was not reconfigured, i.e. adress stays the
> same
> +test:is(box_execute_actual, box.execute, "execute is not the same
> after box.cfg")
Minor: out of 80 chars
> +execute_is_immutable(box_execute_actual,
> + "CREATE TABLE t1 (s1 INTEGER, PRIMARY KEY (s1));",
> + "execute does not work properly after box.cfg")
> +
> +os.exit(test:check() and 0 or 1)
> --
> 2.24.0
>
> Четверг, 26 декабря 2019, 17:08 +03:00 от Alexander Turenko
> <
alexander.turenko@tarantool.org>:
>
> > 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.
> > + --
> I would describe all this hell with registering lbox_execute as
> box.execute, moving it to box_cfg_guard_whitelist, adding this
> box.execute function, then replacing back with lbox_execute at
> load_cfg().
> This description would make clear why everything is fine if we just
> call
> box.execute(), but extra flag is necessary if a user saved box.execute
> before first box.execute() or box.cfg() call.
>
>
>
> --
> Maria Khaydich
>
> References
>
> 1.
https://github.com/tarantool/tarantool/compare/eljashm/gh-4231-box.execute-immutable-function
> 2.
https://github.com/tarantool/tarantool/issues/4231
--
Best regards,
IM