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 D4C0046970E for ; Tue, 17 Dec 2019 17:41:50 +0300 (MSK) Date: Tue, 17 Dec 2019 17:39:40 +0300 From: Igor Munkin Message-ID: <20191217143940.GQ1214@tarantool.org> References: <20191114115020.21091-1-maria.khaydich@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20191114115020.21091-1-maria.khaydich@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH] box.execute should be immutable function List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Maria Cc: tarantool-patches@dev.tarantool.org Masha, Thanks for the patch! I join to all remarks left by Nikita. Besides, I added a couple new below related to the patch itself. On 14.11.19, Maria wrote: > Using box.execute method before explicitly > configuring box automatically invoked box.cfg > nonetheless. Any further calls to the latter > caused its reconfiguration which could lead > to an error when trying to use the method. > > Closes #4231 > Issue: > https://github.com/tarantool/tarantool/issues/4231 > Branch: > https://github.com/tarantool/tarantool/compare/eljashm/gh-4231-box.execute-immutable-function > --- > src/box/lua/load_cfg.lua | 4 +++- > test/box-tap/execute.test.lua | 17 +++++++++++++++++ > 2 files changed, 20 insertions(+), 1 deletion(-) > create mode 100755 test/box-tap/execute.test.lua > > diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua > index e7f62cf4e..042edf913 100644 > --- a/src/box/lua/load_cfg.lua > +++ b/src/box/lua/load_cfg.lua > @@ -528,7 +528,9 @@ box.cfg = locked(load_cfg) > -- metatable. > -- > function box.execute(...) > - load_cfg() I had to read a lot of source code to dive into the problem to be fixed. It's OK, but IMHO it would be great if you dropped a few words right here about the bug and mentioned the related issue. This would definitely simplify further maintenance. > + if not box.cfg then This condition solves the reported problem (the test you provided within this patch is OK), but consider the following: | $ git lo -1 | ec09fcaac box.execute should be immutable function | $ ./tarantool -V | Tarantool 2.3.0-161-gec09fcaac | | $ echo 'box.execute("SELECT 1")' | timeout -s 9 3 ./tarantool | Killed Thus box.execute call prior to box.cfg invokation hangs Tarantool after your patch. I'm totally not into SQL design but AFAIS such box.execute invokation ought to call loag_cfg underneath. However it doesn't since box.cfg is initialized with function several lines above, the check is failed and Tarantool goes into deep recursion. I guess you can introduce a local flag and use it as an upvalue in both box.execute and load_cfg functions. The flag is to be initialized to false, set in the load_cfg call and check within box.execute. However feel free to provide your own solution. > + load_cfg() > + end > return box.execute(...) > end > > diff --git a/test/box-tap/execute.test.lua b/test/box-tap/execute.test.lua > new file mode 100755 > index 000000000..301cf4a1c > --- /dev/null > +++ b/test/box-tap/execute.test.lua > @@ -0,0 +1,17 @@ > +#!/usr/bin/env tarantool > + > +local tap = require('tap') > +local test = tap.test('execute') > +test:plan(1) > + > +local box_execute = box.execute Please consider to add the check related to the flaw found above. > +box.cfg{} > + > +local status, err = pcall( > + function() > + box_execute("CREATE TABLE t1 (s1 INTEGER, PRIMARY KEY (s1));") Minor: A trailing whitespace is left above. > + end) There is a common code style for pcall an anonymous multiline Lua function within box-tap (see [1] and [2]). Please adjust yours considering it. > + > +test:ok(status and err == nil, "box.execute after box.cfg") Please consider the comment regarding the test finalization[3]. > +test:check() > +os.exit(0) > -- > 2.21.0 (Apple Git-122.2) > [1]: https://github.com/tarantool/tarantool/blob/master/test/box-tap/cfg.test.lua#L148 [2]: https://github.com/tarantool/tarantool/blob/master/test/box-tap/merger.test.lua#L573 [3]: https://github.com/tarantool/tarantool/issues/4655#issue-529430689 -- Best regards, IM