SERVER-28841 Fix ScopedCollectionMetadata destructor races

6 files changed, 61 insertions, 60 deletions

diff --git a/src/mongo/db/s/collection_range_deleter.cpp b/src/mongo/db/s/collection_range_deleter.cpp
index 0169757b969..803541d16f9 100644
--- a/src/mongo/db/s/collection_range_deleter.cpp
+++ b/src/mongo/db/s/collection_range_deleter.cpp
@@ -98,7 +98,7 @@ bool CollectionRangeDeleter::cleanupNextRange(OperationContext* txn) {
         }
 
         CollectionShardingState* shardingState = CollectionShardingState::get(txn, _nss);
-        MetadataManager& metadataManager = shardingState->_metadataManager;
+        MetadataManager& metadataManager = *shardingState->_metadataManager;
 
         if (!_rangeInProgress && !metadataManager.hasRangesToClean()) {
             // Nothing left to do
diff --git a/src/mongo/db/s/collection_sharding_state.cpp b/src/mongo/db/s/collection_sharding_state.cpp
index ce015fa245c..a5d1a822308 100644
--- a/src/mongo/db/s/collection_sharding_state.cpp
+++ b/src/mongo/db/s/collection_sharding_state.cpp
@@ -129,7 +129,7 @@ private:
 }  // unnamed namespace
 
 CollectionShardingState::CollectionShardingState(ServiceContext* sc, NamespaceString nss)
