[PATCH 0/3] console: do not use netbox for console text connections

[PATCH 0/3] console: do not use netbox for console text connections

[Vladislav Shpilevoy]

Branch: http://github.com/tarantool/tarantool/tree/gh-2677-prepare-console-for-push
Issue: https://github.com/tarantool/tarantool/issues/2677

At first, netbox text protocol support is full of crutches, slows
down a binary protocol support, and complicates a netbox
implementation.

At second, in #2677 text protocol will have two push types: print
pushes, and YAML pushes. It can not be clearly supported in
netbox, because it will contain only one callback to process
pushed messages. Attempt to fit 3 push types (print, yaml, binary)
into netbox will lead to even more complicated netbox.

The text protocol must be supported by console only, and directly
via sockets.

Vladislav Shpilevoy (3):
  netbox: allow to create a netbox connection from existing socket
  console: do not use netbox for console text connections
  netbox: deprecate console support

 src/box/lua/console.lua   | 178 +++++++++++++++++++++++++++++++++++++---------
 src/box/lua/net_box.lua   |  91 +++++++++++++++++-------
 test/box/net.box.result   |  78 +++++++++++++++++++-
 test/box/net.box.test.lua |  33 ++++++++-
 4 files changed, 318 insertions(+), 62 deletions(-)

-- 
2.14.3 (Apple Git-98)

[PATCH 1/3] netbox: allow to create a netbox connection from existing socket

[Vladislav Shpilevoy]

It is needed to create a binary console connection, when a
socket is already created and a greeting is read and decoded.
---
 src/box/lua/net_box.lua   | 85 ++++++++++++++++++++++++++++++++---------------
 test/box/net.box.result   | 78 +++++++++++++++++++++++++++++++++++++++++--
 test/box/net.box.test.lua | 33 ++++++++++++++++--
 3 files changed, 166 insertions(+), 30 deletions(-)

diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index 87c8c548b..f3f69f52f 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -121,7 +121,8 @@ local function next_id(id) return band(id + 1, 0x7FFFFFFF) end
 -- Suggestion for callback writers: sleep a few secs before approving
 -- reconnect.
 --
-local function create_transport(host, port, user, password, callback)
+local function create_transport(host, port, user, password, callback,
+                                existing_connection, greeting)
     -- check / normalize credentials
     if user == nil and password ~= nil then
         box.error(E_PROC_LUA, 'net.box: user is not defined')
@@ -368,26 +369,34 @@ local function create_transport(host, port, user, password, callback)
     -- in this function, a connection could not be deleted.
     --
     protocol_sm = function ()
+        local err, msg
         local tm_begin, tm = fiber.clock(), callback('fetch_connect_timeout')
-        connection = socket.tcp_connect(host, port, tm)
-        if connection == nil then
-            return error_sm(E_NO_CONNECTION, errno.strerror(errno()))
-        end
-        local size = IPROTO_GREETING_SIZE
-        local err, msg = send_and_recv(size, tm - (fiber.clock() - tm_begin))
-        if err then
-            return error_sm(err, msg)
-        end
-        local g = decode_greeting(ffi.string(recv_buf.rpos, size))
-        recv_buf.rpos = recv_buf.rpos + size
-        if not g then
-            return error_sm(E_NO_CONNECTION, 'Can\'t decode handshake')
+        if existing_connection then
+            connection = existing_connection
+            existing_connection = nil
+            assert(greeting)
+            assert(greeting.protocol ~= 'Lua console')
+        else
+            connection = socket.tcp_connect(host, port, tm)
+            if connection == nil then
+                return error_sm(E_NO_CONNECTION, errno.strerror(errno()))
+            end
+            local size = IPROTO_GREETING_SIZE
+            err, msg = send_and_recv(size, tm - (fiber.clock() - tm_begin))
+            if err then
+                return error_sm(err, msg)
+            end
+            greeting = decode_greeting(ffi.string(recv_buf.rpos, size))
+            recv_buf.rpos = recv_buf.rpos + size
+            if not greeting then
+                return error_sm(E_NO_CONNECTION, 'Can\'t decode handshake')
+            end
         end
-        err, msg = callback('handshake', g)
+        err, msg = callback('handshake', greeting)
         if err then
             return error_sm(err, msg)
         end
-        if g.protocol == 'Lua console' then
+        if greeting.protocol == 'Lua console' then
             local setup_delimiter = 'require("console").delimiter("$EOF$")\n'
             method_codec.inject(send_buf, nil, nil, setup_delimiter)
             local err, response = send_and_recv_console()
@@ -399,10 +408,11 @@ local function create_transport(host, port, user, password, callback)
             local rid = next_request_id
             set_state('active')
             return console_sm(rid)
-        elseif g.protocol == 'Binary' then
-            return iproto_auth_sm(g.salt)
+        elseif greeting.protocol == 'Binary' then
+            return iproto_auth_sm(greeting.salt)
         else
-            return error_sm(E_NO_CONNECTION, 'Unknown protocol: ' .. g.protocol)
+            return error_sm(E_NO_CONNECTION,
+                            'Unknown protocol: '..greeting.protocol)
         end
     end
 
