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

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

[Chris Sosnin]

issue: https://github.com/tarantool/tarantool/issues/4686
branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4686-session-push-fmt

Changes in v2:
 - Current format moved to struct session.
 - Applied review fixes.

Chris Sosnin (2):
  refactoring: store output format in struct session
  box: fix formatting in session.push

 extra/exports                 |  2 +
 src/box/lua/console.c         | 53 ++++++++++++++++++--------
 src/box/lua/console.lua       | 72 +++++++++++++++++++++++++++--------
 src/box/session.h             | 22 ++++++++---
 test/app-tap/console.test.lua | 14 ++++++-
 5 files changed, 126 insertions(+), 37 deletions(-)

-- 
2.21.1 (Apple Git-122.3)

[Tarantool-patches] [PATCH v2 1/2] refactoring: store output format in struct session

[Chris Sosnin]

box.session.storage is a general-purpose table, which can be
used by user. Therefore, we shouldn't store any internal details
in it.

Needed for #4686
---
 extra/exports           |  2 ++
 src/box/lua/console.c   | 12 ++++++++++++
 src/box/lua/console.lua | 36 +++++++++++++++++++++++++++++-------
 src/box/session.h       | 22 ++++++++++++++++------
 4 files changed, 59 insertions(+), 13 deletions(-)

diff --git a/extra/exports b/extra/exports
index 3a0637317..ea184fc7e 100644
--- a/extra/exports
+++ b/extra/exports
@@ -43,6 +43,8 @@ tnt_iconv_close
 tnt_iconv
 exception_get_string
 exception_get_int
+console_get_output_format
+console_set_output_format
 
 tarantool_lua_ibuf
 uuid_nil
diff --git a/src/box/lua/console.c b/src/box/lua/console.c
index 57e7e7f4f..603f7c11b 100644
--- a/src/box/lua/console.c
+++ b/src/box/lua/console.c
@@ -377,6 +377,18 @@ console_session_fd(struct session *session)
 	return session->meta.fd;
 }
 
+enum output_format
+console_get_output_format()
+{
+	return current_session()->meta.output_format;
+}
+
+void
+console_set_output_format(enum output_format output_format)
+{
+	current_session()->meta.output_format = output_format;
+}
+
 /**
  * Dump port lua data as a YAML document tagged with !push! global
  * tag.
diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua
index 17e2c91b2..7208d3d30 100644
--- a/src/box/lua/console.lua
+++ b/src/box/lua/console.lua
@@ -2,6 +2,21 @@
 --
 -- vim: ts=4 sw=4 et
 
+local ffi = require('ffi')
+ffi.cdef[[
+    enum output_format {
+        OUTPUT_FORMAT_YAML = 0,
+        OUTPUT_FORMAT_LUA_LINE,
+        OUTPUT_FORMAT_LUA_BLOCK,
+    };
+
+    enum output_format
+    console_get_output_format();
+
+    void
+    console_set_output_format(enum output_format output_format);
+]]
+
 local serpent = require('serpent')
 local internal = require('console')
 local session_internal = require('box.internal.session')
@@ -172,17 +187,24 @@ local function output_save(fmt, opts)
     -- Output format descriptors are saved per
     -- session thus each console may specify
     -- own mode.
-    box.session.storage.console_output_format = {
-        ["fmt"] = fmt, ["opts"] = opts
-    }
+    if fmt == "yaml" then
+        ffi.C.console_set_output_format(ffi.C.OUTPUT_FORMAT_YAML)
+    elseif fmt == "lua" and opts == "block" then
+        ffi.C.console_set_output_format(ffi.C.OUTPUT_FORMAT_LUA_BLOCK)
+    else
+        ffi.C.console_set_output_format(ffi.C.OUTPUT_FORMAT_LUA_LINE)
+    end
 end
 
 local function current_output()
-    local d = box.session.storage.console_output_format
-    if d == nil then
-        return default_output_format
+    local fmt = ffi.C.console_get_output_format()
+    if fmt == ffi.C.OUTPUT_FORMAT_YAML then
+        return { ["fmt"] = "yaml", ["opts"] = nil }
+    elseif fmt == ffi.C.OUTPUT_FORMAT_LUA_LINE then
+        return { ["fmt"] = "lua", ["opts"] = "line" }
+    elseif fmt == ffi.C.OUTPUT_FORMAT_LUA_BLOCK then
+        return { ["fmt"] = "lua", ["opts"] = "block" }
     end
-    return d
 end
 
 --
diff --git a/src/box/session.h b/src/box/session.h
index 6dfc7cba5..9ade6e7a5 100644
--- a/src/box/session.h
+++ b/src/box/session.h
@@ -59,6 +59,12 @@ enum session_type {
 	session_type_MAX,
 };
 
+enum output_format {
+	OUTPUT_FORMAT_YAML = 0,
+	OUTPUT_FORMAT_LUA_LINE,
+	OUTPUT_FORMAT_LUA_BLOCK,
+};
+
 extern const char *session_type_strs[];
 
 /**
@@ -73,11 +79,15 @@ extern uint32_t default_flags;
  * types, and allows to do not store attributes in struct session,
  * that are used only by a session of particular type.
  */
