[tarantool-patches] [PATCH v2 0/5] box/console: Improve lua mode for remote console

[tarantool-patches] [PATCH v2 0/5] box/console: Improve lua mode for remote console

[Cyrill Gorcunov]

This series address a problem where we can hang a connection setting up
lua mode while server continue treating all data as having built-in yaml
end of string terminator.

We've a long discussion with @alexander.turenko about our test engine
and console eos in general and at first concluded that by default we
should not use ";" as lua terminator but continue using empty string
instead. Actually after thinking more I fear it won't work -- a user
might connect to remote console and this connection will hang because
server will reply with empty eos while we're using read() from socket
with terminator at the end of string. Moreover since our yaml serializer
*always* yield eos terminator I think we should do the same for lua
mode just to be consistent in architecture.

Still for convenience I added ability to zap lua terminator via
require('console').eos("") command. Thus users on local consoles
can drop ';' eos if they want to.

Next to test engine:

1) I think we should convert each test one by one, not the
   whole bunch of tests in a single pass

2) For this sake we need to send something like "\set output lua"
   to the server for a particular test

3) We need to describe somehow which mode a test uses. Sasha pointed
   that he has a string on top of "result" file where we could provide
   some kind of settings describing output mode. But I think maybe we
   could introduce .desc files in json format where we could add per-file
   settings instead (in addition to ini files). Putting test description
   in "result" file looks a bit inconvenient for me. Guys, what do you think?

Regardless of the test engine issues which I have to address I think the
series should be merged since it closes real bugs.

---

The following changes since commit 51d8e4c35ca1da255b4ebea9850ec2a84365e0a3:

  box/memtx: strip_core -- Warn on linux only (2019-08-29 22:27:15 +0300)

are available in the Git repository at:

  https://github.com/cyrillos/tarantool.git console-fix-1

for you to fetch changes up to 9236f677fcf1bde39f9eb004693865c40b449abc:

  box/console: Provide console.eos() api (2019-08-30 23:46:32 +0300)

----------------------------------------------------------------
Cyrill Gorcunov (5):
      box/console: Add mapping for direct symbols
      box/console: Add explicit output EOS mapping
      box/console: Refactor command handling
      box/console: Fix hang in remote console lua mode
      box/console: Provide console.eos() api

 src/box/lua/console.lua | 129 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 116 insertions(+), 13 deletions(-)

[tarantool-patches] [PATCH 1/5] box/console: Add mapping for direct symbols

[Cyrill Gorcunov]

Sole symbols (such as box.NULL) are not processed
by serpent "custom" symbols feature, since they
are not in table.

Thus we should process them separately. Without it
we have

 > require('console').set_default_output("lua,block")
 > ;
 > box.NULL
 > "cdata<void %*>: NULL";

instead of

 > box.NULL
 > box.NULL;

as it should be.

Part-of #3834
---
 src/box/lua/console.lua | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua
index 64086cf5b..6cbb6b405 100644
--- a/src/box/lua/console.lua
+++ b/src/box/lua/console.lua
@@ -46,6 +46,17 @@ output_handlers["lua"] = function(status, opts, ...)
     if not ... then
         return ""
     end
+    -- Map internal symbols in case if they are
+    -- not inside tables and serpent won't handle
+    -- them properly.
+    local map_direct_symbols = {
+        [box.NULL]      = 'box.NULL',
+    }
+    for k,v in pairs(map_direct_symbols) do
+        if k == ... then
+            return v .. output_eos["lua"]
+        end
+    end
     --
     -- Map internal symbols which serpent doesn't know
     -- about to a known representation.
-- 
2.20.1

[tarantool-patches] Re: [PATCH 1/5] box/console: Add mapping for direct symbols

[Cyrill Gorcunov]

On Tue, Sep 03, 2019 at 11:23:51AM +0300, Konstantin Osipov wrote:
> > +    for k,v in pairs(map_direct_symbols) do
> > +        if k == ... then
> > +            return v .. output_eos["lua"]
> > +        end
> > +    end
> 
> Please do not concatenate strings in a loop in Lua: it is O(n^2)
> of the total length, since there is a resize/realloc on each
> concatenation.

The cycle breaks once it matches, note the "return" statement here.
Or you meant something else?

[tarantool-patches] Re: [PATCH 1/5] box/console: Add mapping for direct symbols

[Cyrill Gorcunov]

