[Tarantool-patches] [PATCH] box: fix formatting in session.push
Chris Sosnin
k.sosnin at tarantool.org
Fri Mar 13 17:56:16 MSK 2020
Hi! Thank you for the review!
> On 10 Mar 2020, at 02:13, Vladislav Shpilevoy <v.shpilevoy at 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20200313/51189e95/attachment.html>
More information about the Tarantool-patches
mailing list