[Tarantool-patches] [PATCH 1/1] netbox: don't fire on_connect() at schema update

[Tarantool-patches] [PATCH 1/1] netbox: don't fire on_connect() at schema update

[Vladislav Shpilevoy]

There was a bug that netbox at any schema update called
on_connect() triggers. This was due to overcomplicated logic of
handling of changes in the netbox state machine. On_connect() was
fired each time the machine entered 'active' state, even if its
previous states were 'active' and then 'fetch_schema'. The latter
state can be entered many times without reconnects.

Another bug was about on_disconnect() - it could be fired even if
the connection never entered active state. For example, if its
first 'fetch_schema' has failed.

Now there is an explicit flag showing the machine connect state.
The triggers are fired only when it is changed, on 'active' and on
any error states. Intermediate states (fetch_schema, auth) do not
matter anymore.

Thanks @mtrempoltsev for the initial investigation and a draft
fix.

Closes #4593
---
Branch: https://github.com/tarantool/tarantool/tree/gerold103/gh-4593-netbox-on_connect
Issue: https://github.com/tarantool/tarantool/issues/4593

 src/box/lua/net_box.lua   |  20 +++++---
 test/box/net.box.result   | 100 ++++++++++++++++++++++++++++++++++++++
 test/box/net.box.test.lua |  50 +++++++++++++++++++
 3 files changed, 162 insertions(+), 8 deletions(-)

diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index 31a8c16b7..696b30fd9 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -927,14 +927,17 @@ local function new_sm(host, port, opts, connection, greeting)
     local function callback(what, ...)
         if what == 'state_changed' then
             local state, errno, err = ...
-            if (remote.state == 'active' or remote.state == 'fetch_schema') and
-               (state == 'error' or state == 'closed' or
-                state == 'error_reconnect') then
-               remote._on_disconnect:run(remote)
-            end
-            if remote.state ~= 'error' and remote.state ~= 'error_reconnect' and
-               state == 'active' then
-                remote._on_connect:run(remote)
+            local was_connected = remote._is_connected
+            if state == 'active' then
+                if not was_connected then
+                    remote._is_connected = true
+                    remote._on_connect:run(remote)
+                end
+            elseif errno ~= nil then
+                if was_connected then
+                    remote._is_connected = false
+                    remote._on_disconnect:run(remote)
+                end
             end
             remote.state, remote.error = state, err
             if state == 'error_reconnect' then
@@ -989,6 +992,7 @@ local function new_sm(host, port, opts, connection, greeting)
     remote._on_schema_reload = trigger.new("on_schema_reload")
     remote._on_disconnect = trigger.new("on_disconnect")
     remote._on_connect = trigger.new("on_connect")
+    remote._is_connected = false
     remote._transport = create_transport(host, port, user, password, callback,
                                          connection, greeting)
     remote._transport.start()
diff --git a/test/box/net.box.result b/test/box/net.box.result
index e3dabf7d9..010bb2b1c 100644
--- a/test/box/net.box.result
+++ b/test/box/net.box.result
@@ -3930,3 +3930,103 @@ test_run:grep_log('default', '00000040:.*')
 box.cfg{log_level=log_level}
 ---
 ...
