CXX-103 CXX-334 kill StaticObserver and enable RSMW to be stopped/started

Author: amidvidy

src/SConscript.client

@@ -154,7 +154,6 @@ clientSourceBasic = [
     'mongo/util/text.cpp',
     'mongo/util/time_support.cpp',
     'mongo/util/timer.cpp',
-    'mongo/util/util.cpp',
     'third_party/murmurhash3/MurmurHash3.cpp',
     ]
 

src/mongo/client/dbclient_rs_test.cpp

@@ -67,14 +67,14 @@ namespace {
     class BasicRS: public mongo::unittest::Test {
     protected:
         void setUp() {
-            ReplicaSetMonitor::cleanup();
+            ReplicaSetMonitor::initialize();
             _replSet.reset(new MockReplicaSet("test", 2));
             ConnectionString::setConnectionHook(
                     mongo::MockConnRegistry::get()->getConnStrHook());
         }
 
         void tearDown() {
-            ReplicaSetMonitor::cleanup();
+            ReplicaSetMonitor::shutdown();
             _replSet.reset();
         }
 
@@ -150,7 +150,7 @@ namespace {
     class AllNodesDown: public mongo::unittest::Test {
     protected:
         void setUp() {
-            ReplicaSetMonitor::cleanup();
+            ReplicaSetMonitor::initialize();
             _replSet.reset(new MockReplicaSet("test", 2));
             ConnectionString::setConnectionHook(
                     mongo::MockConnRegistry::get()->getConnStrHook());
@@ -163,7 +163,7 @@ namespace {
         }
 
         void tearDown() {
-            ReplicaSetMonitor::cleanup();
+            ReplicaSetMonitor::shutdown();
             _replSet.reset();
         }
 
@@ -226,15 +226,15 @@ namespace {
     class PrimaryDown: public mongo::unittest::Test {
     protected:
         void setUp() {
-            ReplicaSetMonitor::cleanup();
+            ReplicaSetMonitor::initialize();
             _replSet.reset(new MockReplicaSet("test", 2));
             ConnectionString::setConnectionHook(
                     mongo::MockConnRegistry::get()->getConnStrHook());
             _replSet->kill(_replSet->getPrimary());
         }
 
         void tearDown() {
-            ReplicaSetMonitor::cleanup();
+            ReplicaSetMonitor::shutdown();
             _replSet.reset();
         }
 
@@ -311,7 +311,7 @@ namespace {
     class SecondaryDown: public mongo::unittest::Test {
     protected:
         void setUp() {
-            ReplicaSetMonitor::cleanup();
+            ReplicaSetMonitor::initialize();
             _replSet.reset(new MockReplicaSet("test", 2));
             ConnectionString::setConnectionHook(
                     mongo::MockConnRegistry::get()->getConnStrHook());
@@ -320,7 +320,7 @@ namespace {
         }
 
         void tearDown() {
-            ReplicaSetMonitor::cleanup();
+            ReplicaSetMonitor::shutdown();
             _replSet.reset();
 
         }
@@ -406,7 +406,7 @@ namespace {
 
             // This shuts down the background RSMWatcher thread and prevents it from running. These
             // tests depend on controlling when the RSMs are updated.
-            ReplicaSetMonitor::cleanup();
+            ReplicaSetMonitor::initialize();
 
             _replSet.reset(new MockReplicaSet("test", 5));
             _originalConnectionHook = ConnectionString::getConnectionHook();
@@ -477,7 +477,7 @@ namespace {
             ReplicaSetMonitor::useDeterministicHostSelection = false;
 
             ConnectionString::setConnectionHook(_originalConnectionHook);
-            ReplicaSetMonitor::cleanup();
+            ReplicaSetMonitor::shutdown();
             _replSet.reset();
 
         }

src/mongo/client/init.cpp

@@ -88,6 +88,11 @@ namespace client {
             if (!result.isOK())
                 return result;
 
+            result = ReplicaSetMonitor::initialize();
+            if (!result.isOK()) {
+                return result;
+            }
+
             return Status::OK();
         }
         else if (initStatus == 1) {
@@ -108,7 +113,10 @@ namespace client {
 
         if (initStatus == 1) {
 
-            ReplicaSetMonitor::cleanup();
+            Status result = ReplicaSetMonitor::shutdown(Options::current().autoShutdownGracePeriodMillis());
+            if (!result.isOK()) {
+                return result;
+            }
             shutdownNetworking();
             return Status::OK();
         }

src/mongo/client/replica_set_monitor.cpp

@@ -23,14 +23,14 @@
 #include <limits>
 
 #include <boost/make_shared.hpp>
+#include <boost/scoped_ptr.hpp>
 #include <boost/thread.hpp>
 #include <boost/thread/condition.hpp>
 #include <boost/thread/mutex.hpp>
 
 #include "mongo/client/options.h"
 #include "mongo/client/private/options.h"
 #include "mongo/client/replica_set_monitor_internal.h"
-#include "mongo/util/concurrency/mutex.h" // for StaticObserver
 #include "mongo/util/background.h"
 #include "mongo/util/debug_util.h"
 #include "mongo/util/log.h"
@@ -66,29 +66,27 @@ namespace {
 
     const double socketTimeoutSecs = 5;
 
-    /*  Replica Set Monitor shared state:
-     *      If a program (such as one built with the C++ driver) exits (by either calling exit()
-     *      or by returning from main()), static objects will be destroyed in the reverse order
-     *      of their creation (within each translation unit (source code file)).  This makes it
-     *      vital that the order be explicitly controlled within the source file so that destroyed
-     *      objects never reference objects that have been destroyed earlier.
+    /*  Replica Set Monitor global state
      *
-     *      The order chosen below is intended to allow safe destruction in reverse order from
-     *      construction order:
-     *          setsLock                 -- mutex protecting _seedServers and _sets, destroyed last
+     *          watcherLifetimeLock       -- mutex held during creation/destruction of
+     *                                      replicaSetMonitorWatcher
+     *          replicaSetMonitorWatcher -- background job to check Replica Set members
+     *          setsLock                 -- mutex protecting seedServers and sets
      *          seedServers              -- list (map) of servers
      *          sets                     -- list (map) of ReplicaSetMonitors
-     *          replicaSetMonitorWatcher -- background job to check Replica Set members
-     *          staticObserver           -- sentinel to detect process termination
-     *
-     *      Related to:
-     *          SERVER-8891 -- Simple client fail with segmentation fault in mongoclient library
      *
      *      Mutex locking order:
-     *          Don't lock setsLock while holding any SetState::mutex. It is however safe to grab a
-     *          SetState::mutex without holder setsLock, but then you can't grab setsLock until you
-     *          release the SetState::mutex.
+     *          watcherLock should be acquired first when acquiring it and any other lock.
+     *          Don't lock setsLock while holding any SetState::mutex.
+     *          It is however safe to grab a SetState::mutex without holding setsLock, but
+     *          then you can't grab setsLock until you release the SetState::mutex.
      */
+
+    class ReplicaSetMonitorWatcher;
+
+    boost::mutex watcherLifetimeLock;
+    boost::scoped_ptr<ReplicaSetMonitorWatcher> replicaSetMonitorWatcher;
+        
     boost::mutex setsLock;
     StringMap<set<HostAndPort> > seedServers;
     StringMap<ReplicaSetMonitorPtr> sets;
@@ -105,9 +103,6 @@ namespace {
         ~ReplicaSetMonitorWatcher() {
             stop();
 
-            // We relying on the fact that if the monitor was rerun again, wait will not hang
-            // because _destroyingStatics will make the run method exit immediately.
-            dassert(StaticObserver::_destroyingStatics);
             if (running()) {
                 wait();
             }
@@ -139,16 +134,7 @@ namespace {
         void run() {
             log() << "starting"; // includes thread name in output
 
-            // 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.
-            if (!StaticObserver::_destroyingStatics) {
-                boost::unique_lock<boost::mutex> sl( _monitorMutex );
-                _stopRequestedCV.timed_wait(sl, boost::posix_time::seconds(10));
-            }
-
-            while ( !StaticObserver::_destroyingStatics ) {
+            while ( true ) {
                 {
                     boost::lock_guard<boost::mutex> sl( _monitorMutex );
                     if (_stopRequested) {
@@ -207,9 +193,7 @@ namespace {
 
         boost::condition_variable _stopRequestedCV;
         bool _stopRequested;
-    } replicaSetMonitorWatcher;
-
-    StaticObserver staticObserver;
+    };
 
     //
     // Helpers for stl algorithms
@@ -356,7 +340,11 @@ namespace {
         if ( ! m )
             m = boost::make_shared<ReplicaSetMonitor>( name , servers );
 
-        replicaSetMonitorWatcher.safeGo();
+        // Don't need to hold the lifetime lock for safeGo as
+        // 1) we assume the monitor is created as the contract of this class is such that initialize()
+        //    must have been called.
+        // 2) replicaSetMonitorWatcher synchronizes safeGo internally using the _monitorMutex
+        replicaSetMonitorWatcher->safeGo();
     }
 
     ReplicaSetMonitorPtr ReplicaSetMonitor::get(const string& name,
@@ -374,7 +362,9 @@ namespace {
                 ReplicaSetMonitorPtr& m = sets[name];
                 invariant( !m );
                 m.reset( new ReplicaSetMonitor( name, j->second ) );
-                replicaSetMonitorWatcher.safeGo();
+                // see above comment in createIfNeeded for why we don't need the
+                // watcherLifetimeLock
+                replicaSetMonitorWatcher->safeGo();
                 return m;
             }
         }
@@ -444,14 +434,40 @@ namespace {
         hosts.done();
     }
 
-    void ReplicaSetMonitor::cleanup() {
+    // Users shouldn't need to call this more than once, but our tests do.
+    // Note that this doesn't actually start the watcher, it just creates it.
+    // The watcher is lazily started when we start monitoring our first replica
+    // set, so we don't have to pay the cost of the extra thread unless needed.
+    Status ReplicaSetMonitor::initialize() {
+        boost::lock_guard<boost::mutex> lock(watcherLifetimeLock);
+        if (replicaSetMonitorWatcher) {
+            return Status(ErrorCodes::IllegalOperation,
+                          "ReplicaSetMonitorWatcher has already been initialized");
+        }
+        replicaSetMonitorWatcher.reset(new ReplicaSetMonitorWatcher());
+        return Status::OK();
+    }
+
+    Status ReplicaSetMonitor::shutdown(int gracePeriodMillis) {
+        boost::lock_guard<boost::mutex> lock(watcherLifetimeLock);
+        if (!replicaSetMonitorWatcher) {
+            return Status(ErrorCodes::IllegalOperation,
+                          "ReplicaSetMonitorWatcher has not been initialized");
+        }
         // Call cancel first, in case the RSMW was never started.
-        replicaSetMonitorWatcher.cancel();
-        replicaSetMonitorWatcher.stop();
-        replicaSetMonitorWatcher.wait();
-        boost::lock_guard<boost::mutex> lock(setsLock);
+        replicaSetMonitorWatcher->cancel();
+        replicaSetMonitorWatcher->stop();
+        bool success = replicaSetMonitorWatcher->wait(gracePeriodMillis);
+        if (!success) {
+            return Status(ErrorCodes::InternalError,
+                          "Timed out waiting for ReplicaSetMonitorWatcher to shutdown");
+        }
+        replicaSetMonitorWatcher.reset();
+        boost::lock_guard<boost::mutex> lockSets(setsLock);
         sets = StringMap<ReplicaSetMonitorPtr>();
         seedServers = StringMap<set<HostAndPort> >();
+
+        return Status::OK();
     }
 
     Refresher::Refresher(const SetStatePtr& setState)

src/mongo/client/replica_set_monitor.h

@@ -158,12 +158,25 @@ namespace mongo {
          */
         static void MONGO_CLIENT_FUNC setConfigChangeHook(ConfigChangeHook hook);
 
+        /**
+         * Starts the ReplicaSetMonitorWatcher. You shouldn't have to call this in production code
+         * as it is called in init.cpp. However, it may be useful to pair calls to initalize
+         * and cleanup in tests. The behavior of other methods in this class is undefined if initialize()
+         * has not been called.
+         */
+        static Status MONGO_CLIENT_FUNC initialize();
+
         /**
          * Permanently stops all monitoring on replica sets and clears all cached information
          * as well. As a consequence, NEVER call this if you have other threads that have a
-         * DBClientReplicaSet instance.
+         * DBClientReplicaSet instance. After this is called, the behavior of other methods in this
+         * class is undefined until a subsequent call to initialize.
+         *
+         * The gracePeriodMillis parameter determines the maximum amount of time to wait. A value
+         * of 0 (the default) indicates no timeout. If a nonzero timeout is specified, a return value
+         * of Status::OK() indicates that shutdown completed successfully in the allotted time.
          */
-        static void MONGO_CLIENT_FUNC cleanup();
+        static Status MONGO_CLIENT_FUNC shutdown(int gracePeriodMillis = 0);
 
         /**
          * If a ReplicaSetMonitor has been refreshed more than this many times in a row without

src/mongo/dbtests/replica_set_monitor_test.cpp

@@ -1377,11 +1377,12 @@ namespace mongo_test {
             _originalConnectionHook = ConnectionString::getConnectionHook();
             ConnectionString::setConnectionHook(
                     mongo::MockConnRegistry::get()->getConnStrHook());
+            ReplicaSetMonitor::initialize();
         }
 
         void tearDown() {
             ConnectionString::setConnectionHook(_originalConnectionHook);
-            ReplicaSetMonitor::cleanup();
+            ReplicaSetMonitor::shutdown();
             _replSet.reset();
         }
 
@@ -1429,6 +1430,7 @@ namespace mongo_test {
         const string replSetName(replSet.getSetName());
         set<HostAndPort> seedList;
         seedList.insert(HostAndPort(replSet.getPrimary()));
+        ReplicaSetMonitor::initialize();
         ReplicaSetMonitor::createIfNeeded(replSetName, seedList);
 
         const MockReplicaSet::ReplConfigMap origConfig = replSet.getReplConfig();
@@ -1463,7 +1465,7 @@ namespace mongo_test {
             replMonitor->startOrContinueRefresh().refreshAll();
         }
 
-        ReplicaSetMonitor::cleanup();
+        ReplicaSetMonitor::shutdown();
         ConnectionString::setConnectionHook(originalConnHook);
     }
 
@@ -1498,12 +1500,13 @@ namespace mongo_test {
             }
 
             _replSet->setConfig(config);
+            ReplicaSetMonitor::initialize();
 
         }
 
         void tearDown() {
             ConnectionString::setConnectionHook(_originalConnectionHook);
-            ReplicaSetMonitor::cleanup();
+            ReplicaSetMonitor::shutdown();
             _replSet.reset();
         }
 

src/mongo/util/background.cpp

@@ -27,7 +27,6 @@
 #include <boost/thread/thread.hpp>
 
 #include "mongo/stdx/functional.h"
-#include "mongo/util/concurrency/mutex.h"
 #include "mongo/util/concurrency/thread_name.h"
 #include "mongo/util/debug_util.h"
 #include "mongo/util/log.h"
@@ -130,6 +129,20 @@ namespace mongo {
         return Status::OK();
     }
 
+    namespace {
+        inline boost::xtime incxtimemillis( long long s ) {
+            boost::xtime xt;
+            boost::xtime_get(&xt, MONGO_BOOST_TIME_UTC);
+            xt.sec += (int)( s / 1000 );
+            xt.nsec += (int)(( s % 1000 ) * 1000000);
+            if ( xt.nsec >= 1000000000 ) {
+                xt.nsec -= 1000000000;
+                xt.sec++;
+            }
+            return xt;
+        }
+    }  // namespace
+
     bool BackgroundJob::wait( unsigned msTimeOut ) {
         verify( !_selfDelete ); // you cannot call wait on a self-deleting job
         boost::unique_lock<boost::mutex> l( _status->mutex );