From 4556a8f69b89a21768b5e8b1863663532e0fae2c Mon Sep 17 00:00:00 2001
From: Alan Conway <aconway@apache.org>
Date: Wed, 12 Sep 2012 19:18:43 +0000
Subject: NO-JIRA: HA fix deadlock between Link and HaBroker

HaBroker::setBrokerUrl was calling Link::setUrl (via Backup::setBrokerUrl) with
lock held Link::ioThreadProcessing called HaBroker::getStaus with lock held.

In HaBroker, moved calls to Backup out of lock scope in HaBroker::setMembership
and haBroker::setBrokerUrl.

git-svn-id: https://svn.apache.org/repos/asf/qpid/trunk@1384095 13f79535-47bb-0310-9956-ffa450edef68
---
 qpid/cpp/src/qpid/ha/HaBroker.cpp | 48 +++++++++++++++++++++++----------------
 qpid/cpp/src/qpid/ha/HaBroker.h   |  5 ++--
 2 files changed, 31 insertions(+), 22 deletions(-)

(limited to 'qpid/cpp/src')

diff --git a/qpid/cpp/src/qpid/ha/HaBroker.cpp b/qpid/cpp/src/qpid/ha/HaBroker.cpp
index ffbcb684bc..b555b33e13 100644
--- a/qpid/cpp/src/qpid/ha/HaBroker.cpp
+++ b/qpid/cpp/src/qpid/ha/HaBroker.cpp
@@ -126,14 +126,16 @@ HaBroker::~HaBroker() {
     broker.getConnectionObservers().remove(observer);
 }
 
+// Called from ManagementMethod on promote.
 void HaBroker::recover() {
-    auto_ptr<Backup> b;
+    boost::shared_ptr<Backup> b;
     {
         Mutex::ScopedLock l(lock);
         // No longer replicating, close link. Note: link must be closed before we
         // setStatus(RECOVERING) as that will remove our broker info from the
         // outgoing link properties so we won't recognize self-connects.
         b = backup;
+        backup.reset();         // Reset in lock.
     }
     b.reset();                  // Call destructor outside of lock.
     BrokerInfo::Set backups;
@@ -224,14 +226,18 @@ void HaBroker::updateClientUrl(Mutex::ScopedLock&) {
 }
 
 void HaBroker::setBrokerUrl(const Url& url) {
-    Mutex::ScopedLock l(lock);
-    if (url.empty()) throw Url::Invalid("HA broker URL is empty");
-    brokerUrl = url;
-    mgmtObject->set_brokersUrl(brokerUrl.str());
-    QPID_LOG(info, logPrefix << "Broker URL set to: " << url);
-    if (backup.get()) backup->setBrokerUrl(brokerUrl);
-    // Updating broker URL also updates defaulted client URL:
-    if (clientUrl.empty()) updateClientUrl(l);
+    boost::shared_ptr<Backup> b;
+    {
+        Mutex::ScopedLock l(lock);
+        if (url.empty()) throw Url::Invalid("HA broker URL is empty");
+        brokerUrl = url;
+        mgmtObject->set_brokersUrl(brokerUrl.str());
+        QPID_LOG(info, logPrefix << "Broker URL set to: " << url);
+        // Updating broker URL also updates defaulted client URL:
+        if (clientUrl.empty()) updateClientUrl(l);
+        b = backup;
+    }
+    if (b) b->setBrokerUrl(url); // Oustside lock, avoid deadlock
 }
 
 std::vector<Url> HaBroker::getKnownBrokers() const {
@@ -302,17 +308,21 @@ void HaBroker::membershipUpdated(Mutex::ScopedLock&) {
 }
 
 void HaBroker::setMembership(const Variant::List& brokers) {
-    Mutex::ScopedLock l(lock);
-    membership.assign(brokers);
-    QPID_LOG(info, logPrefix << "Membership update: " <<  membership);
-    BrokerInfo info;
-    // Update my status to what the primary says it is.  The primary can toggle
-    // status between READY and CATCHUP based on the state of our subscriptions.
-    if (membership.get(systemId, info) && status != info.getStatus()) {
-        setStatus(info.getStatus(), l);
-        if (backup.get()) backup->setStatus(status);
+    boost::shared_ptr<Backup> b;
+    {
+        Mutex::ScopedLock l(lock);
+        membership.assign(brokers);
+        QPID_LOG(info, logPrefix << "Membership update: " <<  membership);
+        BrokerInfo info;
+        // Update my status to what the primary says it is.  The primary can toggle
+        // status between READY and CATCHUP based on the state of our subscriptions.
+        if (membership.get(systemId, info) && status != info.getStatus()) {
+            setStatus(info.getStatus(), l);
+            b = backup;
+        }
+        membershipUpdated(l);
     }
-    membershipUpdated(l);
+    if (b) b->setStatus(status); // Oustside lock, avoid deadlock
 }
 
 void HaBroker::resetMembership(const BrokerInfo& b) {
diff --git a/qpid/cpp/src/qpid/ha/HaBroker.h b/qpid/cpp/src/qpid/ha/HaBroker.h
index 7dabe6e35b..3b39b9ec3d 100644
--- a/qpid/cpp/src/qpid/ha/HaBroker.h
+++ b/qpid/cpp/src/qpid/ha/HaBroker.h
@@ -32,7 +32,6 @@
 #include "qmf/org/apache/qpid/ha/HaBroker.h"
 #include "qpid/management/Manageable.h"
 #include "qpid/types/Variant.h"
-#include <memory>
 #include <set>
 #include <boost/shared_ptr.hpp>
 
@@ -122,8 +121,8 @@ class HaBroker : public management::Manageable
 
     mutable sys::Mutex lock;
     boost::shared_ptr<ConnectionObserver> observer; // Used by Backup and Primary
-    std::auto_ptr<Backup> backup;
-    std::auto_ptr<Primary> primary;
+    boost::shared_ptr<Backup> backup;
+    boost::shared_ptr<Primary> primary;
     qmf::org::apache::qpid::ha::HaBroker* mgmtObject;
     Url clientUrl, brokerUrl;
     std::vector<Url> knownBrokers;
-- 
cgit v1.2.1