From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Thu, 22 Mar 2018 22:33:12 +0300 From: Konstantin Osipov Subject: Re: [tarantool-patches] [PATCH 1/3] netbox: allow to create a netbox connection from existing socket Message-ID: <20180322193312.GA18129@atlas> References: <10c500ef97902f645111fd2d46e756bf8dfdac1d.1521745741.git.v.shpilevoy@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <10c500ef97902f645111fd2d46e756bf8dfdac1d.1521745741.git.v.shpilevoy@tarantool.org> To: tarantool-patches@freelists.org Cc: vdavydov.dev@gmail.com, Vladislav Shpilevoy List-ID: * Vladislav Shpilevoy [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