-    : _nss(std::move(nss)), _metadataManager{sc, _nss} {}
+    : _nss(std::move(nss)), _metadataManager(std::make_shared<MetadataManager>(sc, _nss)) {}
 
 CollectionShardingState::~CollectionShardingState() {
     invariant(!_sourceMgr);
@@ -150,26 +150,26 @@ CollectionShardingState* CollectionShardingState::get(OperationContext* txn,
 }
 
 ScopedCollectionMetadata CollectionShardingState::getMetadata() {
-    return _metadataManager.getActiveMetadata();
+    return _metadataManager->getActiveMetadata(_metadataManager);
 }
 
 void CollectionShardingState::refreshMetadata(OperationContext* txn,
                                               std::unique_ptr<CollectionMetadata> newMetadata) {
     invariant(txn->lockState()->isCollectionLockedForMode(_nss.ns(), MODE_X));
 
-    _metadataManager.refreshActiveMetadata(std::move(newMetadata));
+    _metadataManager->refreshActiveMetadata(std::move(newMetadata));
 }
 
 void CollectionShardingState::markNotShardedAtStepdown() {
-    _metadataManager.refreshActiveMetadata(nullptr);
+    _metadataManager->refreshActiveMetadata(nullptr);
 }
 
 void CollectionShardingState::beginReceive(const ChunkRange& range) {
-    _metadataManager.beginReceive(range);
+    _metadataManager->beginReceive(range);
 }
 
 void CollectionShardingState::forgetReceive(const ChunkRange& range) {
-    _metadataManager.forgetReceive(range);
+    _metadataManager->forgetReceive(range);
 }
 
 MigrationSourceManager* CollectionShardingState::getMigrationSourceManager() {
diff --git a/src/mongo/db/s/collection_sharding_state.h b/src/mongo/db/s/collection_sharding_state.h
index adbf743142f..2a01bec5567 100644
--- a/src/mongo/db/s/collection_sharding_state.h
+++ b/src/mongo/db/s/collection_sharding_state.h
@@ -192,7 +192,7 @@ private:
     const NamespaceString _nss;
 
     // Contains all the metadata associated with this collection.
-    MetadataManager _metadataManager;
+    std::shared_ptr<MetadataManager> _metadataManager;
 
     // If this collection is serving as a source shard for chunk migration, this value will be
     // non-null. To write this value there needs to be X-lock on the collection in order to
diff --git a/src/mongo/db/s/metadata_manager.cpp b/src/mongo/db/s/metadata_manager.cpp
index faa062e9476..28ca20e2f6f 100644
--- a/src/mongo/db/s/metadata_manager.cpp
+++ b/src/mongo/db/s/metadata_manager.cpp
@@ -56,13 +56,14 @@ MetadataManager::~MetadataManager() {
     invariant(!_activeMetadataTracker || _activeMetadataTracker->usageCounter == 0);
 }
 
-ScopedCollectionMetadata MetadataManager::getActiveMetadata() {
+ScopedCollectionMetadata MetadataManager::getActiveMetadata(std::shared_ptr<MetadataManager> self) {
+
     stdx::lock_guard<stdx::mutex> scopedLock(_managerLock);
     if (!_activeMetadataTracker) {
         return ScopedCollectionMetadata();
     }
 
-    return ScopedCollectionMetadata(this, _activeMetadataTracker.get());
+    return ScopedCollectionMetadata(std::move(self), _activeMetadataTracker.get());
 }
 
 void MetadataManager::refreshActiveMetadata(std::unique_ptr<CollectionMetadata> remoteMetadata) {
@@ -287,8 +288,8 @@ ScopedCollectionMetadata::ScopedCollectionMetadata() = default;
 
 // called in lock
 ScopedCollectionMetadata::ScopedCollectionMetadata(
-    MetadataManager* manager, MetadataManager::CollectionMetadataTracker* tracker)
-    : _manager(manager), _tracker(tracker) {
+    std::shared_ptr<MetadataManager> manager, MetadataManager::CollectionMetadataTracker* tracker)
+    : _manager(std::move(manager)), _tracker(tracker) {
     _tracker->usageCounter++;
 }
 
diff --git a/src/mongo/db/s/metadata_manager.h b/src/mongo/db/s/metadata_manager.h
index 5c2e7f2e64a..fa61e9ef2ea 100644
--- a/src/mongo/db/s/metadata_manager.h
+++ b/src/mongo/db/s/metadata_manager.h
@@ -59,7 +59,7 @@ public:
      * contains the currently active metadata.  When the usageCounter goes to zero, the RAII
      * object going out of scope will call _removeMetadata.
      */
-    ScopedCollectionMetadata getActiveMetadata();
+    ScopedCollectionMetadata getActiveMetadata(std::shared_ptr<MetadataManager> self);
 
     /**
      * Uses the contents of the specified metadata as a way to purge any pending chunks.
@@ -204,7 +204,7 @@ private:
     const NamespaceString _nss;
 
     // ServiceContext from which to obtain instances of global support objects.
-    ServiceContext* _serviceContext;
+    ServiceContext* const _serviceContext;
 
     // Mutex to protect the state below
     stdx::mutex _managerLock;
@@ -252,12 +252,13 @@ public:
     operator bool() const;
 
 private:
-    friend ScopedCollectionMetadata MetadataManager::getActiveMetadata();
+    friend ScopedCollectionMetadata MetadataManager::getActiveMetadata(
+        std::shared_ptr<MetadataManager>);
 
     /**
      * Increments the counter in the CollectionMetadataTracker.
      */
-    ScopedCollectionMetadata(MetadataManager* manager,
+    ScopedCollectionMetadata(std::shared_ptr<MetadataManager> manager,
                              MetadataManager::CollectionMetadataTracker* tracker);
 
     /**
@@ -266,7 +267,7 @@ private:
      */
     void _decrementUsageCounter();
 
-    MetadataManager* _manager{nullptr};
+    std::shared_ptr<MetadataManager> _manager{nullptr};
     MetadataManager::CollectionMetadataTracker* _tracker{nullptr};
 };
 
diff --git a/src/mongo/db/s/metadata_manager_test.cpp b/src/mongo/db/s/metadata_manager_test.cpp
index 242ef274be1..19f4fd405ab 100644
--- a/src/mongo/db/s/metadata_manager_test.cpp
+++ b/src/mongo/db/s/metadata_manager_test.cpp
@@ -92,25 +92,27 @@ protected:
         return stdx::make_unique<CollectionMetadata>(
             metadata.getKeyPattern(), chunkVersion, chunkVersion, std::move(chunksMap));
     }
+
+    std::shared_ptr<MetadataManager> manager_ptr{std::make_shared<MetadataManager>(
+        getServiceContext(), NamespaceString("TestDb", "CollDB"))};
+    MetadataManager& manager{*this->manager_ptr};
 };
 
 TEST_F(MetadataManagerTest, SetAndGetActiveMetadata) {
-    MetadataManager manager(getServiceContext(), NamespaceString("TestDb", "CollDB"));
     std::unique_ptr<CollectionMetadata> cm = makeEmptyMetadata();
     auto cmPtr = cm.get();
 
     manager.refreshActiveMetadata(std::move(cm));
-    ScopedCollectionMetadata scopedMetadata = manager.getActiveMetadata();
+    ScopedCollectionMetadata scopedMetadata = manager.getActiveMetadata(manager_ptr);
 
     ASSERT_EQ(cmPtr, scopedMetadata.getMetadata());
 };
 
 
 TEST_F(MetadataManagerTest, ResetActiveMetadata) {
-    MetadataManager manager(getServiceContext(), NamespaceString("TestDb", "CollDB"));
     manager.refreshActiveMetadata(makeEmptyMetadata());
 
-    ScopedCollectionMetadata scopedMetadata1 = manager.getActiveMetadata();
+    ScopedCollectionMetadata scopedMetadata1 = manager.getActiveMetadata(manager_ptr);
 
     ChunkVersion newVersion = scopedMetadata1->getCollVersion();
     newVersion.incMajor();
@@ -119,13 +121,12 @@ TEST_F(MetadataManagerTest, ResetActiveMetadata) {
     auto cm2Ptr = cm2.get();
 
     manager.refreshActiveMetadata(std::move(cm2));
-    ScopedCollectionMetadata scopedMetadata2 = manager.getActiveMetadata();
+    ScopedCollectionMetadata scopedMetadata2 = manager.getActiveMetadata(manager_ptr);
 
     ASSERT_EQ(cm2Ptr, scopedMetadata2.getMetadata());
 };
 
 TEST_F(MetadataManagerTest, AddAndRemoveRangesToClean) {
-    MetadataManager manager(getServiceContext(), NamespaceString("TestDb", "CollDB"));
     ChunkRange cr1 = ChunkRange(BSON("key" << 0), BSON("key" << 10));
     ChunkRange cr2 = ChunkRange(BSON("key" << 10), BSON("key" << 20));
 
@@ -148,7 +149,6 @@ TEST_F(MetadataManagerTest, AddAndRemoveRangesToClean) {
 // Tests that a removal in the middle of an existing ChunkRange results in
 // two correct chunk ranges.
 TEST_F(MetadataManagerTest, RemoveRangeInMiddleOfRange) {
-    MetadataManager manager(getServiceContext(), NamespaceString("TestDb", "CollDB"));
     ChunkRange cr1 = ChunkRange(BSON("key" << 0), BSON("key" << 10));
 
     manager.addRangeToClean(cr1);
@@ -172,7 +172,6 @@ TEST_F(MetadataManagerTest, RemoveRangeInMiddleOfRange) {
 
 // Tests removals that overlap with just one ChunkRange.
 TEST_F(MetadataManagerTest, RemoveRangeWithSingleRangeOverlap) {
-    MetadataManager manager(getServiceContext(), NamespaceString("TestDb", "CollDB"));
     ChunkRange cr1 = ChunkRange(BSON("key" << 0), BSON("key" << 10));
 
     manager.addRangeToClean(cr1);
@@ -206,7 +205,6 @@ TEST_F(MetadataManagerTest, RemoveRangeWithSingleRangeOverlap) {
 
 // Tests removals that overlap with more than one ChunkRange.
 TEST_F(MetadataManagerTest, RemoveRangeWithMultipleRangeOverlaps) {
-    MetadataManager manager(getServiceContext(), NamespaceString("TestDb", "CollDB"));
     ChunkRange cr1 = ChunkRange(BSON("key" << 0), BSON("key" << 10));
     ChunkRange cr2 = ChunkRange(BSON("key" << 10), BSON("key" << 20));
     ChunkRange cr3 = ChunkRange(BSON("key" << 20), BSON("key" << 30));
@@ -233,7 +231,6 @@ TEST_F(MetadataManagerTest, RemoveRangeWithMultipleRangeOverlaps) {
 }
 
 TEST_F(MetadataManagerTest, AddAndRemoveRangeNotificationsBlockAndYield) {
-    MetadataManager manager(getServiceContext(), NamespaceString("TestDb", "CollDB"));
     manager.refreshActiveMetadata(makeEmptyMetadata());
 
     ChunkRange cr1(BSON("key" << 0), BSON("key" << 10));
@@ -244,7 +241,6 @@ TEST_F(MetadataManagerTest, AddAndRemoveRangeNotificationsBlockAndYield) {
 }
 
 TEST_F(MetadataManagerTest, RemoveRangeToCleanCorrectlySetsBadStatus) {
-    MetadataManager manager(getServiceContext(), NamespaceString("TestDb", "CollDB"));
     manager.refreshActiveMetadata(makeEmptyMetadata());
 
     ChunkRange cr1(BSON("key" << 0), BSON("key" << 10));
@@ -255,7 +251,6 @@ TEST_F(MetadataManagerTest, RemoveRangeToCleanCorrectlySetsBadStatus) {
 }
 
 TEST_F(MetadataManagerTest, RemovingSubrangeStillSetsNotificationStatus) {
-    MetadataManager manager(getServiceContext(), NamespaceString("TestDb", "CollDB"));
     manager.refreshActiveMetadata(makeEmptyMetadata());
 
     ChunkRange cr1(BSON("key" << 0), BSON("key" << 10));
@@ -275,7 +270,6 @@ TEST_F(MetadataManagerTest, RemovingSubrangeStillSetsNotificationStatus) {
 }
 
 TEST_F(MetadataManagerTest, NotificationBlocksUntilDeletion) {
-    MetadataManager manager(getServiceContext(), NamespaceString("TestDb", "CollDB"));
     manager.refreshActiveMetadata(makeEmptyMetadata());
 
     ChunkRange cr1(BSON("key" << 0), BSON("key" << 10));
@@ -292,25 +286,26 @@ TEST_F(MetadataManagerTest, NotificationBlocksUntilDeletion) {
 
 
 TEST_F(MetadataManagerTest, RefreshAfterSuccessfulMigrationSinglePending) {
-    MetadataManager manager(getServiceContext(), NamespaceString("TestDb", "CollDB"));
     manager.refreshActiveMetadata(makeEmptyMetadata());
 
     const ChunkRange cr1(BSON("key" << 0), BSON("key" << 10));
     manager.beginReceive(cr1);
     ASSERT_EQ(manager.getCopyOfReceivingChunks().size(), 1UL);
-    ASSERT_EQ(manager.getActiveMetadata()->getChunks().size(), 0UL);
+    ASSERT_EQ(manager.getActiveMetadata(manager_ptr)->getChunks().size(), 0UL);
 
-    ChunkVersion version = manager.getActiveMetadata()->getCollVersion();
+    ChunkVersion version = manager.getActiveMetadata(manager_ptr)->getCollVersion();
     version.incMajor();
 
-    manager.refreshActiveMetadata(cloneMetadataPlusChunk(
-        *manager.getActiveMetadata().getMetadata(), cr1.getMin(), cr1.getMax(), version));
+    manager.refreshActiveMetadata(
+        cloneMetadataPlusChunk(*manager.getActiveMetadata(manager_ptr).getMetadata(),
+                               cr1.getMin(),
+                               cr1.getMax(),
+                               version));
     ASSERT_EQ(manager.getCopyOfReceivingChunks().size(), 0UL);
-    ASSERT_EQ(manager.getActiveMetadata()->getChunks().size(), 1UL);
+    ASSERT_EQ(manager.getActiveMetadata(manager_ptr)->getChunks().size(), 1UL);
 }
 
 TEST_F(MetadataManagerTest, RefreshAfterSuccessfulMigrationMultiplePending) {
-    MetadataManager manager(getServiceContext(), NamespaceString("TestDb", "CollDB"));
     manager.refreshActiveMetadata(makeEmptyMetadata());
 
     const ChunkRange cr1(BSON("key" << 0), BSON("key" << 10));
@@ -320,31 +315,36 @@ TEST_F(MetadataManagerTest, RefreshAfterSuccessfulMigrationMultiplePending) {
     manager.beginReceive(cr2);
 
     ASSERT_EQ(manager.getCopyOfReceivingChunks().size(), 2UL);
-    ASSERT_EQ(manager.getActiveMetadata()->getChunks().size(), 0UL);
+    ASSERT_EQ(manager.getActiveMetadata(manager_ptr)->getChunks().size(), 0UL);
 
     {
-        ChunkVersion version = manager.getActiveMetadata()->getCollVersion();
+        ChunkVersion version = manager.getActiveMetadata(manager_ptr)->getCollVersion();
         version.incMajor();
 
-        manager.refreshActiveMetadata(cloneMetadataPlusChunk(
-            *manager.getActiveMetadata().getMetadata(), cr1.getMin(), cr1.getMax(), version));
+        manager.refreshActiveMetadata(
+            cloneMetadataPlusChunk(*manager.getActiveMetadata(manager_ptr).getMetadata(),
+                                   cr1.getMin(),
+                                   cr1.getMax(),
+                                   version));
         ASSERT_EQ(manager.getCopyOfReceivingChunks().size(), 1UL);
-        ASSERT_EQ(manager.getActiveMetadata()->getChunks().size(), 1UL);
+        ASSERT_EQ(manager.getActiveMetadata(manager_ptr)->getChunks().size(), 1UL);
     }
 
     {
-        ChunkVersion version = manager.getActiveMetadata()->getCollVersion();
+        ChunkVersion version = manager.getActiveMetadata(manager_ptr)->getCollVersion();
         version.incMajor();
 
-        manager.refreshActiveMetadata(cloneMetadataPlusChunk(
-            *manager.getActiveMetadata().getMetadata(), cr2.getMin(), cr2.getMax(), version));
+        manager.refreshActiveMetadata(
+            cloneMetadataPlusChunk(*manager.getActiveMetadata(manager_ptr).getMetadata(),
+                                   cr2.getMin(),
+                                   cr2.getMax(),
+                                   version));
         ASSERT_EQ(manager.getCopyOfReceivingChunks().size(), 0UL);
-        ASSERT_EQ(manager.getActiveMetadata()->getChunks().size(), 2UL);
+        ASSERT_EQ(manager.getActiveMetadata(manager_ptr)->getChunks().size(), 2UL);
     }
 }
 
 TEST_F(MetadataManagerTest, RefreshAfterNotYetCompletedMigrationMultiplePending) {
-    MetadataManager manager(getServiceContext(), NamespaceString("TestDb", "CollDB"));
     manager.refreshActiveMetadata(makeEmptyMetadata());
 
     const ChunkRange cr1(BSON("key" << 0), BSON("key" << 10));
@@ -354,19 +354,21 @@ TEST_F(MetadataManagerTest, RefreshAfterNotYetCompletedMigrationMultiplePending)
     manager.beginReceive(cr2);
 
     ASSERT_EQ(manager.getCopyOfReceivingChunks().size(), 2UL);
-    ASSERT_EQ(manager.getActiveMetadata()->getChunks().size(), 0UL);
+    ASSERT_EQ(manager.getActiveMetadata(manager_ptr)->getChunks().size(), 0UL);
 
-    ChunkVersion version = manager.getActiveMetadata()->getCollVersion();
+    ChunkVersion version = manager.getActiveMetadata(manager_ptr)->getCollVersion();
     version.incMajor();
 
-    manager.refreshActiveMetadata(cloneMetadataPlusChunk(
-        *manager.getActiveMetadata().getMetadata(), BSON("key" << 50), BSON("key" << 60), version));
+    manager.refreshActiveMetadata(
+        cloneMetadataPlusChunk(*manager.getActiveMetadata(manager_ptr).getMetadata(),
+                               BSON("key" << 50),
+                               BSON("key" << 60),
+                               version));
     ASSERT_EQ(manager.getCopyOfReceivingChunks().size(), 2UL);
-    ASSERT_EQ(manager.getActiveMetadata()->getChunks().size(), 1UL);
+    ASSERT_EQ(manager.getActiveMetadata(manager_ptr)->getChunks().size(), 1UL);
 }
 
 TEST_F(MetadataManagerTest, BeginReceiveWithOverlappingRange) {
-    MetadataManager manager(getServiceContext(), NamespaceString("TestDb", "CollDB"));
     manager.refreshActiveMetadata(makeEmptyMetadata());
 
     const ChunkRange cr1(BSON("key" << 0), BSON("key" << 10));
@@ -381,7 +383,7 @@ TEST_F(MetadataManagerTest, BeginReceiveWithOverlappingRange) {
     const auto copyOfPending = manager.getCopyOfReceivingChunks();
 
     ASSERT_EQ(copyOfPending.size(), 1UL);
-    ASSERT_EQ(manager.getActiveMetadata()->getChunks().size(), 0UL);
+    ASSERT_EQ(manager.getActiveMetadata(manager_ptr)->getChunks().size(), 0UL);
 
     const auto it = copyOfPending.find(BSON("key" << 5));
     ASSERT(it != copyOfPending.end());
@@ -389,11 +391,10 @@ TEST_F(MetadataManagerTest, BeginReceiveWithOverlappingRange) {
 }
 
 TEST_F(MetadataManagerTest, RefreshMetadataAfterDropAndRecreate) {
-    MetadataManager manager(getServiceContext(), NamespaceString("TestDb", "CollDB"));
     manager.refreshActiveMetadata(makeEmptyMetadata());
 
     {
-        auto metadata = manager.getActiveMetadata();
+        auto metadata = manager.getActiveMetadata(manager_ptr);
         ChunkVersion newVersion = metadata->getCollVersion();
         newVersion.incMajor();
 
@@ -408,9 +409,9 @@ TEST_F(MetadataManagerTest, RefreshMetadataAfterDropAndRecreate) {
 
     manager.refreshActiveMetadata(cloneMetadataPlusChunk(
         *recreateMetadata, BSON("key" << 20), BSON("key" << 30), newVersion));
-    ASSERT_EQ(manager.getActiveMetadata()->getChunks().size(), 1UL);
+    ASSERT_EQ(manager.getActiveMetadata(manager_ptr)->getChunks().size(), 1UL);
 
-    const auto chunkEntry = manager.getActiveMetadata()->getChunks().begin();
+    const auto chunkEntry = manager.getActiveMetadata(manager_ptr)->getChunks().begin();
     ASSERT_BSONOBJ_EQ(BSON("key" << 20), chunkEntry->first);
     ASSERT_BSONOBJ_EQ(BSON("key" << 30), chunkEntry->second.getMaxKey());
     ASSERT_EQ(newVersion, chunkEntry->second.getVersion());
@@ -418,7 +419,6 @@ TEST_F(MetadataManagerTest, RefreshMetadataAfterDropAndRecreate) {
 
 // Tests membership functions for _rangesToClean
 TEST_F(MetadataManagerTest, RangesToCleanMembership) {
-    MetadataManager manager(getServiceContext(), NamespaceString("TestDb", "CollDB"));
     manager.refreshActiveMetadata(makeEmptyMetadata());
 
     ASSERT(!manager.hasRangesToClean());
@@ -432,7 +432,6 @@ TEST_F(MetadataManagerTest, RangesToCleanMembership) {
 
 // Tests that getNextRangeToClean successfully pulls a stored ChunkRange
 TEST_F(MetadataManagerTest, GetNextRangeToClean) {
-    MetadataManager manager(getServiceContext(), NamespaceString("TestDb", "CollDB"));
     manager.refreshActiveMetadata(makeEmptyMetadata());
 
     ChunkRange cr1 = ChunkRange(BSON("key" << 0), BSON("key" << 10));