On Tue, Sep 03, 2019 at 11:24:46AM +0300, Konstantin Osipov wrote:
> * Cyrill Gorcunov <gorcunov@gmail.com> [19/08/31 00:51]:
> > Sole symbols (such as box.NULL) are not processed
> > by serpent "custom" symbols feature, since they
> > are not in table.
> > 
> > Thus we should process them separately. Without it
> > we have
> 
> Aren't the patches out of order? This patch should go after the
> next one?

Crap, indeed. I rebased the series so many times so I managed
to squash the unirdered changes.

Guys, please don't apply the series, I'll resend.

[tarantool-patches] [PATCH 2/5] box/console: Add explicit output EOS mapping

[Cyrill Gorcunov]

This will help us to distinguish end of string/stream
in text protocols (such as remote console connection).

Note that we start printing ";" terminator for every
lua output. Actually current yaml output does the
same but inside console.c module.

And since lua output is yet a new feature in stabilization
phase we're safe to make such changes without breaking api.

Part-of #3834
---
 src/box/lua/console.lua | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua
index 6cbb6b405..5ca5cad1f 100644
--- a/src/box/lua/console.lua
+++ b/src/box/lua/console.lua
@@ -11,6 +11,7 @@ local urilib = require('uri')
 local yaml = require('yaml')
 local net_box = require('net.box')
 
+local LUA_TERM = ';'
 local YAML_TERM = '\n...\n'
 local PUSH_TAG_HANDLE = '!push!'
 
@@ -19,6 +20,10 @@ local PUSH_TAG_HANDLE = '!push!'
 -- compatibility reason.
 local default_output_format = { ["fmt"] = "yaml", ["opts"] = nil }
 local output_handlers = { }
+--
+-- End of streams/strings. They will allow to recognize blocks
+-- depending on output format.
+local output_eos = { ["yaml"] = YAML_TERM, ["lua"] = LUA_TERM }
 
 output_handlers["yaml"] = function(status, opts, ...)
     local err
@@ -42,9 +47,11 @@ end
 
 output_handlers["lua"] = function(status, opts, ...)
     --
-    -- Don't print nil if there is no data
+    -- If no data present at least EOS should be put,
+    -- otherwise wire readers won't be able to find
+    -- where end of string is.
     if not ... then
-        return ""
+        return output_eos["lua"]
     end
     -- Map internal symbols in case if they are
     -- not inside tables and serpent won't handle
@@ -75,9 +82,9 @@ output_handlers["lua"] = function(status, opts, ...)
         nocode = true,
     }
     if opts == "block" then
-        return serpent.block(..., serpent_opts)
+        return serpent.block(..., serpent_opts) .. output_eos["lua"]
     end
-    return serpent.line(..., serpent_opts)
+    return serpent.line(..., serpent_opts) .. output_eos["lua"]
 end
 
 local function output_verify_opts(fmt, opts)
-- 
2.20.1

[tarantool-patches] Re: [PATCH 2/5] box/console: Add explicit output EOS mapping

[Cyrill Gorcunov]

On Tue, Sep 03, 2019 at 11:27:25AM +0300, Konstantin Osipov wrote:
> >  
> > +local LUA_TERM = ';'
> 
> term is a "terminal", not "terminator", usually. Why not call them

We simply already had

YAML_TERM = '\n...\n'

in this file, so I tried to follow naming scheme. I don't mind
rename it.

> EOF or EOS? Why did you call the constant _TERM  and the array
> _eos? Why assign different names to the same thing?

Well, one of the reason is that I tried to minimize changes, but
I agree that putting everything into one place would be better.
Will rework.

> 
> > +    return serpent.line(..., serpent_opts) .. output_eos["lua"]
> Why put them in a table, given you never access them by a variable
> key? 

because we might extend this table with time when say add sql
formatting and etc.

[tarantool-patches] [PATCH 3/5] box/console: Refactor command handling

[Cyrill Gorcunov]

We will need to parse and verify "\set" commands
in remote console text-wire protocol thus to not
duplicate the code lets factor helper out for
reuse sake.

Part-of #3834
---
 src/box/lua/console.lua | 37 +++++++++++++++++++++++++++++++------
 1 file changed, 31 insertions(+), 6 deletions(-)

diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua
index 5ca5cad1f..006748d93 100644
--- a/src/box/lua/console.lua
+++ b/src/box/lua/console.lua
@@ -150,11 +150,16 @@ local function output_save(fmt, opts)
     }
 end
 
