From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp16.mail.ru (smtp16.mail.ru [94.100.176.153]) (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 769DC469710 for ; Wed, 13 May 2020 01:18:34 +0300 (MSK) From: Alexander Turenko Date: Wed, 13 May 2020 01:18:03 +0300 Message-Id: In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: [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: Igor Munkin Cc: tarantool-patches@dev.tarantool.org 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 +-- 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 +-- 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 + -- 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() + -- 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