Re: [Tarantool-patches] [PATCH 1/3] box: check whether box is loaded in box.execute()

[Igor Munkin <imun@tarantool.org>]

Sasha,

Thanks for your patch! I read the cover letter too, but decided to group
all my comments regarding this patch below.

No doubt, it is a bug to be fixed in the nearest stable release (2.3.3)
and we need to fix it despite other issues. Furthermore, the patch
doesn't break existing behaviour, so nothing is changed in "user space".

But it simply looks irrational to me. All these changes with a pile of
clarifying comments alongside only confirm once again, that this
solution is an overkill and too fragile for this dubious feature. We
have already discussed how to call box.cfg prior to box.execute here[1],
but discussion has been stalled and nobody has made the final decision
regarding implicit box.cfg. Unfortunately #4726 is in wishlist now and
seems to have a little prio for being done (or even discussed) in this
release.

Hence, we can either close two issues with one simple shot or introduce
complex and fragile (but thanks to you well-described) behaviour that
might be dropped in the nearest future.

The patch itself is great, except the several cosmetic nits I left
below. I also Cced Vlad to take a look on the changes.

On 13.05.20, Alexander Turenko wrote:
> From: Maria <maria.khaydich@tarantool.org>
> 
> box.execute() initializes box if it is not initialized. For this sake,
> box.execute() is another function (so called <box_load_and_execute>)
> when box is not loaded: it loads box and calls 'real' box.execute().
> However it is not enough: <box_load_and_execute> may be saved by a user
> before box initialization and called after box loading or during box
> loading from a separate fiber.
> 
> Note: calling <box_load_and_execute> during box loading is safe now, but
> calling of box.execute() is not: the 'real' box.execute() does not
> verify whether box is configured. It will be fixed in a further commit.
> 
> This commit changes <box_load_and_execute> to verify whether box is
> initialized and to load box only when it is not loaded. Also it adds
> appropriate locking around load_cfg() invocation from
> <box_load_and_execute>.
> 
> While we're here, clarified contracts of functions that set box
> configuration options.
> 
> Closes #4231
> 
> Co-developed-by: Alexander Turenko <alexander.turenko@tarantool.org>
> ---
>  src/box/lua/load_cfg.lua                      | 69 +++++++++++++++++--
>  .../gh-4231-box-execute-idempotence.test.lua  | 37 ++++++++++
>  .../gh-4231-box-execute-locking.test.lua      | 46 +++++++++++++
>  3 files changed, 148 insertions(+), 4 deletions(-)
>  create mode 100755 test/box-tap/gh-4231-box-execute-idempotence.test.lua
>  create mode 100755 test/box-tap/gh-4231-box-execute-locking.test.lua
> 
> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
> index 5d818addf..0b4a14377 100644
> --- a/src/box/lua/load_cfg.lua
> +++ b/src/box/lua/load_cfg.lua
> @@ -230,6 +230,11 @@ local function check_replicaset_uuid()
>  end
>  
>  -- dynamically settable options
> +--
> +-- Note: An option should be in `dynamic_cfg_skip_at_load` table

Typo: everywhere below you use <var> instead of `var`.

> +-- or should be verified in box_check_config(). Otherwise
> +-- load_cfg() may report an error, but box will be configured in
> +-- fact.
>  local dynamic_cfg = {
>      listen                  = private.cfg_set_listen,
>      replication             = private.cfg_set_replication,
> @@ -550,6 +555,16 @@ setmetatable(box, {
>       end
>  })
>  
> +-- Whether box is loaded.
> +--
> +-- `false` when box is not configured or when the initialization
> +-- is in progress.
> +--
> +-- `true` when box is configured.
> +--
> +-- Use locked() wrapper to obtain reliable results.
> +local box_is_configured = false

Minor: This name is such close to the corresponding variable in box.cc
(is_box_configured) ergo a bit misleading. Why they can't be the same?

> +
>  local function load_cfg(cfg)
>      cfg = upgrade_cfg(cfg, translate_cfg)
>      cfg = prepare_cfg(cfg, default_cfg, template_cfg, modify_cfg)
> @@ -560,6 +575,16 @@ local function load_cfg(cfg)
>          box.cfg = locked(load_cfg) -- restore original box.cfg
>          return box.error() -- re-throw exception from check_cfg()
>      end
> +
> +    -- NB: After this point the function should not raise an
> +    -- error.
> +    --
> +    -- This is important to have right `box_is_configured` (this
> +    -- file) and `is_box_configured` (box.cc) values.
> +    --
> +    -- It also would be counter-intuitive to receive an error from
> +    -- box.cfg({<...>}), but find that box is actually configured.
> +
>      -- Restore box members after initial configuration
>      for k, v in pairs(box_configured) do
>          box[k] = v
> @@ -573,7 +598,13 @@ local function load_cfg(cfg)
>              end,
>              __call = locked(reload_cfg),
>          })
> +
> +    -- This call either succeeds or calls to panic() / exit().

Typo: s/calls to/calls/.

>      private.cfg_load()
> +
> +    -- This block does not raise an error: all necessary checks
> +    -- already performed in private.cfg_check(). See `dynamic_cfg`

Typo: everywhere below you use <var> instead of `var`

> +    -- comment.
>      for key, fun in pairs(dynamic_cfg) do
>          local val = cfg[key]
>          if val ~= nil then
> @@ -588,21 +619,51 @@ local function load_cfg(cfg)
>              end
>          end
>      end
> +

Typo: excess whitespace change.

>      if not box.cfg.read_only and not box.cfg.replication then
>          box.schema.upgrade{auto = true}
>      end
> +
> +    box_is_configured = true
>  end
>  box.cfg = locked(load_cfg)
>  
>  --
>  -- This makes possible do box.execute without calling box.cfg
> --- manually. The load_cfg call overwrites following table and
> --- metatable.
> +-- manually. The load_cfg() call overwrites box.execute, see
> +-- <box_configured> variable.
> +--
> +-- box.execute is <box_load_and_execute> when box is not loaded,
> +-- <lbox_execute> otherwise. <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

Typo: s/not/no/.

> +    -- 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
> +        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()

Minor: I guess assertions below insist on the fact box *must* be
configured at this point :)

> +    -- 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

<snipped>

> -- 
> 2.25.0
> 

[1]: https://github.com/tarantool/tarantool/issues/4726#issuecomment-575344974

-- 
Best regards,
IM