+--
+-- gh-4593: net.box on_connect() and on_disconnect() were called
+-- not in time.
+--
+--
+-- on_disconnect() trigger should not be called if a connection
+-- was refused even before it managed to become active.
+--
+disconnected_count = 0
+---
+...
+connected_count = 0
+---
+...
+box.schema.user.disable('guest')
+---
+...
+function on_connect()                                           \
+    connected_count = connected_count + 1                       \
+end
+---
+...
+function on_disconnect()                                        \
+    disconnected_count = disconnected_count + 1                 \
+end
+---
+...
+c = remote.connect(box.cfg.listen, {wait_connected = false})    \
+c:on_disconnect(on_disconnect)                                  \
+c:on_connect(on_connect)
+---
+...
+c:wait_connected()
+---
+- false
+...
+connected_count
+---
+- 0
+...
+disconnected_count
+---
+- 0
+...
+c:close()
+---
+...
+connected_count
+---
+- 0
+...
+disconnected_count
+---
+- 0
+...
+box.schema.user.enable('guest')
+---
+...
+--
+-- on_connect() should not be called on schema update.
+--
+box.schema.user.grant('guest', 'read,write,execute,create', 'universe')
+---
+...
+c = remote.connect(box.cfg.listen, {wait_connected = false})    \
+c:on_disconnect(on_disconnect)                                  \
+c:on_connect(on_connect)
+---
+...
+function create_space() box.schema.create_space('test') end
+---
+...
+c:call('create_space')
+---
+...
+connected_count
+---
+- 1
+...
+disconnected_count
+---
+- 0
+...
+c:close()
+---
+...
+connected_count
+---
+- 1
+...
+disconnected_count
+---
+- 1
+...
+box.space.test:drop()
+---
+...
+box.schema.user.revoke('guest', 'read,write,execute,create', 'universe')
+---
+...
diff --git a/test/box/net.box.test.lua b/test/box/net.box.test.lua
index 8e65ff470..36a7bcb70 100644
--- a/test/box/net.box.test.lua
+++ b/test/box/net.box.test.lua
@@ -1583,3 +1583,53 @@ test_run:wait_log('default', '00000030:.*', nil, 10)
 test_run:grep_log('default', '00000040:.*')
 
 box.cfg{log_level=log_level}
+
+--
+-- gh-4593: net.box on_connect() and on_disconnect() were called
+-- not in time.
+--
+--
+-- on_disconnect() trigger should not be called if a connection
+-- was refused even before it managed to become active.
+--
+disconnected_count = 0
+connected_count = 0
+box.schema.user.disable('guest')
+
+function on_connect()                                           \
+    connected_count = connected_count + 1                       \
+end
+function on_disconnect()                                        \
+    disconnected_count = disconnected_count + 1                 \
+end
+
+c = remote.connect(box.cfg.listen, {wait_connected = false})    \
+c:on_disconnect(on_disconnect)                                  \
+c:on_connect(on_connect)
+c:wait_connected()
+
+connected_count
+disconnected_count
+c:close()
+connected_count
+disconnected_count
+box.schema.user.enable('guest')
+
+--
+-- on_connect() should not be called on schema update.
+--
+box.schema.user.grant('guest', 'read,write,execute,create', 'universe')
+c = remote.connect(box.cfg.listen, {wait_connected = false})    \
+c:on_disconnect(on_disconnect)                                  \
+c:on_connect(on_connect)
+function create_space() box.schema.create_space('test') end
+c:call('create_space')
+connected_count
+disconnected_count
+
+c:close()
+connected_count
+disconnected_count
+
+box.space.test:drop()
+box.schema.user.revoke('guest', 'read,write,execute,create', 'universe')
-- 
2.21.0 (Apple Git-122)

Re: [Tarantool-patches] [PATCH 1/1] netbox: don't fire on_connect() at schema update

[Cyrill Gorcunov]

On Tue, Oct 29, 2019 at 10:43:30PM +0100, Vladislav Shpilevoy wrote:
> There was a bug that netbox at any schema update called
> on_connect() triggers. This was due to overcomplicated logic of
> handling of changes in the netbox state machine. On_connect() was
> fired each time the machine entered 'active' state, even if its
> previous states were 'active' and then 'fetch_schema'. The latter
> state can be entered many times without reconnects.

Please ignore this message from me -- just need to figure out
the details on CC processing in our mailing list.

Re: [Tarantool-patches] [PATCH 1/1] netbox: don't fire on_connect() at schema update

[Vladislav Shpilevoy]

I moved the test into a separate file: box/gh-4593-netbox-on_connect-disconnect.test.lua

Re: [Tarantool-patches] [PATCH 1/1] netbox: don't fire on_connect() at schema update

[Alexander Turenko]