-local function format(status, ...)
+local function current_output()
     local d = box.session.storage.console_output_format
     if d == nil then
-        d = default_output_format
+        return default_output_format
     end
+    return d
+end
+
+local function format(status, ...)
+    local d = current_output()
     return output_handlers[d["fmt"]](status, d["opts"], ...)
 end
 
@@ -245,21 +250,40 @@ local operators = {
     q = quit
 }
 
-local function preprocess(storage, line)
+local function parse_operators(line)
     local items = {}
     for item in string.gmatch(line, '([^%s]+)') do
         items[#items + 1] = item
     end
     if #items == 0 then
-        return help_wrapper()
+        return 0, nil
     end
     if operators[items[1]] == nil then
+        return #items, nil
+    end
+    return #items, items
+end
+
+local function preprocess(storage, line)
+    local nr_items, items = parse_operators(line)
+    if nr_items == 0 then
+        return help_wrapper()
+    end
+    if items == nil then
         local msg = "Invalid command \\%s. Type \\help for help."
         return format(false, msg:format(items[1]))
     end
     return operators[items[1]](storage, unpack(items))
 end
 
+local function get_command(line)
+    -- All commands start with the escape symbol
+    if line:sub(1, 1) == '\\' then
+        return line:sub(2)
+    end
+    return nil
+end
+
 --
 -- Evaluate command on local instance
 --
@@ -267,8 +291,9 @@ local function local_eval(storage, line)
     if not line then
         return nil
     end
-    if line:sub(1, 1) == '\\' then
-        return preprocess(storage, line:sub(2))
+    local command = get_command(line)
+    if command then
+        return preprocess(storage, command)
     end
     if storage.language == 'sql' then
         return format(pcall(box.execute, line))
-- 
2.20.1

[tarantool-patches] [PATCH 4/5] box/console: Fix hang in remote console lua mode

[Cyrill Gorcunov]

If we change output mode on remote machine via
text-based session

 node a (server)
 ------
 require('console').listen('unix/:/tmp/X.sock')
 ...

 node b (client)
 ------
 require('console').connect('unix/:/tmp/X.sock')
 connected to unix/:/tmp/X.sock
 ...
 unix/:/tmp/X.sock> \set output lua

the client get stuck forever, it is because the text
wire protocol waits for yaml eos terminator which of
course never hit the peer, because lua mode uses own
one.

Thus to fix this problem we have to preprocess the text
we're passing to the server, just like we do in local
console. So we reuse command parsing code and remember
current output terminator in text_connection_mt instance.

Another problem is that named default output mode. There
could be a mixed environment where server operates in
default lua mode but client connects with yaml mode. To
break a tie we yield "\set output" command with current
output mode when establishing a connection. Since the
output format is per-session bound this doesn't affect
any other connections on a server.

Part-of #3834
---
 src/box/lua/console.lua | 50 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 47 insertions(+), 3 deletions(-)

diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua
index 006748d93..203dc668c 100644
--- a/src/box/lua/console.lua
+++ b/src/box/lua/console.lua
@@ -158,6 +158,27 @@ local function current_output()
     return d
 end
 
+local function current_eos()
+    local d = current_output()
+    return output_eos[d["fmt"]]
+end
+
+--
+-- Map output format into a "\set" command.
+local function output_cmd_string()
+    local d = current_output()
+    if d["fmt"] == "yaml" then
+        return "\\set output yaml"
+    elseif d["fmt"] == "lua" then
+        local cmd = "\\set output lua"
+        if d["opts"] == "block" then
+            cmd = cmd .. ",block"
+        end
+        return cmd
+    end
+    return ""
+end
+
 local function format(status, ...)
     local d = current_output()
     return output_handlers[d["fmt"]](status, d["opts"], ...)
@@ -341,15 +362,33 @@ local text_connection_mt = {
         -- @retval     nil Error.
         --
         read = function(self)
-            local ret = self._socket:read(YAML_TERM)
+            local ret = self._socket:read(self.eos)
             if ret and ret ~= '' then
                 return ret
             end
         end,
         --
+        -- The command might modify EOS so
+        -- we have to parse and preprocess it
+        -- first to not stuck in :read() forever.
+        preprocess_eval = function(self, text)
+            local command = get_command(text)
+            if command == nil then
+                return
+            end
+            local nr_items, items = parse_operators(command)
+            if nr_items == 3 then
+                local err, fmt, opts = output_parse(items[3])
+                if not err then
+                    self.eos = output_eos[fmt]
+                end
+            end
+        end,
+        --
         -- Write + Read.
         --
         eval = function(self, text)
+            self:preprocess_eval(text)
             text = text..'$EOF$\n'
             if not self:write(text) then
                 error(self:set_error())
@@ -404,9 +443,14 @@ local function wrap_text_socket(connection, url, print_f)
         host = url.host or 'localhost',
         port = url.service,
         print_f = print_f,
+        eos = current_eos(),
     }, text_connection_mt)
