* [PATCH 0/3] console: do not use netbox for console text connections @ 2018-03-22 19:17 Vladislav Shpilevoy 2018-03-22 19:17 ` [PATCH 1/3] netbox: allow to create a netbox connection from existing socket Vladislav Shpilevoy ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Vladislav Shpilevoy @ 2018-03-22 19:17 UTC (permalink / raw) To: tarantool-patches; +Cc: vdavydov.dev, 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) ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/3] netbox: allow to create a netbox connection from existing socket 2018-03-22 19:17 [PATCH 0/3] console: do not use netbox for console text connections Vladislav Shpilevoy @ 2018-03-22 19:17 ` Vladislav Shpilevoy 2018-03-22 19:33 ` [tarantool-patches] " Konstantin Osipov 2018-03-22 19:17 ` [PATCH 2/3] console: do not use netbox for console text connections Vladislav Shpilevoy 2018-03-22 19:17 ` [PATCH 3/3] netbox: deprecate console support Vladislav Shpilevoy 2 siblings, 1 reply; 11+ messages in thread From: Vladislav Shpilevoy @ 2018-03-22 19:17 UTC (permalink / raw) To: tarantool-patches; +Cc: vdavydov.dev, 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) ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [tarantool-patches] [PATCH 1/3] netbox: allow to create a netbox connection from existing socket 2018-03-22 19:17 ` [PATCH 1/3] netbox: allow to create a netbox connection from existing socket Vladislav Shpilevoy @ 2018-03-22 19:33 ` Konstantin Osipov 2018-03-22 19:50 ` v.shpilevoy 0 siblings, 1 reply; 11+ messages in thread From: Konstantin Osipov @ 2018-03-22 19:33 UTC (permalink / raw) To: tarantool-patches; +Cc: vdavydov.dev, Vladislav Shpilevoy * 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [tarantool-patches] [PATCH 1/3] netbox: allow to create a netbox connection from existing socket 2018-03-22 19:33 ` [tarantool-patches] " Konstantin Osipov @ 2018-03-22 19:50 ` v.shpilevoy 2018-03-22 19:57 ` [tarantool-patches] " v.shpilevoy 2018-03-26 21:55 ` Konstantin Osipov 0 siblings, 2 replies; 11+ messages in thread From: v.shpilevoy @ 2018-03-22 19:50 UTC (permalink / raw) To: Konstantin Osipov; +Cc: tarantool-patches, vdavydov.dev [-- 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 --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [tarantool-patches] Re: [PATCH 1/3] netbox: allow to create a netbox connection from existing socket 2018-03-22 19:50 ` v.shpilevoy @ 2018-03-22 19:57 ` v.shpilevoy 2018-03-23 10:01 ` [tarantool-patches] " v.shpilevoy 2018-03-26 21:55 ` Konstantin Osipov 1 sibling, 1 reply; 11+ messages in thread From: v.shpilevoy @ 2018-03-22 19:57 UTC (permalink / raw) To: tarantool-patches; +Cc: Konstantin Osipov, vdavydov.dev 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 > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [tarantool-patches] [PATCH 1/3] netbox: allow to create a netbox connection from existing socket 2018-03-22 19:57 ` [tarantool-patches] " v.shpilevoy @ 2018-03-23 10:01 ` v.shpilevoy 0 siblings, 0 replies; 11+ messages in thread From: v.shpilevoy @ 2018-03-23 10:01 UTC (permalink / raw) To: tarantool-patches; +Cc: Konstantin Osipov, vdavydov.dev 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [tarantool-patches] [PATCH 1/3] netbox: allow to create a netbox connection from existing socket 2018-03-22 19:50 ` v.shpilevoy 2018-03-22 19:57 ` [tarantool-patches] " v.shpilevoy @ 2018-03-26 21:55 ` Konstantin Osipov 1 sibling, 0 replies; 11+ messages in thread From: Konstantin Osipov @ 2018-03-26 21:55 UTC (permalink / raw) To: v.shpilevoy; +Cc: tarantool-patches, vdavydov.dev * 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/3] console: do not use netbox for console text connections 2018-03-22 19:17 [PATCH 0/3] console: do not use netbox for console text connections Vladislav Shpilevoy 2018-03-22 19:17 ` [PATCH 1/3] netbox: allow to create a netbox connection from existing socket Vladislav Shpilevoy @ 2018-03-22 19:17 ` Vladislav Shpilevoy 2018-03-22 19:36 ` [tarantool-patches] " Konstantin Osipov 2018-03-22 19:17 ` [PATCH 3/3] netbox: deprecate console support Vladislav Shpilevoy 2 siblings, 1 reply; 11+ messages in thread From: Vladislav Shpilevoy @ 2018-03-22 19:17 UTC (permalink / raw) To: tarantool-patches; +Cc: vdavydov.dev, 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) ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [tarantool-patches] [PATCH 2/3] console: do not use netbox for console text connections 2018-03-22 19:17 ` [PATCH 2/3] console: do not use netbox for console text connections Vladislav Shpilevoy @ 2018-03-22 19:36 ` Konstantin Osipov 0 siblings, 0 replies; 11+ messages in thread From: Konstantin Osipov @ 2018-03-22 19:36 UTC (permalink / raw) To: tarantool-patches; +Cc: vdavydov.dev, Vladislav Shpilevoy * 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/3] netbox: deprecate console support 2018-03-22 19:17 [PATCH 0/3] console: do not use netbox for console text connections Vladislav Shpilevoy 2018-03-22 19:17 ` [PATCH 1/3] netbox: allow to create a netbox connection from existing socket Vladislav Shpilevoy 2018-03-22 19:17 ` [PATCH 2/3] console: do not use netbox for console text connections Vladislav Shpilevoy @ 2018-03-22 19:17 ` Vladislav Shpilevoy 2018-03-22 19:37 ` [tarantool-patches] " Konstantin Osipov 2 siblings, 1 reply; 11+ messages in thread From: Vladislav Shpilevoy @ 2018-03-22 19:17 UTC (permalink / raw) To: tarantool-patches; +Cc: vdavydov.dev, 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) ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [tarantool-patches] [PATCH 3/3] netbox: deprecate console support 2018-03-22 19:17 ` [PATCH 3/3] netbox: deprecate console support Vladislav Shpilevoy @ 2018-03-22 19:37 ` Konstantin Osipov 0 siblings, 0 replies; 11+ messages in thread From: Konstantin Osipov @ 2018-03-22 19:37 UTC (permalink / raw) To: tarantool-patches; +Cc: vdavydov.dev, Vladislav Shpilevoy * 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-03-26 21:55 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-03-22 19:17 [PATCH 0/3] console: do not use netbox for console text connections Vladislav Shpilevoy 2018-03-22 19:17 ` [PATCH 1/3] netbox: allow to create a netbox connection from existing socket Vladislav Shpilevoy 2018-03-22 19:33 ` [tarantool-patches] " Konstantin Osipov 2018-03-22 19:50 ` v.shpilevoy 2018-03-22 19:57 ` [tarantool-patches] " v.shpilevoy 2018-03-23 10:01 ` [tarantool-patches] " v.shpilevoy 2018-03-26 21:55 ` Konstantin Osipov 2018-03-22 19:17 ` [PATCH 2/3] console: do not use netbox for console text connections Vladislav Shpilevoy 2018-03-22 19:36 ` [tarantool-patches] " Konstantin Osipov 2018-03-22 19:17 ` [PATCH 3/3] netbox: deprecate console support Vladislav Shpilevoy 2018-03-22 19:37 ` [tarantool-patches] " Konstantin Osipov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox