Re: [Tarantool-patches] [PATCH v2 2/2] box: fix formatting in session.push

[Chris Sosnin <k.sosnin@tarantool.org>]

Thank you for the review!

> On 15 Mar 2020, at 20:27, Vladislav Shpilevoy <v.shpilevoy@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.