From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp40.i.mail.ru (smtp40.i.mail.ru [94.100.177.100]) (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 25CFB43E89B for ; Fri, 27 Mar 2020 13:26:37 +0300 (MSK) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 13.0 \(3608.60.0.2.5\)) From: Chris Sosnin In-Reply-To: Date: Fri, 27 Mar 2020 13:26:35 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <7157885B-8F81-44C4-A317-1DA71CFC8008@tarantool.org> References: <33ee0fa0264869e7f5c0a952b2812a1125e56a82.1584110394.git.k.sosnin@tarantool.org> 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: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org Thank you for the review! > On 15 Mar 2020, at 20:27, Vladislav Shpilevoy = wrote: >=20 > Thanks for the patch! >=20 > See 4 comments below. >=20 > 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. >>=20 >> 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(-) >>=20 >> 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 =3D (struct port_lua *) port; >> struct lua_State *L =3D port_lua->L; >> - int rc =3D lua_yaml_encode(L, luaL_yaml_default, "!push!", >> - "tag:tarantool.io/push,2018"); >> - if (rc =3D=3D 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 =3D console_get_output_format(); >> + if (fmt =3D=3D OUTPUT_FORMAT_YAML) { >> + int rc =3D lua_yaml_encode(L, luaL_yaml_default, = "!push!", >> + "tag:tarantool.io/push,2018"); >> + if (rc =3D=3D 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 =3D=3D 1); >> + } else { >> + assert(fmt =3D=3D OUTPUT_FORMAT_LUA_LINE || >> + fmt =3D=3D OUTPUT_FORMAT_LUA_BLOCK); >> + luaL_findtable(L, LUA_GLOBALSINDEX, "box.internal", 1); >> + lua_getfield(L, -1, "format_lua_value"); >=20 > 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()'? I agree, renamed it to format_lua_push. >=20 >> + lua_pushvalue(L, -3); >> + lua_pcall(L, 1, 1, 0); >=20 > 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. I couldn=E2=80=99t really find a way to make serpent raise an error. >=20 >> } >> - assert(rc =3D=3D 1); >> assert(lua_isstring(L, -1)); >> size_t len; >> const char *result =3D 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 >>=20 >> +-- Used by console_session_push. >> +box_internal.format_lua_value =3D function(value) >> + local opts =3D current_output()["opts"] >> + value =3D format_lua_value(value, opts) >> + return '-- Push begin\n' .. value .. '\n-- Push end\n' >=20 > 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. I agree, removed -- Push end=20 >=20 >> +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") >>=20 >> +-- >> +-- 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. >> -- >=20 > 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. >=20 > I paste it below: >=20 >> Tarantool1: >>=20 >> require('console').listen(3313) >>=20 >> Tarantool2: >>=20 >> require('console').connect(3313) >>=20 >> Now first case, when everything works fine: >>=20 >> localhost:3313> box.session.push(100) require('fiber').sleep(100) >> %TAG !push! tag:tarantool.io/push,2018 >> --- 100 >> ... >>=20 >> As you can see, the fiber sleeps. It didn't return >> anything yet. But the push is already executed and >> propagated to my console. >>=20 >> Now do the same with Lua: >>=20 >> localhost:3313> \set output lua >> true; >> localhost:3313> box.session.push(100) require('fiber').sleep(100) >>=20 >> 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. >=20 >> eval =3D function(self, text) >> self:preprocess_eval(text) >> text =3D text..'$EOF$\n' >> local output =3D current_output() >> if not self:write(text) then >> error(self:set_error()) >> end >> while true do >> local rc =3D self:read() >> if not rc then >> break >> end >> if output["fmt"] =3D=3D "yaml" then >=20 > As a result, this check is always true. >=20 >> local handle, prefix =3D yaml.decode(rc, {tag_only = =3D true}) >> if not handle then >> -- Can not fail - tags are encoded with no >> -- user participation and are correct always. >> return rc >> end >> if handle =3D=3D 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, >=20 > 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. >=20 > It means, that you need to save output format somewhere on the > client too. Not just eos. Thank you very much for the tips, I guess I finally got this right, now your example works as intended. I will resend the diff.