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

[Vladislav Shpilevoy <v.shpilevoy@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 <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.