Merge pull request #2468 from rabbitmq/avoid-double-channel-count-decrease

Attempt at improving the channel count consistency (+ improvements to testcases)

4 files changed, 38 insertions, 32 deletions

diff --git a/src/rabbit_channel_tracking.erl b/src/rabbit_channel_tracking.erl
index 52b84dc90b..42ab664a06 100644
--- a/src/rabbit_channel_tracking.erl
+++ b/src/rabbit_channel_tracking.erl
@@ -102,9 +102,9 @@ handle_cast({connection_closed, ConnDetails}) ->
             rabbit_log_connection:info(
                 "Closing all channels from connection '~p' "
                 "because it has been closed", [pget(name, ConnDetails)]),
+            %% Shutting down channels will take care of unregistering the
+            %% corresponding tracking.
             shutdown_tracked_items(TrackedChs, undefined),
-            [unregister_tracked(rabbit_tracking:id(ThisNode, Name)) ||
-                #tracked_channel{name = Name} <- TrackedChs],
             ok;
         _DifferentNode ->
             ok
diff --git a/test/many_node_ha_SUITE.erl b/test/many_node_ha_SUITE.erl
index 6dc469f6f6..ece7dc8830 100644
--- a/test/many_node_ha_SUITE.erl
+++ b/test/many_node_ha_SUITE.erl
@@ -12,6 +12,11 @@
 
 -compile(export_all).
 
+suite() ->
+    [
+     {timetrap, {minutes, 5}}
+    ].
+
 all() ->
     [
       {group, cluster_size_6}
diff --git a/test/per_user_connection_channel_tracking_SUITE.erl b/test/per_user_connection_channel_tracking_SUITE.erl
index 923bdea541..8b4bd91d09 100644
--- a/test/per_user_connection_channel_tracking_SUITE.erl
+++ b/test/per_user_connection_channel_tracking_SUITE.erl
@@ -10,6 +10,7 @@
 -include_lib("common_test/include/ct.hrl").
 -include_lib("amqp_client/include/amqp_client.hrl").
 -include_lib("eunit/include/eunit.hrl").
+-include_lib("rabbitmq_ct_helpers/include/rabbit_assert.hrl").
 
 -compile(export_all).
 
@@ -148,13 +149,13 @@ single_node_user_connection_channel_tracking(Config) ->
     ?assertEqual(true, is_process_alive(Conn1)),
     ?assertEqual(true, is_process_alive(Chan1)),
     close_channels([Chan1]),
-    ?assertEqual(0, count_channels_in(Config, Username)),
-    ?assertEqual(0, tracked_user_channel_count(Config, Username)),
-    ?assertEqual(false, is_process_alive(Chan1)),
+    ?awaitMatch(0, count_channels_in(Config, Username), 20000),
+    ?awaitMatch(0, tracked_user_channel_count(Config, Username), 20000),
+    ?awaitMatch(false, is_process_alive(Chan1), 20000),
     close_connections([Conn1]),
-    ?assertEqual(0, length(connections_in(Config, Username))),
-    ?assertEqual(0, tracked_user_connection_count(Config, Username)),
-    ?assertEqual(false, is_process_alive(Conn1)),
+    ?awaitMatch(0, length(connections_in(Config, Username)), 20000),
+    ?awaitMatch(0, tracked_user_connection_count(Config, Username), 20000),
+    ?awaitMatch(false, is_process_alive(Conn1), 20000),
 
     [Conn2] = open_connections(Config, [{0, Username2}]),
     Chans2  = [_|_] = open_channels(Conn2, 5),
@@ -183,9 +184,9 @@ single_node_user_connection_channel_tracking(Config) ->
     [?assertEqual(true, is_process_alive(Ch)) || Ch <- Chans4],
     kill_connections([Conn4]),
     [#tracked_connection{username = Username}] = connections_in(Config, Username),
-    ?assertEqual(5, count_channels_in(Config, Username)),
-    ?assertEqual(1, tracked_user_connection_count(Config, Username)),
-    ?assertEqual(5, tracked_user_channel_count(Config, Username)),
+    ?awaitMatch(5, count_channels_in(Config, Username), 20000),
+    ?awaitMatch(1, tracked_user_connection_count(Config, Username), 20000),
+    ?awaitMatch(5, tracked_user_channel_count(Config, Username), 20000),
     ?assertEqual(false, is_process_alive(Conn4)),
     [?assertEqual(false, is_process_alive(Ch)) || Ch <- Chans4],
 
@@ -201,15 +202,15 @@ single_node_user_connection_channel_tracking(Config) ->
     [?assertEqual(true, is_process_alive(Ch)) || Ch <- Chans5],
 
     close_channels(Chans2 ++ Chans3 ++ Chans5),
-    ?assertEqual(0, length(all_channels(Config))),
-    ?assertEqual(0, tracked_user_channel_count(Config, Username)),
-    ?assertEqual(0, tracked_user_channel_count(Config, Username2)),
+    ?awaitMatch(0, length(all_channels(Config)), 20000),
+    ?awaitMatch(0, tracked_user_channel_count(Config, Username), 20000),
+    ?awaitMatch(0, tracked_user_channel_count(Config, Username2), 20000),
 
     close_connections([Conn2, Conn3, Conn5]),
     rabbit_ct_broker_helpers:delete_user(Config, Username2),
-    ?assertEqual(0, tracked_user_connection_count(Config, Username)),
-    ?assertEqual(0, tracked_user_connection_count(Config, Username2)),
-    ?assertEqual(0, length(all_connections(Config))).
+    ?awaitMatch(0, tracked_user_connection_count(Config, Username), 20000),
+    ?awaitMatch(0, tracked_user_connection_count(Config, Username2), 20000),
+    ?awaitMatch(0, length(all_connections(Config)), 20000).
 
 single_node_user_deletion(Config) ->
     set_tracking_execution_timeout(Config, 100),
@@ -264,8 +265,8 @@ single_node_user_deletion(Config) ->
     [?assertEqual(false, is_process_alive(Ch)) || Ch <- Chans2],
 
     %% ensure vhost entry is cleared after 'tracking_execution_timeout'
-    ?assertEqual(false, exists_in_tracked_connection_per_user_table(Config, Username2)),
-    ?assertEqual(false, exists_in_tracked_channel_per_user_table(Config, Username2)),
+    ?awaitMatch(false, exists_in_tracked_connection_per_user_table(Config, Username2), 20000),
+    ?awaitMatch(false, exists_in_tracked_channel_per_user_table(Config, Username2), 20000),
 
     ?assertEqual(1, count_connections_in(Config, Username)),
     ?assertEqual(5, count_channels_in(Config, Username)),
@@ -275,12 +276,12 @@ single_node_user_deletion(Config) ->
     [?assertEqual(true, is_process_alive(Ch)) || Ch <- Chans1],
 
     close_channels(Chans1),
-    ?assertEqual(0, count_channels_in(Config, Username)),
-    ?assertEqual(0, tracked_user_channel_count(Config, Username)),
+    ?awaitMatch(0, count_channels_in(Config, Username), 20000),
+    ?awaitMatch(0, tracked_user_channel_count(Config, Username), 20000),
 
     close_connections([Conn1]),
-    ?assertEqual(0, count_connections_in(Config, Username)),
-    ?assertEqual(0, tracked_user_connection_count(Config, Username)).
+    ?awaitMatch(0, count_connections_in(Config, Username), 20000),
+    ?awaitMatch(0, tracked_user_connection_count(Config, Username), 20000).
 
 single_node_vhost_deletion(Config) ->
     set_tracking_execution_timeout(Config, 100),
@@ -456,12 +457,12 @@ cluster_user_deletion(Config) ->
     ?assertEqual(false, exists_in_tracked_channel_per_user_table(Config, 1, Username2)),
 
     close_channels(Chans1),
-    ?assertEqual(0, count_channels_in(Config, Username)),
-    ?assertEqual(0, tracked_user_channel_count(Config, Username)),
+    ?awaitMatch(0, count_channels_in(Config, Username), 20000),
+    ?awaitMatch(0, tracked_user_channel_count(Config, Username), 20000),
 
     close_connections([Conn1]),
-    ?assertEqual(0, count_connections_in(Config, Username)),
-    ?assertEqual(0, tracked_user_connection_count(Config, Username)).
+    ?awaitMatch(0, count_connections_in(Config, Username), 20000),
+    ?awaitMatch(0, tracked_user_connection_count(Config, Username), 20000).
 
 cluster_vhost_deletion(Config) ->
     set_tracking_execution_timeout(Config, 0, 100),
@@ -665,12 +666,12 @@ cluster_node_removed(Config) ->
     [?assertEqual(true, is_process_alive(Ch)) || Ch <- Chans1],
 
     close_channels(Chans1),
-    ?assertEqual(0, count_channels_in(Config, Username)),
-    ?assertEqual(0, tracked_user_channel_count(Config, Username)),
+    ?awaitMatch(0, count_channels_in(Config, Username), 20000),
+    ?awaitMatch(0, tracked_user_channel_count(Config, Username), 20000),
 
     close_connections([Conn1]),
-    ?assertEqual(0, count_connections_in(Config, Username)),
-    ?assertEqual(0, tracked_user_connection_count(Config, Username)).
+    ?awaitMatch(0, count_connections_in(Config, Username), 20000),
+    ?awaitMatch(0, tracked_user_connection_count(Config, Username), 20000).
 
 %% -------------------------------------------------------------------
 %% Helpers
diff --git a/test/quorum_queue_utils.erl b/test/quorum_queue_utils.erl
index caabd617ae..923ed0bb00 100644
--- a/test/quorum_queue_utils.erl
+++ b/test/quorum_queue_utils.erl
@@ -53,7 +53,7 @@ wait_for_messages(Servers, QName, Number, Fun, N) ->
     end.
 
 wait_for_messages(Config, Stats) ->
-    wait_for_messages(Config, lists:sort(Stats), 10).
+    wait_for_messages(Config, lists:sort(Stats), 60).
 
 wait_for_messages(Config, Stats, 0) ->
     ?assertEqual(Stats,