@@ -541,14 +551,16 @@ end
 -- Now it is necessary to have a strong ref to callback somewhere or
 -- it is GC-ed prematurely. We wrap close() method, stashing the
 -- ref in an upvalue (close() performance doesn't matter much.)
-local create_transport = function(host, port, user, password, callback)
+local create_transport = function(host, port, user, password, callback,
+                                  existing_connection, greeting)
     local weak_refs = setmetatable({callback = callback}, {__mode = 'v'})
     local function weak_callback(...)
         local callback = weak_refs.callback
         if callback then return callback(...) end
     end
-    local transport = create_transport(host, port, user,
-                                       password, weak_callback)
+    local transport = create_transport(host, port, user, password,
+                                       weak_callback, existing_connection,
+                                       greeting)
     local transport_close = transport.close
     local gc_hook = ffi.gc(ffi.new('char[1]'), function()
         pcall(transport_close)
@@ -612,8 +624,7 @@ local console_mt = {
 
 local space_metatable, index_metatable
 
-local function connect(...)
-    local host, port, opts = parse_connect_params(...)
+local function do_connect(host, port, opts, existing_connection, greeting)
     local user, password = opts.user, opts.password; opts.password = nil
     local last_reconnect_error
     local remote = {host = host, port = port, opts = opts, state = 'initial'}
@@ -679,7 +690,8 @@ local function connect(...)
     remote._on_schema_reload = trigger.new("on_schema_reload")
     remote._on_disconnect = trigger.new("on_disconnect")
     remote._on_connect = trigger.new("on_connect")
-    remote._transport = create_transport(host, port, user, password, callback)
+    remote._transport = create_transport(host, port, user, password, callback,
+                                         existing_connection, greeting)
     remote._transport.connect()
     if opts.wait_connected ~= false then
         remote._transport.wait_state('active', tonumber(opts.wait_connected))
@@ -687,6 +699,25 @@ local function connect(...)
     return remote
 end
 
+local function connect(...)
+    local host, port, opts = parse_connect_params(...)
+    return do_connect(host, port, opts)
+end
+
+local function wrap_socket(connection, greeting, url, opts)
+    if connection == nil or type(greeting) ~= 'table' then
+        error('Usage: netbox.wrap_socket(socket, greeting, [opts])')
+    end
+    if greeting.protocol == 'Lua console' then
+        error('Can not wrap console socket')
+    end
+    opts = opts or {}
+    if not opts.user and not opts.password then
+        opts.user, opts.password = url.login, url.password
+    end
+    return do_connect(url.host, url.service, opts, connection, greeting)
+end
+
 local function check_remote_arg(remote, method)
     if type(remote) ~= 'table' then
         local fmt = 'Use remote:%s(...) instead of remote.%s(...):'
@@ -1112,7 +1143,9 @@ end
 local this_module = {
     create_transport = create_transport,
     connect = connect,
-    new = connect -- Tarantool < 1.7.1 compatibility
+    new = connect, -- Tarantool < 1.7.1 compatibility,
+    decode_greeting = internal.decode_greeting,
+    wrap_socket = wrap_socket,
 }
 
 function this_module.timeout(timeout, ...)
diff --git a/test/box/net.box.result b/test/box/net.box.result
index 46d85b327..cb9410085 100644
--- a/test/box/net.box.result
+++ b/test/box/net.box.result
@@ -2292,12 +2292,86 @@ weak.c
 ---
 - null
 ...
-box.schema.user.revoke('guest', 'execute', 'universe')
+--
+-- gh-2677: netbox supports console connections, that complicates
+-- both console and netbox. It was necessary because before a
+-- connection is established, a console does not known is it
+-- binary or text protocol, and netbox could not be created from
+-- existing socket.
+--
+box.schema.user.grant('guest','read,write,execute','universe')
+---
+...
+urilib = require('uri')
+---
+...
+uri = urilib.parse(tostring(box.cfg.listen))
+---
+...
+s = socket.tcp_connect(uri.host, uri.service)
+---
+...
+greeting = s:read({chunk = 128})
+---
+...
+greeting = net.decode_greeting(greeting)
+---
+...
+c = net.wrap_socket(s, greeting, uri)
+---
+...
+c.state
+---
+- active
+...
+a = 100
+---
+...
+function kek(args) return {1, 2, 3, args} end
+---
+...
+c:eval('a = 200')
+---
+...
+a
+---
+- 200
+...
+c:call('kek', {300})
+---
+- [1, 2, 3, 300]
+...
+s = box.schema.create_space('test')
+---
+...
+pk = s:create_index('pk')
+---
+...
+c:reload_schema()
+---
+...
+c.space.test:replace{1}
+---
+- [1]
+...
+c.space.test:get{1}
+---
+- [1]
+...
+c.space.test:delete{1}
+---
+- [1]
+...
+s:drop()
 ---
 ...
 c:close()
 ---
 ...
-c = nil
+c.state
+---
+- closed
+...
+box.schema.user.revoke('guest', 'read,write,execute', 'universe')
 ---
 ...
diff --git a/test/box/net.box.test.lua b/test/box/net.box.test.lua
index 87e26f84c..487401514 100644
--- a/test/box/net.box.test.lua
+++ b/test/box/net.box.test.lua
@@ -937,6 +937,35 @@ collectgarbage('collect')
 -- connection is deleted by 'collect'.
 weak.c
 
-box.schema.user.revoke('guest', 'execute', 'universe')
+--
+-- gh-2677: netbox supports console connections, that complicates
+-- both console and netbox. It was necessary because before a
+-- connection is established, a console does not known is it
+-- binary or text protocol, and netbox could not be created from
+-- existing socket.
+--
+box.schema.user.grant('guest','read,write,execute','universe')
+urilib = require('uri')
+uri = urilib.parse(tostring(box.cfg.listen))
+s = socket.tcp_connect(uri.host, uri.service)
+greeting = s:read({chunk = 128})
+greeting = net.decode_greeting(greeting)
+c = net.wrap_socket(s, greeting, uri)
+c.state
+
+a = 100
+function kek(args) return {1, 2, 3, args} end
+c:eval('a = 200')
+a
+c:call('kek', {300})
+s = box.schema.create_space('test')
+pk = s:create_index('pk')
+c:reload_schema()
+c.space.test:replace{1}
+c.space.test:get{1}
+c.space.test:delete{1}
+s:drop()
 c:close()
-c = nil
+c.state
+
+box.schema.user.revoke('guest', 'read,write,execute', 'universe')
-- 
2.14.3 (Apple Git-98)

[PATCH 2/3] console: do not use netbox for console text connections

[Vladislav Shpilevoy]

Netbox console support complicates both netbox and console. Lets
use sockets directly for text protocol.

Part of #2677
---
 src/box/lua/console.lua | 178 +++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 146 insertions(+), 32 deletions(-)

diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua
index d49cf42be..19ff12876 100644
--- a/src/box/lua/console.lua
+++ b/src/box/lua/console.lua
@@ -8,6 +8,9 @@ local log = require('log')
 local errno = require('errno')
 local urilib = require('uri')
 local yaml = require('yaml')
+local net_box = require('net.box')
+
+local YAML_TERM = '\n...\n'
 
 -- admin formatter must be able to encode any Lua variable
 local formatter = yaml.new()
@@ -29,7 +32,7 @@ local function format(status, ...)
     if status then
         local count = select('#', ...)
         if count == 0 then
-            return "---\n...\n"
+            return "---"..YAML_TERM
         end
         local res = {}
         for i=1,count,1 do
@@ -76,26 +79,122 @@ local function eval(line)
     return local_eval(nil, line)
 end
 
+local text_connection_mt = {
+    __index = {
+        --
+        -- Close underlying socket.
+        --
+        close = function(self)
+            self._socket:close()
+        end,
+        --
+        -- Write a text into a socket.
+        -- @param test Text to send.
+        -- @retval not nil Bytes sent.
+        -- @retval     nil Error.
+        --
+        write = function(self, text)
+            -- It is the hack to protect from SIGPIPE on send in
+            -- a socket, that is actually closed. If a socket is
+            -- readable and read() returns nothing the the socket
+            -- is closed, and writing into it will raise SIGPIPE.
+            if self._socket:readable(0) then
+                local rc = self._socket:recv(1)
+                if not rc or rc == '' then
+                    return nil
+                else
+                    -- But if it was literally readable, the
+                    -- single read byte can not be put back, and
+                    -- must be stored somewhere, until console
+                    -- do read().
+                    self.buffer = self.buffer..rc
+                end
+            end
+            return self._socket:send(text)
+        end,
+        --
+        -- Read a text from a socket until YAML terminator.
+        -- @retval not nil Well formatted YAML.
+        -- @retval     nil Error.
+        --
+        read = function(self)
+            local ret = self._socket:read(YAML_TERM)
+            if ret and ret ~= '' then
+                ret = (self.buffer or '')..ret
+                self.buffer = nil
+                return ret
+            end
+        end,
+        --
+        -- Write + Read.
+        --
+        eval = function(self, text)
+            text = text..'$EOF$\n'
+            if self:write(text) then
+                local rc = self:read()
+                if rc then
+                    return rc
+                end
+            end
+            error(self:set_error())
+        end,
+        --
+        -- Make the connection be in error state, set error
+        -- message.
+        -- @retval Error message.
+        --
+        set_error = function(self)
+            self.state = 'error'
+            self.error = self._socket:error()
+            if not self.error then
+                self.error = 'Peer closed'
+            end
+            return self.error
+        end,
+    }
+}
+
+--
+-- Wrap an existing socket with text Read-Write API inside a
+-- netbox-like object.
+-- @param connection Socket to wrap.
+-- @param url Parsed destination URL.
+-- @retval nil, err Error, and err contains an error message.
+-- @retval  not nil Netbox-like object.
+--
+local function wrap_text_socket(connection, url)
+    local conn = setmetatable({
+        _socket = connection,
+        state = 'active',
+        host = url.host or 'localhost',
+        port = url.service,
+        buffer = nil,
+    }, text_connection_mt)
+    if not conn:write('require("console").delimiter("$EOF$")\n') or
+       not conn:read() then
+        return nil, conn:set_error()
+    end
+    return conn
+end
+
 --
 -- Evaluate command on remote instance
 --
 local function remote_eval(self, line)
-    if not line or self.remote.state ~= 'active' then
-        local err = self.remote.error
-        self.remote:close()
-        self.remote = nil
-        -- restore local REPL mode
-        self.eval = nil
-        self.prompt = nil
-        self.completion = nil
-        pcall(self.on_client_disconnect, self)
-        return (err and format(false, err)) or ''
+    if line and self.remote.state == 'active' then
+        local ok, res = pcall(self.remote.eval, self.remote, line)
+        if self.remote.state == 'active' then
+            return ok and res or format(false, res)
+        end
     end
-    --
-    -- execute line
-    --
-    local ok, res = pcall(self.remote.eval, self.remote, line)
-    return ok and res or format(false, res)
+    local err = self.remote.error
+    self.remote:close()
+    self.remote = nil
+    self.eval = nil
+    self.prompt = nil
+    self.completion = nil
+    pcall(self.on_client_disconnect, self)
+    return (err and format(false, err)) or ''
 end
 
 --
@@ -285,12 +384,7 @@ end
 --
 -- Connect to remove instance
 --
-local netbox_connect
 local function connect(uri, opts)
-    if not netbox_connect then -- workaround the broken loader
-        netbox_connect = require('net.box').connect
-    end
-
     opts = opts or {}
 
     local self = fiber.self().storage.console
@@ -306,18 +400,38 @@ local function connect(uri, opts)
         error('Usage: console.connect("[login:password@][host:]port")')
     end
 
-    -- connect to remote host
+    local ts = fiber.clock()
+    local connection = socket.tcp_connect(u.host, u.service, opts.timeout)
+    if connection == nil then
+        log.verbose(errno.strerror(errno()))
+        box.error(box.error.NO_CONNECTION)
+    end
+    local left_time
+    if opts.timeout then
+        left_time = opts.timeout - (fiber.clock() - ts)
+    end
+    local greeting = connection:read({chunk = 128}, left_time)
+    if not greeting then
+        log.verbose(errno.strerror(errno()))
+        box.error(box.error.NO_CONNECTION)
+    end
+    greeting = net_box.decode_greeting(greeting)
+    if not greeting then
+        log.verbose("Can't decode handshake")
+        box.error(box.error.NO_CONNECTION)
+    end
     local remote
-    remote = netbox_connect(u.host, u.service, {
-        user = u.login, password = u.password,
-        console = true, connect_timeout = opts.timeout
-    })
-    remote.host, remote.port = u.host or 'localhost', u.service
-
-    -- run disconnect trigger if connection failed
-    if not remote:is_connected() then
-        pcall(self.on_client_disconnect, self)
-        error('Connection is not established: '..remote.error)
+    if greeting.protocol == 'Lua console' then
+        remote = wrap_text_socket(connection, u)
+    else
+        remote = net_box.wrap_socket(connection, greeting, u)
+        if not remote.host then
+            remote.host = 'localhost'
+        end
+        local old_eval = remote.eval
+        remote.eval = function(con, line)
+            return old_eval(con, 'return require("console").eval(...)', {line})
+        end
     end
 
     -- check connection && permissions
-- 
2.14.3 (Apple Git-98)

[PATCH 3/3] netbox: deprecate console support

[Vladislav Shpilevoy]

Print warning about that. After a while the cosole support will
be deleted from netbox.
---
 src/box/lua/net_box.lua | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index f3f69f52f..cf1a04237 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -396,7 +396,10 @@ local function create_transport(host, port, user, password, callback,
         if err then
             return error_sm(err, msg)
         end
+        -- @deprecated since 1.10
         if greeting.protocol == 'Lua console' then
+            log.warn("Netbox text protocol support is deprecated since 1.10, "..
+                     "please use require('console').connect() instead")
             local setup_delimiter = 'require("console").delimiter("$EOF$")\n'
             method_codec.inject(send_buf, nil, nil, setup_delimiter)
             local err, response = send_and_recv_console()
@@ -673,7 +676,10 @@ local function do_connect(host, port, opts, existing_connection, greeting)
             end
         end
     end
+    -- @deprecated since 1.10
     if opts.console then
+        log.warn("Netbox console API is deprecated since 1.10, please use "..
+                 "require('console').connect() instead")
         setmetatable(remote, console_mt)
     else
         setmetatable(remote, remote_mt)
-- 
2.14.3 (Apple Git-98)

Re: [tarantool-patches] [PATCH 1/3] netbox: allow to create a netbox connection from existing socket

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [18/03/22 22:18]:

The patch generally looks good to me, I can only nitpick on new
things. 

How about renaming do_connect to connect_impl and wrap_connect to
wrap_socket or simply wrap, or maybe bless or imbue?

> +        if existing_connection then
> +            connection = existing_connection
> +            existing_connection = nil

Why is it necessary to set it to nil?

> +            assert(greeting)
> +            assert(greeting.protocol ~= 'Lua console')

Please keep in mind that in Lua there are no asserts - it's a
runtime check.

> +        else
> +            connection = socket.tcp_connect(host, port, tm)
> +            if connection == nil then
> +                return error_sm(E_NO_CONNECTION, errno.strerror(errno()))
> +            end
> +            local size = IPROTO_GREETING_SIZE
> +            err, msg = send_and_recv(size, tm - (fiber.clock() - tm_begin))
> +            if err then
> +                return error_sm(err, msg)
> +            end
> +            greeting = decode_greeting(ffi.string(recv_buf.rpos, size))
> +            recv_buf.rpos = recv_buf.rpos + size
> +            if not greeting then
> +                return error_sm(E_NO_CONNECTION, 'Can\'t decode handshake')
> +            end
>          end
> -        err, msg = callback('handshake', g)
> +        err, msg = callback('handshake', greeting)
>          if err then
>              return error_sm(err, msg)
>          end
> -        if g.protocol == 'Lua console' then
> +        if greeting.protocol == 'Lua console' then
>              local setup_delimiter = 'require("console").delimiter("$EOF$")\n'
>              method_codec.inject(send_buf, nil, nil, setup_delimiter)
>              local err, response = send_and_recv_console()
> @@ -399,10 +408,11 @@ local function create_transport(host, port, user, password, callback)
>              local rid = next_request_id
>              set_state('active')
>              return console_sm(rid)
> -        elseif g.protocol == 'Binary' then
> -            return iproto_auth_sm(g.salt)
> +        elseif greeting.protocol == 'Binary' then
> +            return iproto_auth_sm(greeting.salt)
>          else
> -            return error_sm(E_NO_CONNECTION, 'Unknown protocol: ' .. g.protocol)
> +            return error_sm(E_NO_CONNECTION,
> +                            'Unknown protocol: '..greeting.protocol)
>          end
>      end
>  
> @@ -541,14 +551,16 @@ end
>  -- Now it is necessary to have a strong ref to callback somewhere or
>  -- it is GC-ed prematurely. We wrap close() method, stashing the
>  -- ref in an upvalue (close() performance doesn't matter much.)
> -local create_transport = function(host, port, user, password, callback)
> +local create_transport = function(host, port, user, password, callback,
> +                                  existing_connection, greeting)
>      local weak_refs = setmetatable({callback = callback}, {__mode = 'v'})
>      local function weak_callback(...)
>          local callback = weak_refs.callback
>          if callback then return callback(...) end
>      end
> -    local transport = create_transport(host, port, user,
> -                                       password, weak_callback)
> +    local transport = create_transport(host, port, user, password,
> +                                       weak_callback, existing_connection,
> +                                       greeting)
>      local transport_close = transport.close
>      local gc_hook = ffi.gc(ffi.new('char[1]'), function()
>          pcall(transport_close)
> @@ -612,8 +624,7 @@ local console_mt = {
>  
>  local space_metatable, index_metatable
>  
> -local function connect(...)
> -    local host, port, opts = parse_connect_params(...)
> +local function do_connect(host, port, opts, existing_connection, greeting)
>      local user, password = opts.user, opts.password; opts.password = nil
>      local last_reconnect_error
>      local remote = {host = host, port = port, opts = opts, state = 'initial'}
> @@ -679,7 +690,8 @@ local function connect(...)
>      remote._on_schema_reload = trigger.new("on_schema_reload")
>      remote._on_disconnect = trigger.new("on_disconnect")
>      remote._on_connect = trigger.new("on_connect")
> -    remote._transport = create_transport(host, port, user, password, callback)
> +    remote._transport = create_transport(host, port, user, password, callback,
> +                                         existing_connection, greeting)
>      remote._transport.connect()
>      if opts.wait_connected ~= false then
>          remote._transport.wait_state('active', tonumber(opts.wait_connected))
> @@ -687,6 +699,25 @@ local function connect(...)
>      return remote
>  end
>  
> +local function connect(...)
> +    local host, port, opts = parse_connect_params(...)
> +    return do_connect(host, port, opts)
> +end
> +
> +local function wrap_socket(connection, greeting, url, opts)
> +    if connection == nil or type(greeting) ~= 'table' then
> +        error('Usage: netbox.wrap_socket(socket, greeting, [opts])')
> +    end
> +    if greeting.protocol == 'Lua console' then
> +        error('Can not wrap console socket')
> +    end
> +    opts = opts or {}
> +    if not opts.user and not opts.password then
> +        opts.user, opts.password = url.login, url.password
> +    end
> +    return do_connect(url.host, url.service, opts, connection, greeting)
> +end

Looks like you should make the next step, i.e. take the piece of
do_connect which establishes a connection and move it to
connect(), and then you can the remains of do_connect into wrap()
and call wrap() connect().

Did you try to do it? Had any trouble?

> +
>  local function check_remote_arg(remote, method)
>      if type(remote) ~= 'table' then
>          local fmt = 'Use remote:%s(...) instead of remote.%s(...):'
> @@ -1112,7 +1143,9 @@ end
>  local this_module = {
>      create_transport = create_transport,
>      connect = connect,
> -    new = connect -- Tarantool < 1.7.1 compatibility
> +    new = connect, -- Tarantool < 1.7.1 compatibility,
> +    decode_greeting = internal.decode_greeting,
> +    wrap_socket = wrap_socket,
>  }
>  
>  function this_module.timeout(timeout, ...)
> diff --git a/test/box/net.box.result b/test/box/net.box.result
> index 46d85b327..cb9410085 100644
> --- a/test/box/net.box.result
> +++ b/test/box/net.box.result
> @@ -2292,12 +2292,86 @@ weak.c
>  ---
>  - null
>  ...
> -box.schema.user.revoke('guest', 'execute', 'universe')
> +--
> +-- gh-2677: netbox supports console connections, that complicates
> +-- both console and netbox. It was necessary because before a
> +-- connection is established, a console does not known is it
> +-- binary or text protocol, and netbox could not be created from
> +-- existing socket.
> +--
> +box.schema.user.grant('guest','read,write,execute','universe')
> +---
> +...
> +urilib = require('uri')
> +---
> +...
> +uri = urilib.parse(tostring(box.cfg.listen))
> +---
> +...
> +s = socket.tcp_connect(uri.host, uri.service)
> +---
> +...
> +greeting = s:read({chunk = 128})
> +---
> +...
> +greeting = net.decode_greeting(greeting)
> +---
> +...
> +c = net.wrap_socket(s, greeting, uri)
> +---
> +...
> +c.state
> +---
> +- active
> +...
> +a = 100
> +---
> +...
> +function kek(args) return {1, 2, 3, args} end
> +---
> +...
> +c:eval('a = 200')
> +---
> +...
> +a
> +---
> +- 200
> +...
> +c:call('kek', {300})
> +---
> +- [1, 2, 3, 300]
> +...
> +s = box.schema.create_space('test')
> +---
> +...
> +pk = s:create_index('pk')
> +---
> +...
> +c:reload_schema()
> +---
> +...
> +c.space.test:replace{1}
> +---
> +- [1]
> +...
> +c.space.test:get{1}
> +---
> +- [1]
> +...
> +c.space.test:delete{1}
> +---
> +- [1]
> +...
> +s:drop()
>  ---
>  ...
>  c:close()
>  ---
>  ...
> -c = nil
> +c.state
> +---
> +- closed
> +...
> +box.schema.user.revoke('guest', 'read,write,execute', 'universe')
>  ---
>  ...
> diff --git a/test/box/net.box.test.lua b/test/box/net.box.test.lua
> index 87e26f84c..487401514 100644
> --- a/test/box/net.box.test.lua
> +++ b/test/box/net.box.test.lua
> @@ -937,6 +937,35 @@ collectgarbage('collect')
>  -- connection is deleted by 'collect'.
>  weak.c
>  
> -box.schema.user.revoke('guest', 'execute', 'universe')
> +--
> +-- gh-2677: netbox supports console connections, that complicates
> +-- both console and netbox. It was necessary because before a
> +-- connection is established, a console does not known is it
> +-- binary or text protocol, and netbox could not be created from
> +-- existing socket.
> +--
> +box.schema.user.grant('guest','read,write,execute','universe')
> +urilib = require('uri')
> +uri = urilib.parse(tostring(box.cfg.listen))
> +s = socket.tcp_connect(uri.host, uri.service)
> +greeting = s:read({chunk = 128})
> +greeting = net.decode_greeting(greeting)
> +c = net.wrap_socket(s, greeting, uri)
> +c.state
> +
> +a = 100
> +function kek(args) return {1, 2, 3, args} end
> +c:eval('a = 200')
> +a
> +c:call('kek', {300})
> +s = box.schema.create_space('test')
> +pk = s:create_index('pk')
> +c:reload_schema()
> +c.space.test:replace{1}
> +c.space.test:get{1}
> +c.space.test:delete{1}
> +s:drop()
>  c:close()
> -c = nil
> +c.state
> +
> +box.schema.user.revoke('guest', 'read,write,execute', 'universe')
> -- 
> 2.14.3 (Apple Git-98)
> 
> 

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.org - www.twitter.com/kostja_osipov

Re: [tarantool-patches] [PATCH 2/3] console: do not use netbox for console text connections

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [18/03/22 22:18]:
> Netbox console support complicates both netbox and console. Lets
> use sockets directly for text protocol.
> 
> Part of #2677
> ---
>  src/box/lua/console.lua | 178 +++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 146 insertions(+), 32 deletions(-)
> 
> diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua
> index d49cf42be..19ff12876 100644
> --- a/src/box/lua/console.lua
> +++ b/src/box/lua/console.lua
> @@ -8,6 +8,9 @@ local log = require('log')
>  local errno = require('errno')
>  local urilib = require('uri')
>  local yaml = require('yaml')
> +local net_box = require('net.box')
> +
> +local YAML_TERM = '\n...\n'
>  
>  -- admin formatter must be able to encode any Lua variable
>  local formatter = yaml.new()
> @@ -29,7 +32,7 @@ local function format(status, ...)
>      if status then
>          local count = select('#', ...)
>          if count == 0 then
> -            return "---\n...\n"
> +            return "---"..YAML_TERM

Why did you do this?

The rest of the patch seems to be fine.

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.org - www.twitter.com/kostja_osipov

Re: [tarantool-patches] [PATCH 3/3] netbox: deprecate console support

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [18/03/22 22:18]:
> Print warning about that. After a while the cosole support will
> be deleted from netbox.
> ---
>  src/box/lua/net_box.lua | 6 ++++++
>  1 file changed, 6 insertions(+)


This is a no-brainer.

> 
> diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> index f3f69f52f..cf1a04237 100644
> --- a/src/box/lua/net_box.lua
> +++ b/src/box/lua/net_box.lua
> @@ -396,7 +396,10 @@ local function create_transport(host, port, user, password, callback,
>          if err then
>              return error_sm(err, msg)
>          end
> +        -- @deprecated since 1.10
>          if greeting.protocol == 'Lua console' then
> +            log.warn("Netbox text protocol support is deprecated since 1.10, "..
> +                     "please use require('console').connect() instead")
>              local setup_delimiter = 'require("console").delimiter("$EOF$")\n'
>              method_codec.inject(send_buf, nil, nil, setup_delimiter)
>              local err, response = send_and_recv_console()
> @@ -673,7 +676,10 @@ local function do_connect(host, port, opts, existing_connection, greeting)
>              end
>          end
>      end
> +    -- @deprecated since 1.10
>      if opts.console then
> +        log.warn("Netbox console API is deprecated since 1.10, please use "..
> +                 "require('console').connect() instead")
>          setmetatable(remote, console_mt)
>      else
>          setmetatable(remote, remote_mt)
> -- 
> 2.14.3 (Apple Git-98)
> 
> 

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.org - www.twitter.com/kostja_osipov

Re: [tarantool-patches] [PATCH 1/3] netbox: allow to create a netbox connection from existing socket

[v.shpilevoy]

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



> 22 марта 2018 г., в 22:33, Konstantin Osipov <kostja@tarantool.org> написал(а):
> 
> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org <mailto:v.shpilevoy@tarantool.org>> [18/03/22 22:18]:
> 
> The patch generally looks good to me, I can only nitpick on new
> things. 
> 
> How about renaming do_connect to connect_impl and wrap_connect to
> wrap_socket or simply wrap, or maybe bless or imbue?

Ok.

> 
>> +        if existing_connection then
>> +            connection = existing_connection
>> +            existing_connection = nil
> 
> Why is it necessary to set it to nil?

On reconnect it will call protocol_sm again, and it must no get old existing_connection again.

> 
>> +            assert(greeting)
>> +            assert(greeting.protocol ~= 'Lua console')
> 
> Please keep in mind that in Lua there are no asserts - it's a
> runtime check.

Ok.

>> +
>> +local function wrap_socket(connection, greeting, url, opts)
>> +    if connection == nil or type(greeting) ~= 'table' then
>> +        error('Usage: netbox.wrap_socket(socket, greeting, [opts])')
>> +    end
>> +    if greeting.protocol == 'Lua console' then
>> +        error('Can not wrap console socket')
>> +    end
>> +    opts = opts or {}
>> +    if not opts.user and not opts.password then
>> +        opts.user, opts.password = url.login, url.password
>> +    end
>> +    return do_connect(url.host, url.service, opts, connection, greeting)
>> +end
> 
> Looks like you should make the next step, i.e. take the piece of
> do_connect which establishes a connection and move it to
> connect(), and then you can the remains of do_connect into wrap()
> and call wrap() connect().
> 
> Did you try to do it? Had any trouble?

Connection establishment must be in connect inside create_transport, because even if an existing socket was wrapped, it still can have opts.reconnect_after option and the following scenario:
socket = ...
con = netbox.wrap_socket(socket, ..., {reconnect_after = ...})
--
-- use connection ...
--

Suppose it is broken due to error. Then netbox will try to establish a new connection after 'reconnect_after' seconds.
> 
> -- 
> Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
> http://tarantool.org <http://tarantool.org/> - www.twitter.com/kostja_osipov <http://www.twitter.com/kostja_osipov>

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

Re: [tarantool-patches] Re: [PATCH 1/3] netbox: allow to create a netbox connection from existing socket

[v.shpilevoy]

All is fixed on branch. Here is the diff for fixes.

> diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua
> index 2109c2fa0..02e7f1066 100644
> --- a/src/box/lua/console.lua
> +++ b/src/box/lua/console.lua
> @@ -32,7 +32,7 @@ local function format(status, ...)
>      if status then
>          local count = select('#', ...)
>          if count == 0 then
> -            return "---"..YAML_TERM
> +            return "---\n...\n"
>          end
>          local res = {}
>          for i=1,count,1 do
> @@ -424,7 +424,7 @@ local function connect(uri, opts)
>      if greeting.protocol == 'Lua console' then
>          remote = wrap_text_socket(connection, u)
>      else
> -        remote = net_box.wrap_socket(connection, greeting, u)
> +        remote = net_box.wrap(connection, greeting, u)
>          if not remote.host then
>              remote.host = 'localhost'
>          end
> diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> index cf1a04237..57e2f1b17 100644
> --- a/src/box/lua/net_box.lua
> +++ b/src/box/lua/net_box.lua
> @@ -373,6 +373,8 @@ local function create_transport(host, port, user, password, callback,
>          local tm_begin, tm = fiber.clock(), callback('fetch_connect_timeout')
>          if existing_connection then
>              connection = existing_connection
> +            -- Nullify the connection - on reconnect it must not
> +            -- be get again.
>              existing_connection = nil
>              assert(greeting)
>              assert(greeting.protocol ~= 'Lua console')
> @@ -627,7 +629,7 @@ local console_mt = {
>  
>  local space_metatable, index_metatable
>  
> -local function do_connect(host, port, opts, existing_connection, greeting)
> +local function connect_impl(host, port, opts, existing_connection, greeting)
>      local user, password = opts.user, opts.password; opts.password = nil
>      local last_reconnect_error
>      local remote = {host = host, port = port, opts = opts, state = 'initial'}
> @@ -707,12 +709,12 @@ end
>  
>  local function connect(...)
>      local host, port, opts = parse_connect_params(...)
> -    return do_connect(host, port, opts)
> +    return connect_impl(host, port, opts)
>  end
>  
> -local function wrap_socket(connection, greeting, url, opts)
> +local function wrap(connection, greeting, url, opts)
>      if connection == nil or type(greeting) ~= 'table' then
> -        error('Usage: netbox.wrap_socket(socket, greeting, [opts])')
> +        error('Usage: netbox.wrap(socket, greeting, [opts])')
>      end
>      if greeting.protocol == 'Lua console' then
>          error('Can not wrap console socket')
> @@ -721,7 +723,7 @@ local function wrap_socket(connection, greeting, url, opts)
>      if not opts.user and not opts.password then
>          opts.user, opts.password = url.login, url.password
>      end
> -    return do_connect(url.host, url.service, opts, connection, greeting)
> +    return connect_impl(url.host, url.service, opts, connection, greeting)
>  end
>  
>  local function check_remote_arg(remote, method)
> @@ -1151,7 +1153,7 @@ local this_module = {
>      connect = connect,
>      new = connect, -- Tarantool < 1.7.1 compatibility,
>      decode_greeting = internal.decode_greeting,
> -    wrap_socket = wrap_socket,
> +    wrap = wrap,
>  }
>  
>  function this_module.timeout(timeout, ...)
> diff --git a/test/box/net.box.result b/test/box/net.box.result
> index cb9410085..cbfa0c7a1 100644
> --- a/test/box/net.box.result
> +++ b/test/box/net.box.result
> @@ -2317,7 +2317,7 @@ greeting = s:read({chunk = 128})
>  greeting = net.decode_greeting(greeting)
>  ---
>  ...
> -c = net.wrap_socket(s, greeting, uri)
> +c = net.wrap(s, greeting, uri, {reconnect_after = 0.01})
>  ---
>  ...
>  c.state
> @@ -2362,6 +2362,23 @@ c.space.test:delete{1}
>  ---
>  - [1]
>  ...
> +--
> +-- Break a connection to test reconnect_after.
> +--
> +_ = c._transport.perform_request(nil, nil, 'inject', nil, '\x80')
> +---
> +...
> +c.state
> +---
> +- error_reconnect
> +...
> +while not c:is_connected() do fiber.sleep(0.01) end
> +---
> +...
> +c:ping()
> +---
> +- true
> +...
>  s:drop()
>  ---
>  ...
> diff --git a/test/box/net.box.test.lua b/test/box/net.box.test.lua
> index 487401514..e0cac1955 100644
> --- a/test/box/net.box.test.lua
> +++ b/test/box/net.box.test.lua
> @@ -950,7 +950,7 @@ uri = urilib.parse(tostring(box.cfg.listen))
>  s = socket.tcp_connect(uri.host, uri.service)
>  greeting = s:read({chunk = 128})
>  greeting = net.decode_greeting(greeting)
> -c = net.wrap_socket(s, greeting, uri)
> +c = net.wrap(s, greeting, uri, {reconnect_after = 0.01})
>  c.state
>  
>  a = 100
> @@ -964,6 +964,14 @@ c:reload_schema()
>  c.space.test:replace{1}
>  c.space.test:get{1}
>  c.space.test:delete{1}
> +--
> +-- Break a connection to test reconnect_after.
> +--
> +_ = c._transport.perform_request(nil, nil, 'inject', nil, '\x80')
> +c.state
> +while not c:is_connected() do fiber.sleep(0.01) end
> +c:ping()
> +
>  s:drop()
>  c:close()
>  c.state
> 
> 

Re: [tarantool-patches] [PATCH 1/3] netbox: allow to create a netbox connection from existing socket

[v.shpilevoy]

I have found, that SIGPIPE appears only under a debugger (lldb or gdb), but it does not mean, that it's ok to crash during debugging. I leave the try-to-read-before-write fix, but I made it slightly more simple. Instead of storing read symbols in a buffer I just mark them as unread in ibuf. The patch is below and on the branch.

diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua
index 02e7f1066..633144c7f 100644
--- a/src/box/lua/console.lua
+++ b/src/box/lua/console.lua
@@ -94,20 +94,19 @@ local text_connection_mt = {
         -- @retval     nil Error.
         --
         write = function(self, text)
-            -- It is the hack to protect from SIGPIPE on send in
+            -- It is the hack to protect from SIGPIPE, which is
+            -- not ignored under debugger (gdb, lldb) on send in
             -- a socket, that is actually closed. If a socket is
             -- readable and read() returns nothing then the socket
             -- is closed, and writing into it will raise SIGPIPE.
             if self._socket:readable(0) then
-                local rc = self._socket:recv(1)
+                local rc = self._socket:read({chunk = 1})
                 if not rc or rc == '' then
                     return nil
                 else
-                    -- But if it was literally readable, the
-                    -- single read byte can not be put back, and
-                    -- must be stored somewhere, until console
-                    -- do read().
-                    self.buffer = self.buffer..rc
+                    assert(#rc == 1)
+                    -- Make the char be unread.
+                    self._socket.rbuf.wpos = self._socket.rbuf.wpos - 1
                 end
             end
             return self._socket:send(text)
@@ -120,8 +119,6 @@ local text_connection_mt = {
         read = function(self)
             local ret = self._socket:read(YAML_TERM)
             if ret and ret ~= '' then
-                ret = (self.buffer or '')..ret
-                self.buffer = nil
                 return ret
             end
         end,
@@ -168,7 +165,6 @@ local function wrap_text_socket(connection, url)
         state = 'active',
         host = url.host or 'localhost',
         port = url.service,
-        buffer = nil,
     }, text_connection_mt)
     if not conn:write('require("console").delimiter("$EOF$")\n') or
        not conn:read() then

Re: [tarantool-patches] [PATCH 1/3] netbox: allow to create a netbox connection from existing socket

[Konstantin Osipov]

* v.shpilevoy@tarantool.org <v.shpilevoy@tarantool.org> [18/03/22 22:54]:
> > 22 марта 2018 г., в 22:33, Konstantin Osipov <kostja@tarantool.org> написал(а):
> > 
> > * Vladislav Shpilevoy <v.shpilevoy@tarantool.org <mailto:v.shpilevoy@tarantool.org>> [18/03/22 22:18]:
> > 
> > The patch generally looks good to me, I can only nitpick on new
> > things. 
> > 
> > How about renaming do_connect to connect_impl and wrap_connect to
> > wrap_socket or simply wrap, or maybe bless or imbue?
> 
> Ok.
> 
> > 
> >> +        if existing_connection then
> >> +            connection = existing_connection
> >> +            existing_connection = nil
> > 
> > Why is it necessary to set it to nil?
> 
> On reconnect it will call protocol_sm again, and it must no get
> old existing_connection again.

This code is a mess and you make it even more messy.

What frustrates me most is that it's a trivial refactoring and
you prefer explaining why a fix is impossible rather than making
a good fix.

You *can* change the responsibilities of all of these functions so
that everything works nicely without extra branches and having to
pass existing_connection through several layers of calls.


> > 
> >> +            assert(greeting)
> >> +            assert(greeting.protocol ~= 'Lua console')
> > 
> > Please keep in mind that in Lua there are no asserts - it's a
> > runtime check.
> kk
> Ok.
> 
> >> +
> >> +local function wrap_socket(connection, greeting, url, opts)
> >> +    if connection == nil or type(greeting) ~= 'table' then
> >> +        error('Usage: netbox.wrap_socket(socket, greeting, [opts])')
> >> +    end
> >> +    if greeting.protocol == 'Lua console' then
> >> +        error('Can not wrap console socket')
> >> +    end
> >> +    opts = opts or {}
> >> +    if not opts.user and not opts.password then
> >> +        opts.user, opts.password = url.login, url.password
> >> +    end
> >> +    return do_connect(url.host, url.service, opts, connection, greeting)
> >> +end
> > 
> > Looks like you should make the next step, i.e. take the piece of
> > do_connect which establishes a connection and move it to
> > connect(), and then you can the remains of do_connect into wrap()
> > and call wrap() connect().
> > 
> > Did you try to do it? Had any trouble?
> 
> Connection establishment must be in connect inside
> create_transport, because even if an existing socket was
> wrapped, it still can have opts.reconnect_after option and the
> following scenario:
> socket = ...
> con = netbox.wrap_socket(socket, ..., {reconnect_after = ...})
> --
> -- use connection ...
> --
> 
> Suppose it is broken due to error. Then netbox will try to
> establish a new connection after 'reconnect_after' seconds.

You can leave reconnect in protocol_sm, but you can make sure
protocol_sm is never called without an existing connection.
You can make the first connect directly in create_transport(),
or better yet, as I first suggested, outside, removing the need
for connect_impl().


-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.org - www.twitter.com/kostja_osipov