* [Tarantool-patches] [PATCH] box: fix formatting in session.push
@ 2020-03-27 10:28 Chris Sosnin
0 siblings, 0 replies; 4+ messages in thread
From: Chris Sosnin @ 2020-03-27 10:28 UTC (permalink / raw)
To: v.shpilevoy, tarantool-patches
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
---
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 | 41 ++++++++++++++++++++++-------------
src/box/lua/console.lua | 40 +++++++++++++++++++++++++---------
test/app-tap/console.test.lua | 22 ++++++++++++++++++-
3 files changed, 77 insertions(+), 26 deletions(-)
diff --git a/src/box/lua/console.c b/src/box/lua/console.c
index 603f7c11b..0185df90a 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_push");
+ 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..79b37bcdb 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,13 @@ local function current_output()
end
end
+-- Used by console_session_push.
+box_internal.format_lua_push = function(value)
+ local opts = current_output()["opts"]
+ value = format_lua_value(value, opts)
+ return '-- Push begin\n' .. value .. ';'
+end
+
--
-- Return current EOS value for currently
-- active output format.
@@ -426,8 +434,8 @@ local text_connection_mt = {
return self._socket:write(text)
end,
--
- -- Read a text from a socket until YAML terminator.
- -- @retval not nil Well formatted YAML.
+ -- Read a text from a socket until EOS.
+ -- @retval not nil Well formatted data.
-- @retval nil Error.
--
read = function(self)
@@ -459,6 +467,7 @@ local text_connection_mt = {
param_handlers[items[2]] == set_output then
local err, fmt, opts = parse_output(items[3])
if not err then
+ self.fmt = fmt
self.eos = output_eos[fmt]
end
end
@@ -470,6 +479,7 @@ local text_connection_mt = {
eval = function(self, text)
self:preprocess_eval(text)
text = text..'$EOF$\n'
+ local fmt = self.fmt or default_output_format["fmt"]
if not self:write(text) then
error(self:set_error())
end
@@ -478,14 +488,23 @@ 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 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 not string.startswith(rc, '-- Push begin') then
+ return rc
+ end
+ if self.print_f then
+ self.print_f(rc)
+ end
end
end
return error(self:set_error())
@@ -524,6 +543,7 @@ local function wrap_text_socket(connection, url, print_f)
port = url.service,
print_f = print_f,
eos = current_eos(),
+ fmt = current_output()["fmt"]
}, text_connection_mt)
--
-- Prepare the connection: setup EOS symbol
diff --git a/test/app-tap/console.test.lua b/test/app-tap/console.test.lua
index da5c1e71e..491811f84 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(77)
-- Start console and connect to it
local server = console.listen(CONSOLE_SOCKET)
@@ -39,6 +39,26 @@ 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({})\n')
+test:is(client:read(";"), '-- Push begin\n{};', "pushed message")
+test:is(client:read(";"), 'true;', "pushed message")
+
+client:write('\\set output lua,block\n')
+client:read(";")
+
+client:write('box.session.push({1})\n')
+test:is(client:read(";"), '-- Push begin\n{\n 1\n};', "pushed message")
+test:is(client:read(";"), 'true;', "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)
^ permalink raw reply [flat|nested] 4+ messages in thread* [Tarantool-patches] [PATCH] box: fix formatting in session.push
@ 2020-03-06 14:03 Chris Sosnin
2020-03-09 23:13 ` Vladislav Shpilevoy
0 siblings, 1 reply; 4+ messages in thread
From: Chris Sosnin @ 2020-03-06 14:03 UTC (permalink / raw)
To: tarantool-patches, v.shpilevoy
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,
+};
+
/*
* 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)
+{
+ 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");
+ 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 */
+ luaL_findtable(L, LUA_GLOBALSINDEX, "box.internal", 1);
+ lua_getfield(L, -1, "format_lua_value");
+ lua_pushvalue(L, -3);
+ lua_call(L, 1, 1);
}
- assert(rc == 1);
- assert(lua_isstring(L, -1));
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.
* @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
+ value = format_lua_value(value)
+ return value .. '\n'
+end
+
output_handlers["lua"] = function(status, opts, ...)
local collect = {}
--
diff --git a/test/app-tap/console.test.lua b/test/app-tap/console.test.lua
index da5c1e71e..71c482bf7 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({ { [\'field\'] = 100 }, { 1, 2, 3 }, \'abc\' })\n')
+test:is(client:read(";"), '{{field = 100}, {1, 2, 3}, "abc"}\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)
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [Tarantool-patches] [PATCH] box: fix formatting in session.push
2020-03-06 14:03 Chris Sosnin
@ 2020-03-09 23:13 ` Vladislav Shpilevoy
2020-03-13 14:56 ` Chris Sosnin
0 siblings, 1 reply; 4+ messages in thread
From: Vladislav Shpilevoy @ 2020-03-09 23:13 UTC (permalink / raw)
To: Chris Sosnin, tarantool-patches
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.
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [Tarantool-patches] [PATCH] box: fix formatting in session.push
2020-03-09 23:13 ` Vladislav Shpilevoy
@ 2020-03-13 14:56 ` Chris Sosnin
0 siblings, 0 replies; 4+ messages in thread
From: Chris Sosnin @ 2020-03-13 14:56 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
[-- Attachment #1: Type: text/plain, Size: 9744 bytes --]
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-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().
Done:
+enum output_format {
+ OUTPUT_FORMAT_YAML = 0,
+ OUTPUT_FORMAT_LUA_LINE,
+ OUTPUT_FORMAT_LUA_BLOCK,
+};
(Here I forgot about ‘opts’ 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 = {}', which will erase the format settings.
Fixed in v2.
>
>> + 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.
+ 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_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 == 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 = 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.
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 = 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.
I’m sorry, I was following the style from schema.lua:1289
box.internal.check_space_arg = check_space_arg -- for net.box
Fixed:
+-- Used by console_session_push.
+box_internal.format_lua_value = function(value)
>
>> + 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 <http://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’t come up with anything better than your suggestion,
at least now it works. It could be a temporary workaround, perhaps?
I will resend v2.
[-- Attachment #2: Type: text/html, Size: 85410 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-03-27 10:28 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-27 10:28 [Tarantool-patches] [PATCH] box: fix formatting in session.push Chris Sosnin
-- strict thread matches above, loose matches on Subject: below --
2020-03-06 14:03 Chris Sosnin
2020-03-09 23:13 ` Vladislav Shpilevoy
2020-03-13 14:56 ` Chris Sosnin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox