From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 6710F469710 for ; Thu, 4 Jun 2020 01:07:43 +0300 (MSK) Date: Thu, 4 Jun 2020 00:58:35 +0300 From: Igor Munkin Message-ID: <20200603215835.GF5745@tarantool.org> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH 1/3] box: check whether box is loaded in box.execute() List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Turenko Cc: Vladislav Shpilevoy , tarantool-patches@dev.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 > > box.execute() initializes box if it is not initialized. For this sake, > box.execute() is another function (so called ) > when box is not loaded: it loads box and calls 'real' box.execute(). > However it is not enough: may be saved by a user > before box initialization and called after box loading or during box > loading from a separate fiber. > > Note: calling 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 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 > . > > While we're here, clarified contracts of functions that set box > configuration options. > > Closes #4231 > > Co-developed-by: Alexander Turenko > --- > 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 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 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 > +-- variable. > +-- > +-- box.execute is when box is not loaded, > +-- otherwise. loads box and > +-- calls . > -- > -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 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 > -- > 2.25.0 > [1]: https://github.com/tarantool/tarantool/issues/4726#issuecomment-575344974 -- Best regards, IM