The patch LGTM in the sense that it should work as expected as far as I
see.

However I would discuss points I described below: I think we can write
the patch in a bit more clean way. Vlad, please, let me know what do you
think about this.

WBR, Alexander Turenko.

> diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> index 31a8c16b7..696b30fd9 100644
> --- a/src/box/lua/net_box.lua
> +++ b/src/box/lua/net_box.lua
> @@ -927,14 +927,17 @@ local function new_sm(host, port, opts, connection, greeting)
>      local function callback(what, ...)
>          if what == 'state_changed' then
>              local state, errno, err = ...
> -            if (remote.state == 'active' or remote.state == 'fetch_schema') and
> -               (state == 'error' or state == 'closed' or
> -                state == 'error_reconnect') then
> -               remote._on_disconnect:run(remote)
> -            end
> -            if remote.state ~= 'error' and remote.state ~= 'error_reconnect' and
> -               state == 'active' then
> -                remote._on_connect:run(remote)
> +            local was_connected = remote._is_connected
> +            if state == 'active' then
> +                if not was_connected then
> +                    remote._is_connected = true
> +                    remote._on_connect:run(remote)
> +                end

We splitted states into 'connected', 'neural' and 'disconnected' (a kind
of tags or properties). We fire the trigger when a connection step into
one of 'connected' states from one of 'disconnected' ones ('neural' are
not counted). This looks okay.

> +            elseif errno ~= nil then
> +                if was_connected then
> +                    remote._is_connected = false
> +                    remote._on_disconnect:run(remote)
> +                end

Here we use `errno ~= nil` condition to determine whether a state is
'disconnected' one. The condition is true for 'error', 'error_reconnect'
and 'closed' states. This way should give a correct behaviour.

When I saw the patch my question was whether a connection step into
'fetch_schema' state with `errno ~= nil`. It was not obvious for me what
list of states are always set with some 'errno' value (however it is
easy to deduce from set_state() calls). That is the first point.

The second is that I cannot prove (at least after brief look into the
code) that 'errno' is newer `nil` / `box.NULL` for 'disconnected'
states, because that are places where 'errno' is passed through a
function.

I think we should at least give a comment that by using `errno ~= nil`
we lean on assumption that we always step into 'error',
'error_reconnect' and 'closed' states with non-null 'errno' and that
there is no other states that set 'errno'; but better don't assume this.

Let's consider unix errno: it should not be used as a primary source of
information **whether** an error occurs. You always check a return code
and only if it says that an error occurs we can consider 'errno' as a
source of information **which kind** of error occurs.

That is why I generally against using of errno / diagnostic area as
sources of information whether an error occurs: in context of Unix APIs
this would be an improper usage.

I would mark states as 'connected' and 'disconnected' explicitly:

 | -- XXX: Give a comment why, say, 'fetch_schema' is not here.
 | local function is_state_connected(state)
 |     return state == 'active'
 | end
 |
 | local disconnected_states = {
 |     initial = true,
 |     error = true,
 |     error_reconnect = true,
 |     closed = true,
 | }
 |
 | local function is_state_disconnected(state)
 |     return disconnected_states[state]
 | end

And use this markers instead of `state == 'active'` and `errno ~= nil`
conditions.

What do you think about this way?

>              end
>              remote.state, remote.error = state, err
>              if state == 'error_reconnect' then
> @@ -989,6 +992,7 @@ local function new_sm(host, port, opts, connection, greeting)
>      remote._on_schema_reload = trigger.new("on_schema_reload")
>      remote._on_disconnect = trigger.new("on_disconnect")
>      remote._on_connect = trigger.new("on_connect")
> +    remote._is_connected = false
>      remote._transport = create_transport(host, port, user, password, callback,
>                                           connection, greeting)
>      remote._transport.start()

Re: [Tarantool-patches] [PATCH 1/1] netbox: don't fire on_connect() at schema update

[Vladislav Shpilevoy]

Hi! Thanks for the review!

So, in short, you are against assuming that errno ~= nil always
means that it is a terminal state.

