From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp53.i.mail.ru (smtp53.i.mail.ru [94.100.177.113]) (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 3E992469719 for ; Thu, 20 Feb 2020 20:51:04 +0300 (MSK) Date: Thu, 20 Feb 2020 20:51:24 +0300 From: Alexander Turenko Message-ID: <20200220175124.pwj24lx3jz4w6zcm@tkn_work_nb> References: <20191114115020.21091-1-maria.khaydich@tarantool.org> <1578917620.195553363@f475.i.mail.ru> <20200113154829.GI31304@tarantool.org> <1579344991.266039392@f319.i.mail.ru> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1579344991.266039392@f319.i.mail.ru> Subject: Re: [Tarantool-patches] [PATCH] box: make box.execute() immutable List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Maria Khaydich Cc: tarantool-patches@dev.tarantool.org 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 > 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 (which is assigned to box.execute when box is not configured) and (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 will be overwritten with some , but that the initial 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 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 : | 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 ) should configure box if it is not configured or just execute otherwise (w/o box reconfiguration). * box.cfg() (more specifically ) 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 when box is not loaded, +-- otherwise. See also 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. +-- loads box and calls . +-- +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 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