-    if not conn:write('require("console").delimiter("$EOF$")\n') or
-       not conn:read() then
+    local cmd_set_output = output_cmd_string() .. '\n'
+    local cmd_delimiter = 'require("console").delimiter("$EOF$")\n'
+    if not conn:write(cmd_set_output) or not conn:read() then
+        conn:set_error()
+    end
+    if not conn:write(cmd_delimiter) or not conn:read() then
         conn:set_error()
     end
     return conn
-- 
2.20.1

[tarantool-patches] Re: [PATCH 4/5] box/console: Fix hang in remote console lua mode

[Cyrill Gorcunov]

On Tue, Sep 03, 2019 at 10:00:02PM +0300, Konstantin Osipov wrote:
> * Cyrill Gorcunov <gorcunov@gmail.com> [19/08/31 00:51]:
> 
> The approach is fine, but I can not stop myself from nitpicking on
> the implementation consistency. Somehow I find this code rather hard to
> read, I tried to find out why, please see below.

This is perfectly fine, I send the series to gather comments and
fix the nits, so I really appreciate it!

> 
> >  
> > +local function current_eos()
> 
> No comment for the function.

ok

> 
> > +    local d = current_output()
> > +    return output_eos[d["fmt"]]
> 
> Why do you need a local variable? 

to make code shorter, the line

	return output_eos[current_output()["fmt"]]

looks too heavy for me. But I must confess I always
forget that we're in lua, where no optimization
would happen (I think) and it will cause jit
to allocate a variable first.

On the other hand the lua handling here should
not be a hot path and make code a bit slower
for beauty/readability sake looks like acceptable
trade off?

> > +end
> 
> Actually, the function is so simple, it is questionable why you
> need it at all. Can't you keep the value pre-computed in the
> object or in the metatable?

Since eos may be changed on the fly I think we can't
precompute it and keep somewhere.

> > +-- Map output format into a "\set" command.
> > +local function output_cmd_string()
> > +    local d = current_output()
> > +    if d["fmt"] == "yaml" then
> > +        return "\\set output yaml"
> > +    elseif d["fmt"] == "lua" then
> > +        local cmd = "\\set output lua"
> > +        if d["opts"] == "block" then
> > +            cmd = cmd .. ",block"
> > +        end
> > +        return cmd
> > +    end
> > +    return ""
> > +end
> 
> You could use string.format here:
> string.format("\\set output %s", format)

Will try to use, thanks!

> 
> The code would win if you pass in 'd' as an argument, rather than
> compute it locally, and give it a meaningful name.
> 
> Ideally, why not store the whole thing in self and compute once.

But the eos is per session value, can be changed dynamically.
Or you mean pre-compute this command for each output once
and just fetch it from the precomputed set?

> 
> > +        -- The command might modify EOS so
> > +        -- we have to parse and preprocess it
> > +        -- first to not stuck in :read() forever.
> > +        preprocess_eval = function(self, text)
> > +            local command = get_command(text)
> 
> Why get_command() but not get_current_eos()?

Because it generates \set output command, not eos
as a symbol. Probably better name it gen_eos_command.

> 
> > +            if command == nil then
> > +                return
> > +            end
> > +            local nr_items, items = parse_operators(command)
> > +            if nr_items == 3 then
> > +                local err, fmt, opts = output_parse(items[3])
> 
> why parse_operators but output_parse? 

Because I try to use output_ as a prefix to "output" engine,
ie everything related to output would have this prefix. Easier
for grep.

> > +                if not err then
> > +                    self.eos = output_eos[fmt]
> 
> why self.eos but global output_eos? 

because this is not global output but the value bound
to a particular session, you can have one server and
several clients, where each client have own eos
setting. Same applies to next your comment. So that
why we need to carry it into object instance.

