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

[tarantool-patches] [PATCH v3 0/6] 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.

v3:
 - address Kostya comments

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 console-fix-2

for you to fetch changes up to 6c07d1f174eae99fcb86120ffd07f69e2dc6e4d2:

  box/console: Rename output_parse to parse_output (2019-09-06 00:03:23 +0300)

----------------------------------------------------------------
Cyrill Gorcunov (6):
      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
      box/console: Rename output_parse to parse_output

 src/box/lua/console.lua | 149 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 132 insertions(+), 17 deletions(-)

[tarantool-patches] [PATCH 1/6] 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..22bafab3a 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
+        end
+    end
     --
     -- Map internal symbols which serpent doesn't know
     -- about to a known representation.
-- 
2.20.1

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

[Sergey Ostanevich]

[-- Attachment #1: Type: text/plain, Size: 1536 bytes --]

Cyrill,

Why not to move the map_direct_symbols to the module level, so that creation and 
fill of the table will happens only once and not at every call to the output handler?


>Пятница,  6 сентября 2019, 0:28 +03:00 от Cyrill Gorcunov <gorcunov@gmail.com>:
>
>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..22bafab3a 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
>+        end
>+    end
>     --
>     -- Map internal symbols which serpent doesn't know
>     -- about to a known representation.
>-- 
>2.20.1
>
>


-- 
Sergey Ostanevich

[-- Attachment #2: Type: text/html, Size: 2268 bytes --]

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

[Cyrill Gorcunov]

On Mon, Sep 09, 2019 at 06:11:44PM +0300, Sergey Ostanevich wrote:
>    Cyrill,
> 
>    Why not to move the map_direct_symbols to the module level, so that
>    creation and
>    fill of the table will happens only once and not at every call to the
>    output handler?

Well, initially I thought to keep such things inside serializer helper
so that if we need we will easily extend this table and the change
will be inside function context. I must admit I think all this comes
from a strong habbit of working with compilers where such thing would
be simply optimized. That said I agree we should do so.

I think this issue could be addressed on top as a separate patch?
Just to not resend the whole series.

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

[Sergey Ostanevich]

[-- Attachment #1: Type: text/plain, Size: 931 bytes --]



Sure, separate patch is OK if you do not envision any other problem with such a move.


Sent from Mail.ru app for iOS


Monday, 9 September 2019, 18:54 +0300 from gorcunov@gmail.com  <gorcunov@gmail.com>:
>On Mon, Sep 09, 2019 at 06:11:44PM +0300, Sergey Ostanevich wrote:
>>    Cyrill,
>> 
>>    Why not to move the map_direct_symbols to the module level, so that
>>    creation and
>>    fill of the table will happens only once and not at every call to the
>>    output handler?
>
>Well, initially I thought to keep such things inside serializer helper
>so that if we need we will easily extend this table and the change
>will be inside function context. I must admit I think all this comes
>from a strong habbit of working with compilers where such thing would
>be simply optimized. That said I agree we should do so.
>
>I think this issue could be addressed on top as a separate patch?
>Just to not resend the whole series.

[-- Attachment #2: Type: text/html, Size: 1867 bytes --]

[tarantool-patches] [PATCH 2/6] 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.

We keep the eos values in table because we will need to
fetch eos from active format in later patches, thus it
is easier to use dictionary here. Moreover in future
we might need to extend this eos table for other
serializers.

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

diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua
index 22bafab3a..5430e6787 100644
--- a/src/box/lua/console.lua
+++ b/src/box/lua/console.lua
@@ -11,7 +11,6 @@ local urilib = require('uri')
 local yaml = require('yaml')
 local net_box = require('net.box')
 
-local YAML_TERM = '\n...\n'
 local PUSH_TAG_HANDLE = '!push!'
 
 --
@@ -20,6 +19,11 @@ local PUSH_TAG_HANDLE = '!push!'
 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"] = '\n...\n', ["lua"] = ';' }
+
 output_handlers["yaml"] = function(status, opts, ...)
     local err
     if status then
@@ -42,9 +46,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
@@ -54,7 +60,7 @@ output_handlers["lua"] = function(status, opts, ...)
     }
     for k,v in pairs(map_direct_symbols) do
         if k == ... then
-            return v
+            return v .. output_eos["lua"]
         end
     end
     --
@@ -75,9 +81,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)
@@ -309,7 +315,7 @@ local text_connection_mt = {
         -- @retval     nil Error.
         --
         read = function(self)
-            local ret = self._socket:read(YAML_TERM)
+            local ret = self._socket:read(output_eos["yaml"])
             if ret and ret ~= '' then
                 return ret
             end
-- 
2.20.1

[tarantool-patches] [PATCH 3/6] 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
---
v3:
 - More comments

 src/box/lua/console.lua | 43 +++++++++++++++++++++++++++++++++++------
 1 file changed, 37 insertions(+), 6 deletions(-)

diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua
index 5430e6787..04cca35ca 100644
--- a/src/box/lua/console.lua
+++ b/src/box/lua/console.lua
@@ -149,11 +149,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
 
@@ -244,21 +249,46 @@ local operators = {
     q = quit
 }
 
-local function preprocess(storage, line)
+--
+-- Generate command arguments from the line to
+-- be passed into command handlers.
+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
+
+--
+-- Preprocess a command via operator helpers.
+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
 
+--
+-- Return a command without a leading slash.
+local function get_command(line)
+    if line:sub(1, 1) == '\\' then
+        return line:sub(2)
+    end
+    return nil
+end
+
 --
 -- Evaluate command on local instance
 --
@@ -266,8 +296,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/6] 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
---
v3:
 - Use current_output() helper in current_eos()
   instead of local variable
 - Rename output_cmd_string() to output_to_cmd_string()
 - Use string.format in output_to_cmd_string
 - Leave preprocess_eval inside metatable because moving
   this method out of text_connection_mt is really confusing,
   everything specific to console text connection should sit
   inside text_connection_mt for better design
 - More comments

 src/box/lua/console.lua | 57 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 54 insertions(+), 3 deletions(-)

diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua
index 04cca35ca..a6ff0e659 100644
--- a/src/box/lua/console.lua
+++ b/src/box/lua/console.lua
@@ -157,6 +157,24 @@ local function current_output()
     return d
 end
 
+--
+-- Return current EOS value for currently
+-- active output format.
+local function current_eos()
+    return output_eos[current_output()["fmt"]]
+end
+
+--
+-- Map output format descriptor into a "\set" command.
+local function output_to_cmd_string(desc)
+    if desc["opts"] then
+        string.format("\\set output %s,%s", desc["fmt"], desc["opts"])
+    else
+        string.format("\\set output %s", desc["fmt"])
+    end
+    return cmd
+end
+
 local function format(status, ...)
     local d = current_output()
     return output_handlers[d["fmt"]](status, d["opts"], ...)
@@ -346,15 +364,39 @@ local text_connection_mt = {
         -- @retval     nil Error.
         --
         read = function(self)
-            local ret = self._socket:read(output_eos["yaml"])
+            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.
+        --
+        -- Same time the EOS is per connection
+        -- value so we keep it inside the metatable
+        -- instace, and because we can't use same
+        -- name as output_eos() function we name
+        -- it simplier as self.eos.
+        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())
@@ -409,9 +451,18 @@ 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
+    --
+    -- Prepare the connection: setup EOS symbol
+    -- by executing the \set command on remote machine
+    -- explicitly, and then setup a delimiter.
+    local cmd_set_output = output_to_cmd_string(current_output()) .. '\n'
+    if not conn:write(cmd_set_output) or not conn:read() then
+        conn:set_error()
+    end
+    local cmd_delimiter = 'require("console").delimiter("$EOF$")\n'
+    if not conn:write(cmd_delimiter) or not conn:read() then
         conn:set_error()
     end
     return conn
-- 
2.20.1

[tarantool-patches] [PATCH 5/6] 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(+)
v3:
 - Don't use ... in arguments but rather name it eos_value

diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua
index a6ff0e659..c1e8daaf8 100644
--- a/src/box/lua/console.lua
+++ b/src/box/lua/console.lua
@@ -164,6 +164,21 @@ local function current_eos()
     return output_eos[current_output()["fmt"]]
 end
 
+local function console_eos(eos_value)
+    if not eos_value 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"] = eos_value
+    else
+        error("console.eos(): is immutable for output " .. d["fmt"])
+    end
+end
+
 --
 -- Map output format descriptor into a "\set" command.
 local function output_to_cmd_string(desc)
@@ -792,6 +807,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] [PATCH 6/6] box/console: Rename output_parse to parse_output

[Cyrill Gorcunov]

To be consistent in naming scheme, for example
we already have parse_operators.

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

diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua
index c1e8daaf8..75cad9de2 100644
--- a/src/box/lua/console.lua
+++ b/src/box/lua/console.lua
@@ -99,7 +99,7 @@ local function output_verify_opts(fmt, opts)
     return nil
 end
 
-local function output_parse(value)
+local function parse_output(value)
     local fmt, opts
     if value:match("([^,]+),([^,]+)") ~= nil then
         fmt, opts = value:match("([^,]+),([^,]+)")
@@ -123,7 +123,7 @@ local function set_default_output(value)
     if value == nil then
         error("Nil output value passed")
     end
-    local err, fmt, opts = output_parse(value)
+    local err, fmt, opts = parse_output(value)
     if err then
         error(err)
     end
@@ -234,7 +234,7 @@ local function set_language(storage, value)
 end
 
 local function set_output(storage, value)
-    local err, fmt, opts = output_parse(value)
+    local err, fmt, opts = parse_output(value)
     if err then
         return error(err)
     end
@@ -401,7 +401,7 @@ local text_connection_mt = {
             end
             local nr_items, items = parse_operators(command)
             if nr_items == 3 then
-                local err, fmt, opts = output_parse(items[3])
+                local err, fmt, opts = parse_output(items[3])
                 if not err then
                     self.eos = output_eos[fmt]
                 end
-- 
2.20.1