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

[Alexander Turenko]

From: Maria <maria.khaydich at 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 at 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
+-- 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
+
 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().
     private.cfg_load()
+
+    -- This block does not raise an error: all necessary checks
+    -- already performed in private.cfg_check(). See `dynamic_cfg`
+    -- 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
+
     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
+    -- 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()
+    -- 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-box-execute-idempotence.test.lua b/test/box-tap/gh-4231-box-execute-idempotence.test.lua
new file mode 100755
index 000000000..940451521
--- /dev/null
+++ b/test/box-tap/gh-4231-box-execute-idempotence.test.lua
@@ -0,0 +1,37 @@
+#!/usr/bin/env tarantool
+
+--
+-- gh-4231: box.execute should be an idempotent function meaning
+-- its effect should be the same if the user chooses to save it
+-- before explicit box.cfg{} invocation and use the saved version
+-- afterwards.
+--
+
+local tap = require('tap')
+local test = tap.test('gh-4231-box-execute-idempotence')
+test:plan(3)
+
+local box_load_and_execute = box.execute
+
+-- box is not initialized after invalid box.cfg() call and
+-- box.execute() should initialize it first.
+pcall(box.cfg, {listen = 'invalid'})
+local ok, res = pcall(box.execute, 'SELECT 1')
+test:ok(ok, 'verify box.execute() after invalid box.cfg() call', {err = res})
+
+-- Make test cases independent: if the box.execute() call above
+-- fails, initialize box explicitly for the next test cases.
+box.cfg{}
+
+-- This call should be successful and should skip box
+-- (re)initialization.
+local ok, res = pcall(box_load_and_execute, 'SELECT 1')
+test:ok(ok, 'verify box_load_and_execute after successful box.cfg() call',
+        {err = res})
+
+-- Just in case: verify usual box.execute() after
+-- box_load_and_execute().
+local ok, res = pcall(box.execute, 'SELECT 1')
+test:ok(ok, 'verify box.execute() after box_load_and_execute()', {err = res})
+
+os.exit(test:check() and 0 or 1)
diff --git a/test/box-tap/gh-4231-box-execute-locking.test.lua b/test/box-tap/gh-4231-box-execute-locking.test.lua
new file mode 100755
index 000000000..52c60f764
--- /dev/null
+++ b/test/box-tap/gh-4231-box-execute-locking.test.lua
@@ -0,0 +1,46 @@
+#!/usr/bin/env tarantool
+
+-- gh-4231: run box_load_and_execute() (it is box.execute value
+-- before box will be loaded) in several fibers in parallel and
+-- ensure that it returns correct results (i.e. that the function
+-- waits until box will be fully configured).
+
+local fiber = require('fiber')
+local tap = require('tap')
+
+local box_load_and_execute = box.execute
+local fiber_count = 10
+local results = fiber.channel(fiber_count)
+
+local function select_from_vindex()
+    local res = box_load_and_execute('SELECT * FROM "_vindex"')
+    results:put(res)
+end
+
+local test = tap.test('gh-4231-box-execute-locking')
+
+test:plan(fiber_count)
+
+local exp_metadata = {
+    {name = 'id',    type = 'unsigned'},
+    {name = 'iid',   type = 'unsigned'},
+    {name = 'name',  type = 'string'},
+    {name = 'type',  type = 'string'},
+    {name = 'opts',  type = 'map'},
+    {name = 'parts', type = 'array'},
+}
+
+for _ = 1, fiber_count do
+    fiber.create(select_from_vindex)
+end
+for i = 1, fiber_count do
+    local res = results:get()
+    test:test(('result %d'):format(i), function(test)
+        test:plan(2)
+        test:is_deeply(res.metadata, exp_metadata, 'verify metadata')
+        local rows_is_ok = type(res.rows) == 'table' and #res.rows > 0
+        test:ok(rows_is_ok, 'verify rows')
+    end)
+end
+
+os.exit(test:check() and 0 or 1)
-- 
2.25.0