[Tarantool-patches] [PATCH v2 2/2] box: fix formatting in session.push
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Sun Mar 15 20:27:19 MSK 2020
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.
More information about the Tarantool-patches
mailing list