Introduce a new function for the case of dropping a master because we synced - remove a deadlock.bug26125

2 files changed, 26 insertions, 6 deletions

diff --git a/src/rabbit_mirror_queue_misc.erl b/src/rabbit_mirror_queue_misc.erl
index 256543de..b0f092a9 100644
--- a/src/rabbit_mirror_queue_misc.erl
+++ b/src/rabbit_mirror_queue_misc.erl
@@ -21,7 +21,8 @@
          report_deaths/4, store_updated_slaves/1,
          initial_queue_node/2, suggested_queue_nodes/1,
          is_mirrored/1, update_mirrors/2, validate_policy/1,
-         maybe_auto_sync/1, log_info/3, log_warning/3]).
+         maybe_auto_sync/1, maybe_drop_master_after_sync/1,
+         log_info/3, log_warning/3]).
 
 %% for testing only
 -export([module/1]).
@@ -57,6 +58,7 @@
 -spec(is_mirrored/1 :: (rabbit_types:amqqueue()) -> boolean()).
 -spec(update_mirrors/2 ::
         (rabbit_types:amqqueue(), rabbit_types:amqqueue()) -> 'ok').
+-spec(maybe_drop_master_after_sync/1 :: (rabbit_types:amqqueue()) -> 'ok').
 -spec(maybe_auto_sync/1 :: (rabbit_types:amqqueue()) -> 'ok').
 -spec(log_info/3 :: (rabbit_amqqueue:name(), string(), [any()]) -> 'ok').
 -spec(log_warning/3 :: (rabbit_amqqueue:name(), string(), [any()]) -> 'ok').
@@ -346,6 +348,26 @@ update_mirrors0(OldQ = #amqqueue{name = QName},
     maybe_auto_sync(NewQ),
     ok.
 
+%% The arrival of a newly synced slave may cause the master to die if
+%% the policy does not want the master but it has been kept alive
+%% because there were no synced slaves.
+%%
+%% We don't just call update_mirrors/2 here since that could decide to
+%% start a slave for some other reason, and since we are the slave ATM
+%% that allows complicated deadlocks.
+maybe_drop_master_after_sync(Q = #amqqueue{name = QName,
+                                           pid  = MPid}) ->
+    {DesiredMNode, DesiredSNodes} = suggested_queue_nodes(Q),
+    case node(MPid) of
+        DesiredMNode -> ok;
+        OldMNode     -> false = lists:member(OldMNode, DesiredSNodes), %% [0]
+                        drop_mirror(QName, OldMNode)
+    end,
+    ok.
+%% [0] ASSERTION - if the policy wants the master to change, it has
+%% not just shuffled it into the slaves. All our modes ensure this
+%% does not happen, but we should guard against a misbehaving plugin.
+
 %%----------------------------------------------------------------------------
 
 validate_policy(KeyList) ->
diff --git a/src/rabbit_mirror_queue_slave.erl b/src/rabbit_mirror_queue_slave.erl
index f6acd91a..ee889f84 100644
--- a/src/rabbit_mirror_queue_slave.erl
+++ b/src/rabbit_mirror_queue_slave.erl
@@ -922,8 +922,6 @@ update_ram_duration(BQ, BQS) ->
         rabbit_memory_monitor:report_ram_duration(self(), RamDuration),
     BQ:set_ram_duration_target(DesiredDuration, BQS1).
 
-%% [1] - the arrival of this newly synced slave may cause the master to die if
-%% the admin has requested a migration-type change to policy.
 record_synchronised(#amqqueue { name = QName }) ->
     Self = self(),
     case rabbit_misc:execute_mnesia_transaction(
@@ -934,9 +932,9 @@ record_synchronised(#amqqueue { name = QName }) ->
                        [Q1 = #amqqueue { sync_slave_pids = SSPids }] ->
                            Q2 = Q1#amqqueue{sync_slave_pids = [Self | SSPids]},
                            rabbit_mirror_queue_misc:store_updated_slaves(Q2),
-                           {ok, Q1, Q2}
+                           {ok, Q2}
                    end
            end) of
-        ok           -> ok;
-        {ok, Q1, Q2} -> rabbit_mirror_queue_misc:update_mirrors(Q1, Q2) %% [1]
+        ok      -> ok;
+        {ok, Q} -> rabbit_mirror_queue_misc:maybe_drop_master_after_sync(Q)
     end.