[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)