I don't think that error ~= nil is a bad idea, but ok, I don't mind
checking the states explicitly as it was before.

Force pushed to the branch:
=======================================================================

diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index 696b30fd9..c2e1bb9c4 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -933,7 +933,8 @@ local function new_sm(host, port, opts, connection, greeting)
                     remote._is_connected = true
                     remote._on_connect:run(remote)
                 end
-            elseif errno ~= nil then
+            elseif state == 'error' or state == 'error_reconnect' or
+                   state == 'closed' then
                 if was_connected then
                     remote._is_connected = false
                     remote._on_disconnect:run(remote)

=======================================================================

On 05/11/2019 16:12, Alexander Turenko wrote:
> The patch LGTM in the sense that it should work as expected as far as I
> see.
> 
> However I would discuss points I described below: I think we can write
> the patch in a bit more clean way. Vlad, please, let me know what do you
> think about this.
> 
> WBR, Alexander Turenko.
> 
>> diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
>> index 31a8c16b7..696b30fd9 100644
>> --- a/src/box/lua/net_box.lua
>> +++ b/src/box/lua/net_box.lua
>> @@ -927,14 +927,17 @@ local function new_sm(host, port, opts, connection, greeting)
>>      local function callback(what, ...)
>>          if what == 'state_changed' then
>>              local state, errno, err = ...
>> -            if (remote.state == 'active' or remote.state == 'fetch_schema') and
>> -               (state == 'error' or state == 'closed' or
>> -                state == 'error_reconnect') then
>> -               remote._on_disconnect:run(remote)
>> -            end
>> -            if remote.state ~= 'error' and remote.state ~= 'error_reconnect' and
>> -               state == 'active' then
>> -                remote._on_connect:run(remote)
>> +            local was_connected = remote._is_connected
>> +            if state == 'active' then
>> +                if not was_connected then
>> +                    remote._is_connected = true
>> +                    remote._on_connect:run(remote)
>> +                end
> 
> We splitted states into 'connected', 'neural' and 'disconnected' (a kind
> of tags or properties). We fire the trigger when a connection step into
> one of 'connected' states from one of 'disconnected' ones ('neural' are
> not counted). This looks okay.
> 
>> +            elseif errno ~= nil then
>> +                if was_connected then
>> +                    remote._is_connected = false
>> +                    remote._on_disconnect:run(remote)
>> +                end
> 
> Here we use `errno ~= nil` condition to determine whether a state is
> 'disconnected' one. The condition is true for 'error', 'error_reconnect'
> and 'closed' states. This way should give a correct behaviour.
> 
> When I saw the patch my question was whether a connection step into
> 'fetch_schema' state with `errno ~= nil`. It was not obvious for me what
> list of states are always set with some 'errno' value (however it is
> easy to deduce from set_state() calls). That is the first point.
> 
> The second is that I cannot prove (at least after brief look into the
> code) that 'errno' is newer `nil` / `box.NULL` for 'disconnected'
> states, because that are places where 'errno' is passed through a
> function.
> 
> I think we should at least give a comment that by using `errno ~= nil`
> we lean on assumption that we always step into 'error',
> 'error_reconnect' and 'closed' states with non-null 'errno' and that
> there is no other states that set 'errno'; but better don't assume this.
> 
> Let's consider unix errno: it should not be used as a primary source of
> information **whether** an error occurs. You always check a return code
> and only if it says that an error occurs we can consider 'errno' as a
> source of information **which kind** of error occurs.

Yes, but it is not unix errno. Here the error code is rather like a
return value. Nil means everything is ok, not nil means that the state
machine has reached a terminal state.

> That is why I generally against using of errno / diagnostic area as
> sources of information whether an error occurs: in context of Unix APIs
> this would be an improper usage.
> 
> I would mark states as 'connected' and 'disconnected' explicitly:
> 
>  | -- XXX: Give a comment why, say, 'fetch_schema' is not here.
>  | local function is_state_connected(state)
>  |     return state == 'active'
>  | end
>  |
>  | local disconnected_states = {
>  |     initial = true,
>  |     error = true,
>  |     error_reconnect = true,
>  |     closed = true,
>  | }
>  |
>  | local function is_state_disconnected(state)
>  |     return disconnected_states[state]
>  | end

This way is the same as it was before - checking of the states
explicitly, by their names. Previously I would need to patch
callback() on any update in the state set. In your proposal I
need to update these functions. So it is the same, it does not
simplify anything.

Re: [Tarantool-patches] [PATCH 1/1] netbox: don't fire on_connect() at schema update

[Alexander Turenko]

On Tue, Nov 05, 2019 at 05:12:09PM +0300, Vladislav Shpilevoy wrote:
> Hi! Thanks for the review!
> 
> So, in short, you are against assuming that errno ~= nil always
> means that it is a terminal state.

'error_reconnect' is not the terminal state, but yes: I'm against
assuming that errno ~= nil means anything about a current state.

> I don't think that error ~= nil is a bad idea, but ok, I don't mind
> checking the states explicitly as it was before.

I'm more comfortable with this approach, thanks!

The patch LGTM. CCed Kirill.

https://github.com/tarantool/tarantool/issues/4593
https://github.com/tarantool/tarantool/tree/gerold103/gh-4593-netbox-on_connect

> 
> Force pushed to the branch:
> =======================================================================
> 
> diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> index 696b30fd9..c2e1bb9c4 100644
> --- a/src/box/lua/net_box.lua
> +++ b/src/box/lua/net_box.lua
> @@ -933,7 +933,8 @@ local function new_sm(host, port, opts, connection, greeting)
>                      remote._is_connected = true
>                      remote._on_connect:run(remote)
>                  end
> -            elseif errno ~= nil then
> +            elseif state == 'error' or state == 'error_reconnect' or
> +                   state == 'closed' then
>                  if was_connected then
>                      remote._is_connected = false
>                      remote._on_disconnect:run(remote)
> 
> =======================================================================

> > Let's consider unix errno: it should not be used as a primary source of
> > information **whether** an error occurs. You always check a return code
> > and only if it says that an error occurs we can consider 'errno' as a
> > source of information **which kind** of error occurs.
> 
> Yes, but it is not unix errno. Here the error code is rather like a
> return value. Nil means everything is ok, not nil means that the state
> machine has reached a terminal state.

I think that the name 'errno' is not good here, but it is debatable and
surely should not be changed within the bugfix issue.

> > That is why I generally against using of errno / diagnostic area as
> > sources of information whether an error occurs: in context of Unix APIs
> > this would be an improper usage.
> > 
> > I would mark states as 'connected' and 'disconnected' explicitly:
> > 
> >  | -- XXX: Give a comment why, say, 'fetch_schema' is not here.
> >  | local function is_state_connected(state)
> >  |     return state == 'active'
> >  | end
> >  |
> >  | local disconnected_states = {
> >  |     initial = true,
> >  |     error = true,
> >  |     error_reconnect = true,
> >  |     closed = true,
> >  | }
> >  |
> >  | local function is_state_disconnected(state)
> >  |     return disconnected_states[state]
> >  | end
> 
> This way is the same as it was before - checking of the states
> explicitly, by their names. Previously I would need to patch
> callback() on any update in the state set. In your proposal I
> need to update these functions. So it is the same, it does not
> simplify anything.

is_state_disconnected() can be reused in set_state() too, but I don't
mind of checking states explicitly.

Re: [Tarantool-patches] [PATCH 1/1] netbox: don't fire on_connect() at schema update

[Kirill Yukhin]

Hello,

On 29 окт 22:43, Vladislav Shpilevoy wrote:
> There was a bug that netbox at any schema update called
> on_connect() triggers. This was due to overcomplicated logic of
> handling of changes in the netbox state machine. On_connect() was
> fired each time the machine entered 'active' state, even if its
> previous states were 'active' and then 'fetch_schema'. The latter
> state can be entered many times without reconnects.
> 
> Another bug was about on_disconnect() - it could be fired even if
> the connection never entered active state. For example, if its
> first 'fetch_schema' has failed.
> 
> Now there is an explicit flag showing the machine connect state.
> The triggers are fired only when it is changed, on 'active' and on
> any error states. Intermediate states (fetch_schema, auth) do not
> matter anymore.
> 
> Thanks @mtrempoltsev for the initial investigation and a draft
> fix.
> 
> Closes #4593
> ---
> Branch: https://github.com/tarantool/tarantool/tree/gerold103/gh-4593-netbox-on_connect
> Issue: https://github.com/tarantool/tarantool/issues/4593

I've checked your patch into 2.2 and master.

--
Regards, Kirill Yukhin

Re: [Tarantool-patches] [PATCH 1/1] netbox: don't fire on_connect() at schema update

[Alexander Turenko]

On Tue, Nov 05, 2019 at 07:34:44PM +0300, Kirill Yukhin wrote:
> Hello,
> 
> On 29 окт 22:43, Vladislav Shpilevoy wrote:
> > There was a bug that netbox at any schema update called
> > on_connect() triggers. This was due to overcomplicated logic of
> > handling of changes in the netbox state machine. On_connect() was
> > fired each time the machine entered 'active' state, even if its
> > previous states were 'active' and then 'fetch_schema'. The latter
> > state can be entered many times without reconnects.
> > 
> > Another bug was about on_disconnect() - it could be fired even if
> > the connection never entered active state. For example, if its
> > first 'fetch_schema' has failed.
> > 
> > Now there is an explicit flag showing the machine connect state.
> > The triggers are fired only when it is changed, on 'active' and on
> > any error states. Intermediate states (fetch_schema, auth) do not
> > matter anymore.
> > 
> > Thanks @mtrempoltsev for the initial investigation and a draft
> > fix.
> > 
> > Closes #4593
> > ---
> > Branch: https://github.com/tarantool/tarantool/tree/gerold103/gh-4593-netbox-on_connect
> > Issue: https://github.com/tarantool/tarantool/issues/4593
> 
> I've checked your patch into 2.2 and master.

This is the bugfix and should be pushed to 2.1 and 1.10 as well.

WBR, Alexander Turenko.

Re: [Tarantool-patches] [PATCH 1/1] netbox: don't fire on_connect() at schema update

[Kirill Yukhin]

Hello,

On 06 ноя 03:56, Alexander Turenko wrote:
> On Tue, Nov 05, 2019 at 07:34:44PM +0300, Kirill Yukhin wrote:
> > Hello,
> > 
> > On 29 окт 22:43, Vladislav Shpilevoy wrote:
> > > There was a bug that netbox at any schema update called
> > > on_connect() triggers. This was due to overcomplicated logic of
> > > handling of changes in the netbox state machine. On_connect() was
> > > fired each time the machine entered 'active' state, even if its
> > > previous states were 'active' and then 'fetch_schema'. The latter
> > > state can be entered many times without reconnects.
> > > 
> > > Another bug was about on_disconnect() - it could be fired even if
> > > the connection never entered active state. For example, if its
> > > first 'fetch_schema' has failed.
> > > 
> > > Now there is an explicit flag showing the machine connect state.
> > > The triggers are fired only when it is changed, on 'active' and on
> > > any error states. Intermediate states (fetch_schema, auth) do not
> > > matter anymore.
> > > 
> > > Thanks @mtrempoltsev for the initial investigation and a draft
> > > fix.
> > > 
> > > Closes #4593
> > > ---
> > > Branch: https://github.com/tarantool/tarantool/tree/gerold103/gh-4593-netbox-on_connect
> > > Issue: https://github.com/tarantool/tarantool/issues/4593
> > 
> > I've checked your patch into 2.2 and master.
> 
> This is the bugfix and should be pushed to 2.1 and 1.10 as well.

Done.

--
Regards, Kirill Yukhin