[Tarantool-patches] [PATCH] box: make box.execute() immutable
Alexander Turenko
alexander.turenko at tarantool.org
Thu Feb 20 20:51:24 MSK 2020
Sorry for the late response.
See comments below.
While looking into the patch, experimenting and commenting I made the
patch (it is based on your changes), so I'll paste it at end of the
email for the reference.
WBR, Alexander Turenko.
> commit 573318ea8749931741247f181333ab68b43a82c6
> Author: Maria <maria.khaydich at tarantool.org>
> Date: Tue Oct 29 17:56:51 2019 +0300
>
> box: box.execute should be immutable function
Now I recalled the right term: idempotent.
Nit: it would be good to use imperative mood in the commit header:
| Use the imperative mood in the subject line. A properly formed Git
| commit subject line should always be able to complete the following
| sentence: “If applied, this commit will /your subject line here/”.
https://www.tarantool.io/en/doc/1.10/dev_guide/developer_guidelines/#how-to-write-a-commit-message
>
> 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.
Sorry that I nitpick around wording again, but after the description I
would check something like the following:
| box.execute('SELECT 1') -- here box.cfg{} is called under hood
| box.execute('SELECT 1') -- everything looks ok
After that I would be confused, because everything works as expected.
I would say that there are two different functions
<box_load_and_execute> (which is assigned to box.execute when box is not
configured) and <lbox_execute> (after box confguration) and would
describe the case using those terms. I guess it would easier to
understand.
See also the comment about the test at very end of the email.
>
> The patch introduces a fix making box.execute immutable.
>
> Closes #4231
>
> 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
Nit: over 66 symbols.
> +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
After this we can failed to configure box, consider the case:
| tarantool> box.cfg{listen = 'incorrect'}
| ---
| - error: 'Incorrect value for option ''listen'': expected host:service or /unix.socket'
| ...
|
| tarantool> box.execute('SELECT 1')
We should set the flag only at successful box configuration. I would
also add such test case.
> -- 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.
Technically saving of box.execute does not lead to invocation of
box.cfg. But anyway I would describe things in the way as proposed
above.
> + -- The flag box_loaded makes sure execute
> + -- is an immutable function meaning it won't be overwritten by
> + -- any following attempts to reconfigure box.
> + --
The problem is not that <lbox_execute> will be overwritten with some
<box_another_execute>, but that the initial <box_load_and_execute> does
not work correctly after successful box configuration.
> + if not box_loaded then
> + load_cfg()
> + end
I considered using of `type(box.cfg) == 'function'` check as in
tarantoolctl, but in fact it is not realiable: if box was not configured
after box.cfg() due to an error (say, after `box.cfg{listen =
'invalid'}`) the type of box.cfg will be 'table' and reload_cfg() will
work on the next box.cfg() call. So we should use box_is_configured C
function:
| local ffi = require('ffi')
|
| ffi.cdef([[
| bool
| box_is_configured(void);
| ]])
|
| local function box_is_configured()
| return ffi.C.box_is_configured()
| end
This way requires to add the function into extra/exports.
BTW, it seems that it is possible that <box_load_and_execute> will be
called when `box.cfg{}` is already in progress in another fiber: this is
why all load_cfg() / reload_cfg() calls are decorated using `locked`
wrapper. It seems we should do the same in <box_load_and_execute>:
| function box.execute(...)
| if not box_is_configured() then
| locked(function()
| -- Re-check, because after the lock release the box
| -- state may be changed. We should call load_cfg()
| -- only once.
| if not box_is_configured() then
| load_cfg()
| end
| end)()
| end
| return box.execute(...)
| end
I experimented with this like so:
| $ ./src/tarantool
| tarantool> box_execute = box.execute
| tarantool> for i = 1, 10 do require('fiber').create(function() require('log').info(require('yaml').encode((box_execute('SELECT * FROM "_vindex"')))) end) end
While looking into the code I found one more case:
| tarantool> box_cfg = box.cfg
| ---
| ...
| tarantool> box_cfg()
| <...>
| tarantool> box_cfg()
| ---
| - error: 'builtin/box/load_cfg.lua:19: builtin/box/load_cfg.lua:547: bad argument
| #1 to ''pairs'' (table expected, got nil)'
| ...
I think it does NOT mean that we should not fix box.execute() code,
because the expected behaviour is a bit different:
* box.execute() (more specifically <box_load_and_execute>) should
configure box if it is not configured or just execute otherwise (w/o
box reconfiguration).
* box.cfg() (more specifically <load_cfg>) should configure box if it is
not configured, otherwise **reconfigure** it.
So box.cfg() should call reload_cfg() if box is already configured. I
propose to fix it in a separate patch (but within the scope of the
issue).
> 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..5bdbec88f
> --- /dev/null
> +++ b/test/box-tap/gh-4231-immutable-box-execute.test.lua
> @@ -0,0 +1,33 @@
> +#!/usr/bin/env tarantool
> +local tap = require('tap')
> +local test = tap.test('execute')
> +test:plan(3)
> +
> +--
> +-- gh-4231: box.execute should be immutable function meaning it's
> +-- address doesn't change after first box.cfg implicit invocation
But it is not so even after the patch:
| tarantool> box.execute
| ---
| - 'function: 0x4074bc30'
| ...
| tarantool> box.execute('SELECT 1')
| <...>
| tarantool> box.execute
| ---
| - 'function: 0x416b2600'
| ...
> +--
> +
> +local function execute_is_immutable(execute, cmd, msg)
> + local status, err = pcall(function()
> + execute(cmd)
> + end)
Nit: Can be written as:
| local status, err = pcall(execute, cmd)
> + test:ok(status and err == nil, msg)
> +end
> +
> +local box_execute_stub = box.execute
> +execute_is_immutable(box_execute_stub, "SELECT 1",
> + "execute does not work properly before box.cfg")
This will print:
ok - execute does not work properly before box.cfg
--that looks counter-intuitive for me.
BTW, it looks as the preparation code for the test rather than the test
itself. Personally I think that such code should do assert() rather than
produce 'ok/not ok' TAP13 output. This is not the strict requirement,
however.
Consider [1], from words 'I would split test preparation / clean up code
from actual test cases'.
[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-January/013595.html
> +
> +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
Typo: adress -> address.
> +test:is(box_execute_actual, box.execute,
> + "execute is not the same after box.cfg")
> +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)
The test case works properly (does not fail) on current master w/o the
fix. I guess you intend to test box_execute_stub after box.cfg().
BTW, I suggest to run a test on a tarantool version before a fix to
verify that the test actually able to catch the problem.
----
diff --git a/extra/exports b/extra/exports
index 7b84a1452..8599614ee 100644
--- a/extra/exports
+++ b/extra/exports
@@ -118,6 +118,8 @@ swim_member_unref
swim_member_is_dropped
swim_member_is_payload_up_to_date
+box_is_configured
+
# Module API
_say
diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
index 1f9b4392f..dfff772d7 100644
--- a/src/box/lua/load_cfg.lua
+++ b/src/box/lua/load_cfg.lua
@@ -6,6 +6,12 @@ local private = require('box.internal')
local urilib = require('uri')
local math = require('math')
local fiber = require('fiber')
+local ffi = require('ffi')
+
+ffi.cdef([[
+ bool
+ box_is_configured(void);
+]])
-- Function decorator that is used to prevent box.cfg() from
-- being called concurrently by different fibers.
@@ -517,14 +523,10 @@ 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
@@ -562,24 +564,43 @@ local function load_cfg(cfg)
end
box.cfg = locked(load_cfg)
+local function box_is_configured()
+ return ffi.C.box_is_configured()
+end
+
--
--- This makes possible do box.execute without calling box.cfg
--- manually. The load_cfg call overwrites following table and
--- metatable.
+-- box.execute is <box_load_and_execute> when box is not loaded,
+-- <lbox_execute> otherwise. See also <box_configured> variable.
--
-function box.execute(...)
- --
- -- 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.
+-- <box_load_and_execute> loads box and calls <lbox_execute>.
+--
+local box_load_and_execute
+box_load_and_execute = function(...)
+ -- Configure box if it is not configured, no-op otherwise (not
+ -- reconfiguration).
--
- if not box_loaded then
- load_cfg()
+ -- 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
More information about the Tarantool-patches
mailing list