From: Vladimir Davydov <vdavydov.dev@gmail.com> To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> Cc: tarantool-patches@freelists.org Subject: Re: [PATCH 1/1] netbox: fix wait_connected ignorance Date: Thu, 6 Dec 2018 20:46:19 +0300 [thread overview] Message-ID: <20181206174619.aw6ege2sm72yleao@esperanza> (raw) In-Reply-To: <c20a920cf9fd4294ab5ac062c4e1a95746419ba2.1544022329.git.v.shpilevoy@tarantool.org> On Wed, Dec 05, 2018 at 06:06:39PM +0300, Vladislav Shpilevoy wrote: > After this patch d2468dacaf it became possible to > wrap an existing connection into netbox API. A regular > netbox.connect function was refactored so as to reuse > connection establishment code. > > But connection should be established in a worker > fiber, not in a caller's one. Otherwise it is > impossible to do not wait for connect result. > > The patch just moves connection establishment into a > worker fiber, without any functional changes. > > Closes #3856 > --- > https://github.com/tarantool/tarantool/tree/gerold103/gh-3856-netbox-ignores-wait-connected > https://github.com/tarantool/tarantool/issues/3856 Please rebase on top of 2.1 - we don't do merges anymore. Should I cherry-pick it to 1.10? > > src/box/lua/net_box.lua | 34 ++++++++++++++-------------------- > test/box/net.box.result | 17 +++++++++++++++++ > test/box/net.box.test.lua | 7 +++++++ > 3 files changed, 38 insertions(+), 20 deletions(-) > > diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua > index fd6ebf9de..d54b3e7d9 100644 > --- a/src/box/lua/net_box.lua > +++ b/src/box/lua/net_box.lua > @@ -419,21 +419,21 @@ local function create_transport(host, port, user, password, callback, > > local function start() > if state ~= 'initial' then return not is_final_state[state] end > - if not connection and not callback('reconnect_timeout') then > - set_state('error', E_NO_CONNECTION) > - return > - end > fiber.create(function() > local ok, err > worker_fiber = fiber_self() > fiber.name(string.format('%s:%s (net.box)', host, port), {truncate=true}) > - -- It is possible, if the first connection attempt had > - -- been failed, but reconnect timeout is set. In such > - -- a case the worker must be run, and immediately > - -- start reconnecting. > if not connection then > - set_state('error_reconnect', E_NO_CONNECTION, greeting) > - goto do_reconnect > + local tm = callback('fetch_connect_timeout') > + connection, greeting = establish_connection(host, port, tm) This code is very similar to the reconnect code below. You probably didn't reuse it for a reason, but still, may be we could do something like this? diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua index d54b3e7d..f342889a 100644 --- a/src/box/lua/net_box.lua +++ b/src/box/lua/net_box.lua @@ -424,16 +424,7 @@ local function create_transport(host, port, user, password, callback, worker_fiber = fiber_self() fiber.name(string.format('%s:%s (net.box)', host, port), {truncate=true}) if not connection then - local tm = callback('fetch_connect_timeout') - connection, greeting = establish_connection(host, port, tm) - if not connection then - if not callback('reconnect_timeout') then - set_state('error', E_NO_CONNECTION, greeting) - return - end - set_state('error_reconnect', E_NO_CONNECTION, greeting) - goto do_reconnect - end + goto do_connect end ::handle_connection:: ok, err = pcall(protocol_sm) @@ -448,6 +439,7 @@ local function create_transport(host, port, user, password, callback, local timeout = callback('reconnect_timeout') while timeout and state == 'error_reconnect' do fiber.sleep(timeout) + ::do_connect:: timeout = callback('reconnect_timeout') if not timeout or state ~= 'error_reconnect' then break > + if not connection then > + if not callback('reconnect_timeout') then > + set_state('error', E_NO_CONNECTION, greeting) > + return Shouldn't we clear worker_fiber here? diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua index d54b3e7d..33f26706 100644 --- a/src/box/lua/net_box.lua +++ b/src/box/lua/net_box.lua @@ -429,7 +429,7 @@ local function create_transport(host, port, user, password, callback, if not connection then if not callback('reconnect_timeout') then set_state('error', E_NO_CONNECTION, greeting) - return + goto stop end set_state('error_reconnect', E_NO_CONNECTION, greeting) goto do_reconnect @@ -461,6 +461,7 @@ local function create_transport(host, port, user, password, callback, set_state('error_reconnect', E_NO_CONNECTION, greeting) timeout = callback('reconnect_timeout') end + ::stop:: send_buf:recycle() recv_buf:recycle() worker_fiber = nil > + end > + set_state('error_reconnect', E_NO_CONNECTION, greeting) > + goto do_reconnect > + end > end > ::handle_connection:: > ok, err = pcall(protocol_sm) > @@ -472,7 +472,9 @@ local function create_transport(host, port, user, password, callback, > set_state('closed', E_NO_CONNECTION, 'Connection closed') > end > if worker_fiber then > - worker_fiber:cancel() > + if worker_fiber:status() ~= 'dead' then > + worker_fiber:cancel() > + end ... then you probably wouldn't need to change this. > worker_fiber = nil > end > end
next prev parent reply other threads:[~2018-12-06 17:46 UTC|newest] Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-12-05 15:06 Vladislav Shpilevoy 2018-12-06 17:46 ` Vladimir Davydov [this message] 2018-12-06 22:45 ` [tarantool-patches] " Vladislav Shpilevoy 2018-12-09 11:16 ` Vladimir Davydov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20181206174619.aw6ege2sm72yleao@esperanza \ --to=vdavydov.dev@gmail.com \ --cc=tarantool-patches@freelists.org \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [PATCH 1/1] netbox: fix wait_connected ignorance' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox