From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 04E144429A8 for ; Wed, 30 Oct 2019 00:38:00 +0300 (MSK) From: Vladislav Shpilevoy Date: Tue, 29 Oct 2019 22:43:30 +0100 Message-Id: <0706877cdc0598bb77489636dd22e852b7a50682.1572385348.git.v.shpilevoy@tarantool.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: [Tarantool-patches] [PATCH 1/1] netbox: don't fire on_connect() at schema update List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: tarantool-patches@dev.tarantool.org 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)