> --- 100 > ... >=20 > As you can see, the fiber sleeps. It didn't return > anything yet. But the push is already executed and > propagated to my console. >=20 > Now do the same with Lua: >=20 > localhost:3313> \set output lua > true; > localhost:3313> box.session.push(100) require('fiber').sleep(100) >=20 > 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. >=20 > 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. >=20 > 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 >=20 > -- Push begin >=20 > And in the end of the push you send >=20 > -- Push end >=20 > Or something like that. Also I would investigate whether Lua > has some tag-like thing similar to what YAML has. >=20 > I remember, we tried to use YAML comments instead of tags, but > don't remember why we refused to use them. I agree, my first version does not work as intended, thanks for the = catch! However I didn=E2=80=99t come up with anything better than your = suggestion, at least now it works. It could be a temporary workaround, perhaps? I will resend v2. --Apple-Mail=_7D4D13D9-257D-4D8A-BA76-BEBAA7CFFC25 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 Hi! = Thank you for the review!

On 10 Mar 2020, at 02:13, = Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:

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-ses= sion-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 =3D = NULL;

+enum {
+ = OUTPUT_FORMAT_YAML,
+ OUTPUT_FORMAT_LUA,
+};

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

Done:

+enum output_format {
+     OUTPUT_FORMAT_YAML =3D 0,
+     OUTPUT_FORMAT_LUA_LINE,
+     OUTPUT_FORMAT_LUA_BLOCK,
+};

(Here I forgot about = =E2=80=98opts=E2=80=99 in the first version, it is possible to \set = output lua,block, for example)

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

I = added 2 following functions:

+enum output_format
+console_get_output_format()

+void
+console_set_output_format(enum = output_format output_format)

+{
+ = 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 =3D {}', = which will erase the format settings.

Fixed in v2.

+ int cmp =3D = strcmp(lua_tostring(L, -1), "lua");
+ = lua_pop(L, 5);
+ if (cmp =3D=3D 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 =3D (struct port_lua *) port;
= struct lua_State *L =3D port_lua->L;
- int rc =3D = lua_yaml_encode(L, luaL_yaml_default, "!push!",
- "tag:tarantool.io/push,2018");
- if (rc =3D=3D= 2) {
- /*
- * Nil and error object are = pushed onto the stack.
- */
- = assert(lua_isnil(L, -2));
+ int fmt =3D= console_current_output(L);
+ if (fmt =3D=3D = OUTPUT_FORMAT_YAML) {
+ int rc =3D lua_yaml_encode(L, = luaL_yaml_default, "!push!",
+ "tag:tarantool.io/push,2018");
+ if (rc =3D=3D= 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 = =3D=3D 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.

+ = assert(fmt =3D=3D OUTPUT_FORMAT_LUA_LINE ||
+ = fmt =3D=3D = OUTPUT_FORMAT_LUA_BLOCK);

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

Thanks for = clarification, fixed.

+ = lua_pcall(L, 1, 1, 0);

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

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

It does = return a string, fixed.

size_t len;
const = char *result =3D lua_tolstring(L, -1, &len);
*size =3D = (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.

Perhaps, this could do

+ * 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 = 17e2c91b2..cdf64b063 100644
--- = a/src/box/lua/console.lua
+++ b/src/box/lua/console.lua
@@ -12,6 +12,7 @@ local errno =3D require('errno')
local urilib =3D require('uri')
local yaml =3D = require('yaml')
local net_box =3D require('net.box')
+local box_internal =3D require('box.internal')

local PUSH_TAG_HANDLE =3D '!push!'

@@ -96,6 +97,11 @@ local function = format_lua_value(value, opts)
return= serpent.line(value, serpent_opts)
end

+box_internal.format_lua_value =3D 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.

I=E2=80=99m = sorry, I was following the style from = schema.lua:1289

box.internal.check_space_arg =3D = check_space_arg -- for net.box

Fixed:

+-- Used = by console_session_push.
+box_internal.format_lua_value =3D = function(value)

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

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.

I agree, my = first version does not work as intended, thanks for the = catch!

However I didn=E2=80=99t come up with anything better = than your suggestion,

at least now it works. It could be = a temporary workaround, perhaps?

I will resend v2.

= --Apple-Mail=_7D4D13D9-257D-4D8A-BA76-BEBAA7CFFC25--