SERVER-10582 oplog_all_ops.js fails on Assertion failure, mutex problem when locking RSM

Avoid the mutex lock ordering inconsistency by not holding the _monitorMutex when calling ReplicaSetMonitor::checkAll.

4 files changed, 32 insertions, 13 deletions

diff --git a/src/mongo/client/dbclient_rs.cpp b/src/mongo/client/dbclient_rs.cpp
index 9bfc1868eca..26b47c26e84 100644
--- a/src/mongo/client/dbclient_rs.cpp
+++ b/src/mongo/client/dbclient_rs.cpp
@@ -68,6 +68,11 @@ namespace mongo {
             _stopRequested(false) {
         }
 
+        ~ReplicaSetMonitorWatcher() {
+            stop();
+            wait();
+        }
+
         virtual string name() const { return "ReplicaSetMonitorWatcher"; }
 
         void safeGo() {
@@ -98,16 +103,24 @@ namespace mongo {
         void run() {
             log() << "starting" << endl;
 
-            scoped_lock sl( _monitorMutex );
             // Added only for patching timing problems in test. Remove after tests
             // are fixed - see 392b933598668768bf12b1e41ad444aa3548d970.
             // Should not be needed after SERVER-7533 gets implemented and tests start
             // using it.
-            _stopRequestedCV.timed_wait(sl.boost(), boost::posix_time::seconds(10));
+            if (!inShutdown() && !StaticObserver::_destroyingStatics) {
+                scoped_lock sl( _monitorMutex );
+                _stopRequestedCV.timed_wait(sl.boost(), boost::posix_time::seconds(10));
+            }
 
             while ( !inShutdown() &&
-                    !_stopRequested &&
                     !StaticObserver::_destroyingStatics ) {
+                {
+                    scoped_lock sl( _monitorMutex );
+                    if (_stopRequested) {
+                        break;
+                    }
+                }
+
                 try {
                     ReplicaSetMonitor::checkAll();
                 }
@@ -118,9 +131,15 @@ namespace mongo {
                     error() << "unknown error" << endl;
                 }
 
+                scoped_lock sl( _monitorMutex );
+                if (_stopRequested) {
+                    break;
+                }
+
                 _stopRequestedCV.timed_wait(sl.boost(), boost::posix_time::seconds(10));
             }
 
+            scoped_lock sl( _monitorMutex );
             _started = false;
         }
 
@@ -1286,7 +1305,7 @@ namespace mongo {
         return false;
     }
 
-    void ReplicaSetMonitor::clearAll() {
+    void ReplicaSetMonitor::cleanup() {
         {
             scoped_lock lock(_setsLock);
             _sets.clear();
diff --git a/src/mongo/client/dbclient_rs.h b/src/mongo/client/dbclient_rs.h
index 61797cb7f73..a97d77be2bb 100644
--- a/src/mongo/client/dbclient_rs.h
+++ b/src/mongo/client/dbclient_rs.h
@@ -229,7 +229,7 @@ namespace mongo {
          * fully terminates as it will cause a deadlock. This is intended for performing cleanups
          * in unit tests.
          */
-        static void clearAll();
+        static void cleanup();
 
         ~ReplicaSetMonitor();
 
diff --git a/src/mongo/client/dbclient_rs_test.cpp b/src/mongo/client/dbclient_rs_test.cpp
index 43bc892653b..9cef022a91d 100644
--- a/src/mongo/client/dbclient_rs_test.cpp
+++ b/src/mongo/client/dbclient_rs_test.cpp
@@ -91,7 +91,7 @@ namespace {
         }
 
         void tearDown() {
-            ReplicaSetMonitor::clearAll();
+            ReplicaSetMonitor::cleanup();
             _replSet.reset();
 
             // TODO: remove this after we remove replSetGetStatus from ReplicaSetMonitor.
@@ -176,7 +176,7 @@ namespace {
         }
 
         void tearDown() {
-            ReplicaSetMonitor::clearAll();
+            ReplicaSetMonitor::cleanup();
             _replSet.reset();
 
             // TODO: remove this after we remove replSetGetStatus from ReplicaSetMonitor.
@@ -249,7 +249,7 @@ namespace {
         }
 
         void tearDown() {
-            ReplicaSetMonitor::clearAll();
+            ReplicaSetMonitor::cleanup();
             _replSet.reset();
 
             // TODO: remove this after we remove replSetGetStatus from ReplicaSetMonitor.
@@ -337,7 +337,7 @@ namespace {
         }
 
         void tearDown() {
-            ReplicaSetMonitor::clearAll();
+            ReplicaSetMonitor::cleanup();
             _replSet.reset();
 
             // TODO: remove this after we remove replSetGetStatus from ReplicaSetMonitor.
@@ -487,7 +487,7 @@ namespace {
 
         void tearDown() {
             ConnectionString::setConnectionHook(_originalConnectionHook);
-            ReplicaSetMonitor::clearAll();
+            ReplicaSetMonitor::cleanup();
             _replSet.reset();
 
             // TODO: remove this after we remove replSetGetStatus from ReplicaSetMonitor.
diff --git a/src/mongo/dbtests/replica_set_monitor_test.cpp b/src/mongo/dbtests/replica_set_monitor_test.cpp
index 7d05b9bb2e5..cd79599c098 100644
--- a/src/mongo/dbtests/replica_set_monitor_test.cpp
+++ b/src/mongo/dbtests/replica_set_monitor_test.cpp
@@ -1546,7 +1546,7 @@ namespace mongo_test {
 
         void tearDown() {
             ConnectionString::setConnectionHook(_originalConnectionHook);
-            ReplicaSetMonitor::clearAll();
+            ReplicaSetMonitor::cleanup();
             _replSet.reset();
             mongo::ScopedDbConnection::clearPool();
         }
@@ -1626,7 +1626,7 @@ namespace mongo_test {
             replMonitor->check(); // Force refresh -> should not crash
         }
 
-        ReplicaSetMonitor::clearAll();
+        ReplicaSetMonitor::cleanup();
         ConnectionString::setConnectionHook(originalConnHook);
         mongo::ScopedDbConnection::clearPool();
     }
@@ -1667,7 +1667,7 @@ namespace mongo_test {
 
         void tearDown() {
             ConnectionString::setConnectionHook(_originalConnectionHook);
-            ReplicaSetMonitor::clearAll();
+            ReplicaSetMonitor::cleanup();
             _replSet.reset();
         }