From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp43.i.mail.ru (smtp43.i.mail.ru [94.100.177.103]) (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 29C214696C3 for ; Sun, 15 Mar 2020 20:27:21 +0300 (MSK) References: <33ee0fa0264869e7f5c0a952b2812a1125e56a82.1584110394.git.k.sosnin@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Sun, 15 Mar 2020 18:27:19 +0100 MIME-Version: 1.0 In-Reply-To: <33ee0fa0264869e7f5c0a952b2812a1125e56a82.1584110394.git.k.sosnin@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v2 2/2] box: fix formatting in session.push List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Chris Sosnin , tarantool-patches@dev.tarantool.org Thanks for the patch! See 4 comments below. On 13/03/2020 15:58, Chris Sosnin wrote: > box.session.push() encodes data as a YAML document independent on > the current console output format. This patch adds handling for Lua > as well. > > Closes #4686 > --- > src/box/lua/console.c | 41 ++++++++++++++++++++++------------- > src/box/lua/console.lua | 36 +++++++++++++++++++++++------- > test/app-tap/console.test.lua | 14 +++++++++++- > 3 files changed, 67 insertions(+), 24 deletions(-) > > diff --git a/src/box/lua/console.c b/src/box/lua/console.c > index 603f7c11b..8d3e40f95 100644 > --- a/src/box/lua/console.c > +++ b/src/box/lua/console.c > @@ -403,18 +403,28 @@ port_lua_dump_plain(struct port *port, uint32_t *size) > { > struct port_lua *port_lua = (struct port_lua *) port; > struct lua_State *L = port_lua->L; > - int rc = lua_yaml_encode(L, luaL_yaml_default, "!push!", > - "tag:tarantool.io/push,2018"); > - if (rc == 2) { > - /* > - * Nil and error object are pushed onto the stack. > - */ > - assert(lua_isnil(L, -2)); > - assert(lua_isstring(L, -1)); > - diag_set(ClientError, ER_PROC_LUA, lua_tostring(L, -1)); > - return NULL; > + enum output_format fmt = console_get_output_format(); > + if (fmt == OUTPUT_FORMAT_YAML) { > + int rc = lua_yaml_encode(L, luaL_yaml_default, "!push!", > + "tag:tarantool.io/push,2018"); > + if (rc == 2) { > + /* > + * Nil and error object are pushed onto the stack. > + */ > + assert(lua_isnil(L, -2)); > + assert(lua_isstring(L, -1)); > + diag_set(ClientError, ER_PROC_LUA, lua_tostring(L, -1)); > + return NULL; > + } > + assert(rc == 1); > + } else { > + assert(fmt == OUTPUT_FORMAT_LUA_LINE || > + fmt == OUTPUT_FORMAT_LUA_BLOCK); > + luaL_findtable(L, LUA_GLOBALSINDEX, "box.internal", 1); > + lua_getfield(L, -1, "format_lua_value"); 1. format_lua_value() does not look like a good name taking into account, that now it not only formats a value, but also adds 'push' comments. 'format_lua_push()'? > + lua_pushvalue(L, -3); > + lua_pcall(L, 1, 1, 0); 2. In case lua_pcall() fails, the function should fail too, and should return NULL + set an error in diag. Also it would be good to have a test on it. > } > - assert(rc == 1); > assert(lua_isstring(L, -1)); > size_t len; > const char *result = lua_tolstring(L, -1, &len); > diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua > index 7208d3d30..fb7d9ca9b 100644 > --- a/src/box/lua/console.lua > +++ b/src/box/lua/console.lua > @@ -207,6 +208,18 @@ local function current_output() > end > end > > +-- Used by console_session_push. > +box_internal.format_lua_value = function(value) > + local opts = current_output()["opts"] > + value = format_lua_value(value, opts) > + return '-- Push begin\n' .. value .. '\n-- Push end\n' 3. Did you check that it is really necessary to append a special 'push end' tail? I couldn't find any Lua value, which couldn't be identified by 'push begin' and eos (';') markers. > +end > + > +local function is_lua_push(value) > + return string.startswith(value, '-- Push begin\n') and > + string.endswith(value, '\n-- Push end\n') > +end > diff --git a/test/app-tap/console.test.lua b/test/app-tap/console.test.lua > index da5c1e71e..26f3916a4 100755 > --- a/test/app-tap/console.test.lua > +++ b/test/app-tap/console.test.lua > @@ -39,6 +39,18 @@ test:is(client:read(EOL), '%TAG !push! tag:tarantool.io/push,2018\n--- 200\n...\ > "pushed message") > test:is(client:read(EOL), '---\n- true\n...\n', "pushed message") > > +-- > +-- gh-4686: box.session.push should respect output format. > +-- > +client:write('\\set output lua\n') > +client:read(";") > + > +client:write('box.session.push(200)\n') > +test:is(client:read(";"), '-- Push begin\n200\n-- Push end\ntrue;', "pushed message") > + > +client:write('\\set output yaml\n') > +client:read(EOL) > + > -- > -- gh-3790: box.session.push support uint64_t sync. > -- 4. My test from the previous version of the patch still does not work. I tried box.session.push() to a Lua console + a long fiber sleep, and didn't get any output. I paste it below: > Tarantool1: > > require('console').listen(3313) > > Tarantool2: > > require('console').connect(3313) > > Now first case, when everything works fine: > > localhost:3313> box.session.push(100) require('fiber').sleep(100) > %TAG !push! tag:tarantool.io/push,2018 > --- 100 > ... > > As you can see, the fiber sleeps. It didn't return > anything yet. But the push is already executed and > propagated to my console. > > Now do the same with Lua: > > localhost:3313> \set output lua > true; > localhost:3313> box.session.push(100) require('fiber').sleep(100) > > And no output. Push 100 should have been printed right > away, but it is not. This is because console waits for ';' > before printing anything when in Lua mode. I tried to investigate, and found that you can't call current_output() in text_connection_mt.eval. Because '\set output ' is executed on server side, not on client side. Even after you change output to Lua, it is still YAML on the client side. > eval = function(self, text) > self:preprocess_eval(text) > text = text..'$EOF$\n' > local output = current_output() > if not self:write(text) then > error(self:set_error()) > end > while true do > local rc = self:read() > if not rc then > break > end > if output["fmt"] == "yaml" then As a result, this check is always true. > local handle, prefix = yaml.decode(rc, {tag_only = true}) > if not handle then > -- Can not fail - tags are encoded with no > -- user participation and are correct always. > return rc > end > if handle == PUSH_TAG_HANDLE and self.print_f then > self.print_f(rc) > end > else > if is_lua_push(rc) and self.print_f then > self.print_f(rc) > end > end > end > return error(self:set_error()) > end, Results are formatted in Lua, because the server formats them. The only changed thing on the client side is eos symbol in text_connection_mt.preprocess_eval. It means, that you need to save output format somewhere on the client too. Not just eos.