> 
> 
> > +                end
> > +            end
> > +        end,
> 
> Why is this function part of the metatable? Can not it be a
> standalone function, not exported in the metatable?
> 
> > +    local cmd_set_output = output_cmd_string() .. '\n'
> > +    local cmd_delimiter = 'require("console").delimiter("$EOF$")\n'
> > +    if not conn:write(cmd_set_output) or not conn:read() then
> > +        conn:set_error()
> > +    end
> > +    if not conn:write(cmd_delimiter) or not conn:read() then
> >          conn:set_error()
> 
> Please add a comment for what's going on here.

OK

[tarantool-patches] Re: [PATCH 4/5] box/console: Fix hang in remote console lua mode

[Cyrill Gorcunov]

On Wed, Sep 04, 2019 at 09:46:57AM +0300, Konstantin Osipov wrote:
> * Cyrill Gorcunov <gorcunov@gmail.com> [19/09/03 22:45]:
> > > why self.eos but global output_eos? 
> > 
> > because this is not global output but the value bound
> > to a particular session, you can have one server and
> > several clients, where each client have own eos
> > setting. Same applies to next your comment. So that
> > why we need to carry it into object instance.
> 
> I mean, why the variable name is different?

Well, to point that it is related to "self" object
and don't collide with global output_eos when grepping
the source code. But I odn't have strong preference here
will rename. Thanks!

[tarantool-patches] [PATCH 5/5] box/console: Provide console.eos() api

[Cyrill Gorcunov]

The other modules would be able to find out which eos marker
is currently active. For example when reading replies from
remote server via text based console protocol.

@TarantoolBot document
Title: > require('console').eos()

Returns a string with currently active end of string marker.

@TarantoolBot document
Title: > require('console').eos("")

Sets end of string marker for appliable output modes.
YAML output mode does not support eos change. For Lua
mode it is possible to set eos to empty string (like
in example above) or any other symbol.

Note that default semicolon symbol is mandatory for
remote text consoles thus if get zapped the read
procedure from remote socket will not be possible and
connection get stuck forever. Use this ability with
extreme caution.

Part-of #3834
---
 src/box/lua/console.lua | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua
index 203dc668c..67238283b 100644
--- a/src/box/lua/console.lua
+++ b/src/box/lua/console.lua
@@ -163,6 +163,21 @@ local function current_eos()
     return output_eos[d["fmt"]]
 end
 
+local function console_eos(...)
+    if not ... then
+        return tostring(current_eos())
+    end
+    -- We can't allow to change yaml eos format because
+    -- it is a part of encoding standart, thus permit
+    -- only for modes where it is safe.
+    local d = current_output()
+    if d["fmt"] == "lua" then
+        output_eos["lua"] = ...
+    else
+        error("console.eos(): is immutable for output " .. d["fmt"])
+    end
+end
+
 --
 -- Map output format into a "\set" command.
 local function output_cmd_string()
@@ -780,6 +795,7 @@ package.loaded['console'] = {
     delimiter = delimiter;
     set_default_output = set_default_output;
     get_default_output = get_default_output;
+    eos = console_eos;
     ac = ac;
     connect = connect;
     listen = listen;
-- 
2.20.1

[tarantool-patches] Re: [PATCH 5/5] box/console: Provide console.eos() api

[Cyrill Gorcunov]

On Tue, Sep 03, 2019 at 10:00:57PM +0300, Konstantin Osipov wrote:
> * Cyrill Gorcunov <gorcunov@gmail.com> [19/08/31 00:51]:
> >  
> > +local function console_eos(...)
> 
> Please add a comment.

OK

> > +    if not ... then
> > +        return tostring(current_eos())
> > +    end
> > +    -- We can't allow to change yaml eos format because
> > +    -- it is a part of encoding standart, thus permit
> > +    -- only for modes where it is safe.
> > +    local d = current_output()
> > +    if d["fmt"] == "lua" then
> > +        output_eos["lua"] = ...
> 
> Why do you need variadic arguments here?

because this function is get/set, it might be
called without arguments to fetch current eos
setting, and with argument to setup new eos.

[tarantool-patches] Re: [PATCH 5/5] box/console: Provide console.eos() api

[Cyrill Gorcunov]

On Wed, Sep 04, 2019 at 09:47:25AM +0300, Konstantin Osipov wrote:
> * Cyrill Gorcunov <gorcunov@gmail.com> [19/09/03 23:31]:
> > because this function is get/set, it might be
> > called without arguments to fetch current eos
> > setting, and with argument to setup new eos.
> 
> this is fine, you declare it with one argument and it will get nil
> value in case it is not set.

Sure, will update then. Thanks!