-union session_meta {
-	/** IProto connection. */
-	void *connection;
-	/** Console file/socket descriptor. */
-	int fd;
+struct session_meta {
+	union {
+		/** IProto connection. */
+		void *connection;
+		/** Console file/socket descriptor. */
+		int fd;
+	};
+	/** Console output format. */
+	enum output_format output_format;
 };
 
 /**
@@ -100,7 +110,7 @@ struct session {
 	/** Session virtual methods. */
 	const struct session_vtab *vtab;
 	/** Session metadata. */
-	union session_meta meta;
+	struct session_meta meta;
 	/**
 	 * ID of statements prepared in current session.
 	 * This map is allocated on demand.
-- 
2.21.1 (Apple Git-122.3)

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

[Chris Sosnin]

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
@@ -390,8 +390,8 @@ console_set_output_format(enum output_format output_format)
 }
 
 /**
- * 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.
  *
@@ -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");
+		lua_pushvalue(L, -3);
+		lua_pcall(L, 1, 1, 0);
 	}
-	assert(rc == 1);
 	assert(lua_isstring(L, -1));
 	size_t len;
 	const char *result = lua_tolstring(L, -1, &len);
@@ -423,10 +433,11 @@ 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 Lua string into a console
+ * socket.
  * @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 7208d3d30..fb7d9ca9b 100644
--- a/src/box/lua/console.lua
+++ b/src/box/lua/console.lua
@@ -27,6 +27,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!'
 
@@ -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'
+end
+
+local function is_lua_push(value)
+    return string.startswith(value, '-- Push begin\n') and
+           string.endswith(value, '\n-- Push end\n')
+end
+
 --
 -- Return current EOS value for currently
 -- active output format.
@@ -470,6 +483,7 @@ local text_connection_mt = {
         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
@@ -478,14 +492,20 @@ local text_connection_mt = {
                 if not rc then
                     break
                 end
-                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)
+                if output["fmt"] == "yaml" then
+                    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())
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
@@ -21,7 +21,7 @@ local EOL = "\n...\n"
 
 test = tap.test("console")
 
-test:plan(73)
+test:plan(74)
 
 -- Start console and connect to it
 local server = console.listen(CONSOLE_SOCKET)
@@ -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.
 --
-- 
2.21.1 (Apple Git-122.3)

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

[Vladislav Shpilevoy]

Hi! Thanks for the patchset!

Changelog and description of the patchset are missing.

On 13/03/2020 15:58, Chris Sosnin wrote:
> issue: https://github.com/tarantool/tarantool/issues/4686
> branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4686-session-push-fmt
> 
> Changes in v2:
>  - Current format moved to struct session.
>  - Applied review fixes.
> 
> Chris Sosnin (2):
>   refactoring: store output format in struct session
>   box: fix formatting in session.push
> 
>  extra/exports                 |  2 +
>  src/box/lua/console.c         | 53 ++++++++++++++++++--------
>  src/box/lua/console.lua       | 72 +++++++++++++++++++++++++++--------
>  src/box/session.h             | 22 ++++++++---
>  test/app-tap/console.test.lua | 14 ++++++-
>  5 files changed, 126 insertions(+), 37 deletions(-)
> 

Re: [Tarantool-patches] [PATCH v2 1/2] refactoring: store output format in struct session

[Vladislav Shpilevoy]

Thanks for the patch!

> refactoring: store output format in struct session

'refactoring' is not a subsystem name. For this patch it
should be 'session', I think.

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

[Vladislav Shpilevoy]

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.

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

[Chris Sosnin]

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.

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

[Vladislav Shpilevoy]

Hi! Thanks for the fixes!

>>> +		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.

Yeah, me neither. But anyway an error should be handled. With
panic or with return -1, but we won't be able to test 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.
> 
> I agree, removed -- Push end 

Since there is no 'Push end' anymore, it should not be
called 'Push begin'. I renamed it to just 'Push'.

Also you didn't add a test on the push arriving out of
bound. I did it on the branch.

See my review fixes on top of this commit on the branch,
and below. If you agree, squash them. If don't, then lets
discuss.

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

[Chris Sosnin]

Thank you very much for the review and for your fixes!

I squashed your changes and pushed the updated version.

> On 3 Apr 2020, at 02:14, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
> Hi! Thanks for the fixes!
> 
>>>> +		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.
> 
> Yeah, me neither. But anyway an error should be handled. With
> panic or with return -1, but we won't be able to test 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.
>> 
>> I agree, removed -- Push end 
> 
> Since there is no 'Push end' anymore, it should not be
> called 'Push begin'. I renamed it to just 'Push'.
> 
> Also you didn't add a test on the push arriving out of
> bound. I did it on the branch.
> 
> See my review fixes on top of this commit on the branch,
> and below. If you agree, squash them. If don't, then lets
> discuss.
> 
> 

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

[Vladislav Shpilevoy]

Thanks for the fixes!

The patchset LGTM. Except that the changelog is still
missing.

Please, add a changelog, and send on a second review.
I propose either Nikita or Alexander L.

On 13/03/2020 15:58, Chris Sosnin wrote:
> issue: https://github.com/tarantool/tarantool/issues/4686
> branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4686-session-push-fmt
> 
> Changes in v2:
>  - Current format moved to struct session.
>  - Applied review fixes.
> 
> Chris Sosnin (2):
>   refactoring: store output format in struct session
>   box: fix formatting in session.push
> 
>  extra/exports                 |  2 +
>  src/box/lua/console.c         | 53 ++++++++++++++++++--------
>  src/box/lua/console.lua       | 72 +++++++++++++++++++++++++++--------
>  src/box/session.h             | 22 ++++++++---
>  test/app-tap/console.test.lua | 14 ++++++-
>  5 files changed, 126 insertions(+), 37 deletions(-)
> 

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

[Vladislav Shpilevoy]

Hi! I suggest to hurry up, we have the release closer and closer.

On 04/04/2020 01:42, Vladislav Shpilevoy wrote:
> Thanks for the fixes!
> 
> The patchset LGTM. Except that the changelog is still
> missing.
> 
> Please, add a changelog, and send on a second review.
> I propose either Nikita or Alexander L.
> 
> On 13/03/2020 15:58, Chris Sosnin wrote:
>> issue: https://github.com/tarantool/tarantool/issues/4686
>> branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4686-session-push-fmt
>>
>> Changes in v2:
>>  - Current format moved to struct session.
>>  - Applied review fixes.
>>
>> Chris Sosnin (2):
>>   refactoring: store output format in struct session
>>   box: fix formatting in session.push
>>
>>  extra/exports                 |  2 +
>>  src/box/lua/console.c         | 53 ++++++++++++++++++--------
>>  src/box/lua/console.lua       | 72 +++++++++++++++++++++++++++--------
>>  src/box/session.h             | 22 ++++++++---
>>  test/app-tap/console.test.lua | 14 ++++++-
>>  5 files changed, 126 insertions(+), 37 deletions(-)
>>

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

[Nikita Pettik]

On 07 Apr 22:29, Vladislav Shpilevoy wrote:
> Hi! I suggest to hurry up, we have the release closer and closer.

Still 54 open issues.
 
> On 04/04/2020 01:42, Vladislav Shpilevoy wrote:
> > Thanks for the fixes!
> > 
> > The patchset LGTM. Except that the changelog is still
> > missing.
> > 
> > Please, add a changelog, and send on a second review.
> > I propose either Nikita or Alexander L.
> > 
> > On 13/03/2020 15:58, Chris Sosnin wrote:
> >> issue: https://github.com/tarantool/tarantool/issues/4686
> >> branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4686-session-push-fmt
> >>
> >> Changes in v2:
> >>  - Current format moved to struct session.
> >>  - Applied review fixes.
> >>
> >> Chris Sosnin (2):
> >>   refactoring: store output format in struct session
> >>   box: fix formatting in session.push
> >>
> >>  extra/exports                 |  2 +
> >>  src/box/lua/console.c         | 53 ++++++++++++++++++--------
> >>  src/box/lua/console.lua       | 72 +++++++++++++++++++++++++++--------
> >>  src/box/session.h             | 22 ++++++++---
> >>  test/app-tap/console.test.lua | 14 ++++++-
> >>  5 files changed, 126 insertions(+), 37 deletions(-)
> >>

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

[Chris Sosnin]

I’ve sent the patch set to Nikita with the following change log:

@ChangeLog
- Add Lua output format support for box.session.push

I thought this will suffice.

> On 7 Apr 2020, at 23:29, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
> Hi! I suggest to hurry up, we have the release closer and closer.
> 
> On 04/04/2020 01:42, Vladislav Shpilevoy wrote:
>> Thanks for the fixes!
>> 
>> The patchset LGTM. Except that the changelog is still
>> missing.
>> 
>> Please, add a changelog, and send on a second review.
>> I propose either Nikita or Alexander L.
>> 
>> On 13/03/2020 15:58, Chris Sosnin wrote:
>>> issue: https://github.com/tarantool/tarantool/issues/4686
>>> branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4686-session-push-fmt
>>> 
>>> Changes in v2:
>>> - Current format moved to struct session.
>>> - Applied review fixes.
>>> 
>>> Chris Sosnin (2):
>>>  refactoring: store output format in struct session
>>>  box: fix formatting in session.push
>>> 
>>> extra/exports                 |  2 +
>>> src/box/lua/console.c         | 53 ++++++++++++++++++--------
>>> src/box/lua/console.lua       | 72 +++++++++++++++++++++++++++--------
>>> src/box/session.h             | 22 ++++++++---
>>> test/app-tap/console.test.lua | 14 ++++++-
>>> 5 files changed, 126 insertions(+), 37 deletions(-)
>>> 

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

[Kirill Yukhin]

Hello,

On 13 Mar 17:58, Chris Sosnin wrote:
> issue: https://github.com/tarantool/tarantool/issues/4686
> branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4686-session-push-fmt
> 
> Changes in v2:
>  - Current format moved to struct session.
>  - Applied review fixes.
> 
> Chris Sosnin (2):
>   refactoring: store output format in struct session
>   box: fix formatting in session.push

I've checked your patch into 2.3 and master.

--
Regards, Kirill Yukhin