[Tarantool-patches] [PATCH v2 2/2] box: fix formatting in session.push
Chris Sosnin
k.sosnin at tarantool.org
Fri Mar 27 13:26:35 MSK 2020
Thank you for the review!
> On 15 Mar 2020, at 20:27, Vladislav Shpilevoy <v.shpilevoy at tarantool.org> wrote:
>
> 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()'?
I agree, renamed it to 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.
I couldn’t really find a way to make serpent raise an error.
>
>> }
>> - 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.
I agree, removed -- Push end
>
>> +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 <type>' 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.
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.
More information about the Tarantool-patches
mailing list