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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the patch!

See 9 comments below.

On 06/03/2020 15:03, Chris Sosnin wrote:
> box.session.push() encodes data as a YAML document independent on
> the current console output format. This patch handles lua case in the
> following way:
> 
> tarantool>box.session.push(<data>)
> <data>
> true;
> 
> Closes #4686
> ---
> issue: https://github.com/tarantool/tarantool/issues/4686
> branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4686-session-push-fmt
> 
>  src/box/lua/console.c         | 62 ++++++++++++++++++++++++++---------
>  src/box/lua/console.lua       |  6 ++++
>  test/app-tap/console.test.lua | 14 +++++++-
>  3 files changed, 66 insertions(+), 16 deletions(-)
> 
> diff --git a/src/box/lua/console.c b/src/box/lua/console.c
> index 57e7e7f4f..a7573c306 100644
> --- a/src/box/lua/console.c
> +++ b/src/box/lua/console.c
> @@ -50,6 +50,11 @@ extern char serpent_lua[];
>  
>  static struct luaL_serializer *luaL_yaml_default = NULL;
>  
> +enum {
> +	OUTPUT_FORMAT_YAML,
> +	OUTPUT_FORMAT_LUA,
> +};

1. Please, give it a type and use it as a result of
console_current_output().

> +
>  /*
>   * Completion engine (Mike Paul's).
>   * Used internally when collecting completions locally. Also a Lua
> @@ -377,9 +382,28 @@ console_session_fd(struct session *session)
>  	return session->meta.fd;
>  }
>  
> +static int
> +console_current_output(struct lua_State *L)

2. console_output_format() name would look better here, IMO.

> +{
> +	lua_getfield(L, LUA_GLOBALSINDEX, "box");
> +	lua_getfield(L, -1, "session");
> +	lua_getfield(L, -1, "storage");
> +	lua_getfield(L, -1, "console_output_format");
> +	if (lua_isnil(L, -1)) {
> +		lua_pop(L, 4);
> +		return OUTPUT_FORMAT_YAML;
> +	}
> +	lua_getfield(L, -1, "fmt");

3. I think we should not use box.session.storage for internal
information, such as output format. I would rather move it to
session_meta in struct session in a separate preparatory patch,
(or as a follow-up patch).

box.session.storage is a documented table allowed to be used by
users in any way. Some code easily may contain something like
'box.session.storage = {}', which will erase the format settings.

> +	int cmp = strcmp(lua_tostring(L, -1), "lua");
> +	lua_pop(L, 5);
> +	if (cmp == 0)
> +		return OUTPUT_FORMAT_LUA;
> +	return OUTPUT_FORMAT_YAML;
> +}
> +
>  /**
> - * Dump port lua data as a YAML document tagged with !push! global
> - * tag.
> + * Dump port lua data with respect to output format:
> + * YAML document tagged with !push! global tag or lua string.
>   * @param port Port lua.
>   * @param[out] size Size of the result.
>   *
> @@ -391,19 +415,27 @@ 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));
> +	int fmt = console_current_output(L);
> +	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);
>  		assert(lua_isstring(L, -1));
> -		diag_set(ClientError, ER_PROC_LUA, lua_tostring(L, -1));
> -		return NULL;
> +	} else { /* OUTPUT_FORMAT_LUA */

4. Better make it an assertion. That the format is 'Lua' here.

> +		luaL_findtable(L, LUA_GLOBALSINDEX, "box.internal", 1);
> +		lua_getfield(L, -1, "format_lua_value");
> +		lua_pushvalue(L, -3);
> +		lua_call(L, 1, 1);

5. Should be pcall(). Otherwise any error will break C code, which
called box_session_push(). lua_yaml_encode() is also unsafe, but it
uses Lua deeply inside. On the contrary, this place is easy to
protect. Also add a test case, please. Seems like serpent can fail on
some input. At least, I see 2 error() calls in its sources.

>  	}
> -	assert(rc == 1);
> -	assert(lua_isstring(L, -1));

6. Why did you remove that? Does it return not a string for Lua format?

>  	size_t len;
>  	const char *result = lua_tolstring(L, -1, &len);
>  	*size = (uint32_t) len;
> @@ -411,10 +443,10 @@ port_lua_dump_plain(struct port *port, uint32_t *size)
>  }
>  
>  /**
> - * Push a tagged YAML document into a console socket.
> + * Push a tagged YAML document or a plain text into a console socket.

7. The line is out of 66 symbols now.

YAML document is also plain text. The comment should be more
specific, probably.

>   * @param session Console session.
>   * @param sync Unused request sync.
> - * @param port Port with YAML to push.
> + * @param port Port with the data to push.
>   *
>   * @retval  0 Success.
>   * @retval -1 Error.
> diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua
> index 17e2c91b2..cdf64b063 100644
> --- a/src/box/lua/console.lua
> +++ b/src/box/lua/console.lua
> @@ -12,6 +12,7 @@ local errno = require('errno')
>  local urilib = require('uri')
>  local yaml = require('yaml')
>  local net_box = require('net.box')
> +local box_internal = require('box.internal')
>  
>  local PUSH_TAG_HANDLE = '!push!'
>  
> @@ -96,6 +97,11 @@ local function format_lua_value(value, opts)
>      return serpent.line(value, serpent_opts)
>  end
>  
> +box_internal.format_lua_value = function(value)  -- for console_session_push

8. Please,
- Don't write comments on the same lines as code;
- Use single whitespace between words;
- Use a capital letter in the beginning of sentence;
- Use the dot in sentence's end.

> +    value = format_lua_value(value)
> +    return value .. '\n'
> +end
> +
>  output_handlers["lua"] = function(status, opts, ...)
>      local collect = {}
>      --

9. Generally, I see that the patch prints pushes exactly like result of
a function. I am not sure this is ok. For example, take YAML. The whole point
in having YAML tags for YAML push output was in being able to separate pushes
from a normal result and from each other. In that way pushes could be returned
earlier than function returns. What is the whole purpose of pushes really.

With the current Lua implementation I am not sure this is possible. Take this
simple example.

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.

So you need to design a way how to distinguish pushes from
normal results, and print them earlier. But don't treat them
as a final result. This was huge pain in the ass, when was
being designed for YAML. I hope you will find something simple
for Lua.

For example, AFAIU, we could try to use Lua comments as some
kind of hidden meta. When you send a push, you first sent a
comment saying

     -- Push begin

And in the end of the push you send

     -- Push end

Or something like that. Also I would investigate whether Lua
has some tag-like thing similar to what YAML has.

I remember, we tried to use YAML comments instead of tags, but
don't remember why we refused to use them.