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

[Konstantin Osipov <kostja@tarantool.org>]

* 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