* [PATCH 1/1] netbox: fix wait_connected ignorance
@ 2018-12-05 15:06 Vladislav Shpilevoy
2018-12-06 17:46 ` Vladimir Davydov
0 siblings, 1 reply; 4+ messages in thread
From: Vladislav Shpilevoy @ 2018-12-05 15:06 UTC (permalink / raw)
To: tarantool-patches; +Cc: vdavydov.dev
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
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)
+ 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
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
worker_fiber = nil
end
end
@@ -990,15 +992,7 @@ end
-- @retval Net.box object.
--
local function connect(...)
- local host, port, opts = parse_connect_params(...)
- local connection, greeting =
- establish_connection(host, port, opts.connect_timeout)
- if not connection then
- local dummy_conn = new_sm(host, port, opts)
- dummy_conn.error = greeting
- return dummy_conn
- end
- return new_sm(host, port, opts, connection, greeting)
+ return new_sm(parse_connect_params(...))
end
local function check_remote_arg(remote, method)
diff --git a/test/box/net.box.result b/test/box/net.box.result
index 6e59d0bc0..41d758679 100644
--- a/test/box/net.box.result
+++ b/test/box/net.box.result
@@ -3404,6 +3404,23 @@ ok, err
- false
- Connection closed
...
+--
+-- gh-3856: wait_connected = false is ignored.
+--
+c = net.connect('8.8.8.8:123456', {wait_connected = false})
+---
+...
+c
+---
+- opts:
+ wait_connected: false
+ host: 8.8.8.8
+ state: initial
+ port: '123456'
+...
+c:close()
+---
+...
box.schema.func.drop('do_long')
---
...
diff --git a/test/box/net.box.test.lua b/test/box/net.box.test.lua
index 26773aac9..2945d4aca 100644
--- a/test/box/net.box.test.lua
+++ b/test/box/net.box.test.lua
@@ -1373,6 +1373,13 @@ ready = true
while not err do fiber.sleep(0.01) end
ok, err
+--
+-- gh-3856: wait_connected = false is ignored.
+--
+c = net.connect('8.8.8.8:123456', {wait_connected = false})
+c
+c:close()
+
box.schema.func.drop('do_long')
box.schema.user.revoke('guest', 'write', 'space', '_schema')
box.schema.user.revoke('guest', 'read,write', 'space', '_space')
--
2.17.2 (Apple Git-113)
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] netbox: fix wait_connected ignorance
2018-12-05 15:06 [PATCH 1/1] netbox: fix wait_connected ignorance Vladislav Shpilevoy
@ 2018-12-06 17:46 ` Vladimir Davydov
2018-12-06 22:45 ` [tarantool-patches] " Vladislav Shpilevoy
0 siblings, 1 reply; 4+ messages in thread
From: Vladimir Davydov @ 2018-12-06 17:46 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [tarantool-patches] Re: [PATCH 1/1] netbox: fix wait_connected ignorance
2018-12-06 17:46 ` Vladimir Davydov
@ 2018-12-06 22:45 ` Vladislav Shpilevoy
2018-12-09 11:16 ` Vladimir Davydov
0 siblings, 1 reply; 4+ messages in thread
From: Vladislav Shpilevoy @ 2018-12-06 22:45 UTC (permalink / raw)
To: tarantool-patches, Vladimir Davydov
Hi! Thanks for the review.
On 06/12/2018 20:46, Vladimir Davydov wrote:
> 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.
Done.
>
> Should I cherry-pick it to 1.10?
Yes, the bug appears on 1.10 as well.
>
>>
>> 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
>
Yes, we can, but there are a few problems:
1. In Lua labels have visibility scope. So to put do_connect inside
while I need to replace while with another explicit goto.
2. In the first attempt to connect I set error as set_state('error')
instead of set_state('error_reconnect') - it is important, because
the latter logs an error.
But nonetheless I've solved these problems. This makes diff 3 lines
shorter, but I am not sure that such a small shortening is worth
making code less clear. It is up to you.
diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index cf6f7a168..20deaf2bd 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -427,16 +427,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)
- goto stop
- end
- set_state('error_reconnect', E_NO_CONNECTION, greeting)
- goto do_reconnect
- end
+ goto do_connect
end
::handle_connection::
ok, err = pcall(protocol_sm)
@@ -447,23 +438,29 @@ local function create_transport(host, port, user, password, callback,
connection:close()
connection = nil
end
+ timeout = callback('reconnect_timeout')
::do_reconnect::
+ if not timeout or state ~= 'error_reconnect' then
+ goto stop
+ end
+ fiber.sleep(timeout)
timeout = callback('reconnect_timeout')
- while timeout and state == 'error_reconnect' do
- fiber.sleep(timeout)
- timeout = callback('reconnect_timeout')
- if not timeout or state ~= 'error_reconnect' then
- break
- end
- connection, greeting =
- establish_connection(host, port,
- callback('fetch_connect_timeout'))
- if connection then
- goto handle_connection
- end
- set_state('error_reconnect', E_NO_CONNECTION, greeting)
- timeout = callback('reconnect_timeout')
+ if not timeout or state ~= 'error_reconnect' then
+ goto stop
+ end
+ ::do_connect::
+ connection, greeting =
+ establish_connection(host, port, callback('fetch_connect_timeout'))
+ if connection then
+ goto handle_connection
+ end
+ timeout = callback('reconnect_timeout')
+ if not timeout then
+ set_state('error', E_NO_CONNECTION, greeting)
+ goto stop
end
+ set_state('error_reconnect', E_NO_CONNECTION, greeting)
+ goto do_reconnect
::stop::
send_buf:recycle()
recv_buf:recycle()
>> + 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
>
>
Yes, it is possible.
diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index 72c0104b8..cf6f7a168 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -423,7 +423,7 @@ local function create_transport(host, port, user, password, callback,
local function start()
if state ~= 'initial' then return not is_final_state[state] end
fiber.create(function()
- local ok, err
+ local ok, err, timeout
worker_fiber = fiber_self()
fiber.name(string.format('%s:%s (net.box)', host, port), {truncate=true})
if not connection then
@@ -432,7 +432,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
@@ -448,7 +448,7 @@ local function create_transport(host, port, user, password, callback,
connection = nil
end
::do_reconnect::
- local timeout = callback('reconnect_timeout')
+ timeout = callback('reconnect_timeout')
while timeout and state == 'error_reconnect' do
fiber.sleep(timeout)
timeout = callback('reconnect_timeout')
@@ -464,6 +464,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
@@ -475,9 +476,7 @@ local function create_transport(host, port, user, password, callback,
set_state('closed', E_NO_CONNECTION, 'Connection closed')
end
if worker_fiber then
- if worker_fiber:status() ~= 'dead' then
- worker_fiber:cancel()
- end
+ worker_fiber:cancel()
worker_fiber = nil
end
end
New patch:
==================================================================
commit fbd63b6195358549505597dcda86d21ccfbb272a
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date: Wed Dec 5 14:59:10 2018 +0300
netbox: fix wait_connected ignorance
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
diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index 2040d5e41..20deaf2bd 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -422,21 +422,12 @@ 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
+ local ok, err, timeout
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
+ goto do_connect
end
::handle_connection::
ok, err = pcall(protocol_sm)
@@ -447,23 +438,30 @@ local function create_transport(host, port, user, password, callback,
connection:close()
connection = nil
end
+ timeout = callback('reconnect_timeout')
::do_reconnect::
- local timeout = callback('reconnect_timeout')
- while timeout and state == 'error_reconnect' do
- fiber.sleep(timeout)
- timeout = callback('reconnect_timeout')
- if not timeout or state ~= 'error_reconnect' then
- break
- end
- connection, greeting =
- establish_connection(host, port,
- callback('fetch_connect_timeout'))
- if connection then
- goto handle_connection
- end
- set_state('error_reconnect', E_NO_CONNECTION, greeting)
- timeout = callback('reconnect_timeout')
+ if not timeout or state ~= 'error_reconnect' then
+ goto stop
+ end
+ fiber.sleep(timeout)
+ timeout = callback('reconnect_timeout')
+ if not timeout or state ~= 'error_reconnect' then
+ goto stop
end
+ ::do_connect::
+ connection, greeting =
+ establish_connection(host, port, callback('fetch_connect_timeout'))
+ if connection then
+ goto handle_connection
+ end
+ timeout = callback('reconnect_timeout')
+ if not timeout then
+ set_state('error', E_NO_CONNECTION, greeting)
+ goto stop
+ end
+ set_state('error_reconnect', E_NO_CONNECTION, greeting)
+ goto do_reconnect
+ ::stop::
send_buf:recycle()
recv_buf:recycle()
worker_fiber = nil
@@ -993,15 +991,7 @@ end
-- @retval Net.box object.
--
local function connect(...)
- local host, port, opts = parse_connect_params(...)
- local connection, greeting =
- establish_connection(host, port, opts.connect_timeout)
- if not connection then
- local dummy_conn = new_sm(host, port, opts)
- dummy_conn.error = greeting
- return dummy_conn
- end
- return new_sm(host, port, opts, connection, greeting)
+ return new_sm(parse_connect_params(...))
end
local function check_remote_arg(remote, method)
diff --git a/test/box/net.box.result b/test/box/net.box.result
index faae5e9a5..b6b6d88a4 100644
--- a/test/box/net.box.result
+++ b/test/box/net.box.result
@@ -3405,6 +3405,23 @@ ok, err
- false
- Connection closed
...
+--
+-- gh-3856: wait_connected = false is ignored.
+--
+c = net.connect('8.8.8.8:123456', {wait_connected = false})
+---
+...
+c
+---
+- opts:
+ wait_connected: false
+ host: 8.8.8.8
+ state: initial
+ port: '123456'
+...
+c:close()
+---
+...
box.schema.func.drop('do_long')
---
...
diff --git a/test/box/net.box.test.lua b/test/box/net.box.test.lua
index dd6c2fd94..5152e3569 100644
--- a/test/box/net.box.test.lua
+++ b/test/box/net.box.test.lua
@@ -1374,6 +1374,13 @@ ready = true
while not err do fiber.sleep(0.01) end
ok, err
+--
+-- gh-3856: wait_connected = false is ignored.
+--
+c = net.connect('8.8.8.8:123456', {wait_connected = false})
+c
+c:close()
+
box.schema.func.drop('do_long')
box.schema.user.revoke('guest', 'write', 'space', '_schema')
box.schema.user.revoke('guest', 'read,write', 'space', '_space')
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [tarantool-patches] Re: [PATCH 1/1] netbox: fix wait_connected ignorance
2018-12-06 22:45 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2018-12-09 11:16 ` Vladimir Davydov
0 siblings, 0 replies; 4+ messages in thread
From: Vladimir Davydov @ 2018-12-09 11:16 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On Fri, Dec 07, 2018 at 01:45:16AM +0300, Vladislav Shpilevoy wrote:
> On 06/12/2018 20:46, Vladimir Davydov wrote:
> > On Wed, Dec 05, 2018 at 06:06:39PM +0300, Vladislav Shpilevoy wrote:
> > > 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
> >
> Yes, we can, but there are a few problems:
>
> 1. In Lua labels have visibility scope. So to put do_connect inside
> while I need to replace while with another explicit goto.
>
> 2. In the first attempt to connect I set error as set_state('error')
> instead of set_state('error_reconnect') - it is important, because
> the latter logs an error.
>
> But nonetheless I've solved these problems. This makes diff 3 lines
> shorter, but I am not sure that such a small shortening is worth
> making code less clear. It is up to you.
Thanks, the new version looks more straighforward to me, actually.
Pushed to 2.1 and 1.10.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-12-09 11:16 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-05 15:06 [PATCH 1/1] netbox: fix wait_connected ignorance Vladislav Shpilevoy
2018-12-06 17:46 ` Vladimir Davydov
2018-12-06 22:45 ` [tarantool-patches] " Vladislav Shpilevoy
2018-12-09 11:16 ` Vladimir Davydov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox