SERVER-38288 Delete CursorManager::invalidateAll().

7 files changed, 60 insertions, 190 deletions

diff --git a/src/mongo/db/catalog/index_catalog.h b/src/mongo/db/catalog/index_catalog.h
index 55d5c79446a..d79e429cf6c 100644
--- a/src/mongo/db/catalog/index_catalog.h
+++ b/src/mongo/db/catalog/index_catalog.h
@@ -294,9 +294,7 @@ public:
      *
      * Use this method to notify the IndexCatalog that the spec for this index has changed.
      *
-     * It is invalid to dereference 'oldDesc' after calling this method.  This method broadcasts
-     * an invalidateAll() on the cursor manager to notify other users of the IndexCatalog that
-     * this descriptor is now invalid.
+     * It is invalid to dereference 'oldDesc' after calling this method.
      */
     virtual const IndexDescriptor* refreshEntry(OperationContext* const opCtx,
                                                 const IndexDescriptor* const oldDesc) = 0;
diff --git a/src/mongo/db/clientcursor.cpp b/src/mongo/db/clientcursor.cpp
index c7412752356..30c52ef2adc 100644
--- a/src/mongo/db/clientcursor.cpp
+++ b/src/mongo/db/clientcursor.cpp
@@ -157,8 +157,10 @@ GenericCursor ClientCursor::toGenericCursor() const {
 // Pin methods
 //
 
-ClientCursorPin::ClientCursorPin(OperationContext* opCtx, ClientCursor* cursor)
-    : _opCtx(opCtx), _cursor(cursor) {
+ClientCursorPin::ClientCursorPin(OperationContext* opCtx,
+                                 ClientCursor* cursor,
+                                 CursorManager* cursorManager)
+    : _opCtx(opCtx), _cursor(cursor), _cursorManager(cursorManager) {
     invariant(_cursor);
     invariant(_cursor->_operationUsingCursor);
     invariant(!_cursor->_disposed);
@@ -171,7 +173,7 @@ ClientCursorPin::ClientCursorPin(OperationContext* opCtx, ClientCursor* cursor)
 }
 
 ClientCursorPin::ClientCursorPin(ClientCursorPin&& other)
-    : _opCtx(other._opCtx), _cursor(other._cursor) {
+    : _opCtx(other._opCtx), _cursor(other._cursor), _cursorManager(other._cursorManager) {
     // The pinned cursor is being transferred to us from another pin. The 'other' pin must have a
     // pinned cursor.
     invariant(other._cursor);
@@ -180,6 +182,7 @@ ClientCursorPin::ClientCursorPin(ClientCursorPin&& other)
     // Be sure to set the 'other' pin's cursor to null in order to transfer ownership to ourself.
     other._cursor = nullptr;
     other._opCtx = nullptr;
+    other._cursorManager = nullptr;
 }
 
 ClientCursorPin& ClientCursorPin::operator=(ClientCursorPin&& other) {
@@ -202,6 +205,9 @@ ClientCursorPin& ClientCursorPin::operator=(ClientCursorPin&& other) {
     _opCtx = other._opCtx;
     other._opCtx = nullptr;
 
+    _cursorManager = other._cursorManager;
+    other._cursorManager = nullptr;
+
     return *this;
 }
 
@@ -214,11 +220,11 @@ void ClientCursorPin::release() {
         return;
 
     invariant(_cursor->_operationUsingCursor);
+    invariant(_cursorManager);
 
     // Unpin the cursor. This must be done by calling into the cursor manager, since the cursor
     // manager must acquire the appropriate mutex in order to safely perform the unpin operation.
-    CursorManager::getGlobalCursorManager()->unpin(
-        _opCtx, std::unique_ptr<ClientCursor, ClientCursor::Deleter>(_cursor));
+    _cursorManager->unpin(_opCtx, std::unique_ptr<ClientCursor, ClientCursor::Deleter>(_cursor));
     cursorStatsOpenPinned.decrement();
 
     _cursor = nullptr;
@@ -227,6 +233,7 @@ void ClientCursorPin::release() {
 void ClientCursorPin::deleteUnderlying() {
     invariant(_cursor);
     invariant(_cursor->_operationUsingCursor);
+    invariant(_cursorManager);
     // Note the following subtleties of this method's implementation:
     // - We must unpin the cursor (by clearing the '_operationUsingCursor' field) before
     //   destruction, since it is an error to delete a pinned cursor.
@@ -236,7 +243,7 @@ void ClientCursorPin::deleteUnderlying() {
     //   access '_cursor', meaning that it is safe for us to write to '_operationUsingCursor'
     //   without holding the CursorManager mutex.
 
-    CursorManager::getGlobalCursorManager()->deregisterCursor(_cursor);
+    _cursorManager->deregisterCursor(_cursor);
 
     // Make sure the cursor is disposed and unpinned before being destroyed.
     _cursor->dispose(_opCtx);
diff --git a/src/mongo/db/clientcursor.h b/src/mongo/db/clientcursor.h
index 45eb548e495..8bbc5491ee2 100644
--- a/src/mongo/db/clientcursor.h
+++ b/src/mongo/db/clientcursor.h
@@ -44,6 +44,7 @@
 namespace mongo {
 
 class Collection;
+class CursorManager;
 class RecoveryUnit;
 
 /**
@@ -481,10 +482,11 @@ public:
 private:
     friend class CursorManager;
 
-    ClientCursorPin(OperationContext* opCtx, ClientCursor* cursor);
+    ClientCursorPin(OperationContext* opCtx, ClientCursor* cursor, CursorManager* cursorManager);
 
     OperationContext* _opCtx = nullptr;
     ClientCursor* _cursor = nullptr;
+    CursorManager* _cursorManager = nullptr;
 };
 
 void startClientCursorMonitor();
diff --git a/src/mongo/db/cursor_manager.cpp b/src/mongo/db/cursor_manager.cpp
index 853830e413c..6659347026f 100644
--- a/src/mongo/db/cursor_manager.cpp
+++ b/src/mongo/db/cursor_manager.cpp
@@ -308,54 +308,15 @@ CursorManager::CursorManager()
       _cursorMap(stdx::make_unique<Partitioned<stdx::unordered_map<CursorId, ClientCursor*>>>()) {}
 
 CursorManager::~CursorManager() {
-    // All cursors should have been deleted already.
-    invariant(_cursorMap->empty());
-
-    if (!isGlobalManager()) {
-        globalCursorIdCache->deregisterCursorManager(_collectionCacheRuntimeId, _nss);
-    }
-}
-
-void CursorManager::invalidateAll(OperationContext* opCtx,
-                                  bool collectionGoingAway,
-                                  const std::string& reason) {
-    dassert(isGlobalManager() || opCtx->lockState()->isCollectionLockedForMode(_nss.ns(), MODE_X));
-    fassert(28819, !BackgroundOperation::inProgForNs(_nss));
-
-    // Mark all cursors as killed, but keep around those we can in order to provide a useful error
-    // message to the user when they attempt to use it next time.
-    std::vector<std::unique_ptr<ClientCursor, ClientCursor::Deleter>> toDisposeWithoutMutex;
-    {
-        auto allCurrentPartitions = _cursorMap->lockAllPartitions();
-        for (auto&& partition : allCurrentPartitions) {
-            for (auto it = partition.begin(); it != partition.end();) {
-                auto* cursor = it->second;
-                cursor->markAsKilled({ErrorCodes::QueryPlanKilled, reason});
-
-                // If there's an operation actively using the cursor, then that operation is now
-                // responsible for cleaning it up.  Otherwise we can immediately dispose of it.
-                if (cursor->_operationUsingCursor) {
-                    partition.erase(it++);
-                    continue;
-                }
-
-                if (!collectionGoingAway) {
-                    // We keep around unpinned cursors so that future attempts to use the cursor
-                    // will result in a useful error message.
-                    ++it;
-                } else {
-                    toDisposeWithoutMutex.emplace_back(cursor);
-                    partition.erase(it++);
-                }
-            }
+    auto allPartitions = _cursorMap->lockAllPartitions();
+    for (auto&& partition : allPartitions) {
+        for (auto&& cursor : partition) {
+            // Callers must ensure that no cursors are in use.
+            invariant(!cursor.second->_operationUsingCursor);
+            cursor.second->dispose(nullptr);
+            delete cursor.second;
         }
     }
-
-    // Dispose of the cursors we can now delete. This might involve lock acquisitions for safe
-    // cleanup, so avoid doing it while holding mutexes.
-    for (auto&& cursor : toDisposeWithoutMutex) {
-        cursor->dispose(opCtx);
-    }
 }
 
 bool CursorManager::cursorShouldTimeout_inlock(const ClientCursor* cursor, Date_t now) {
@@ -431,7 +392,7 @@ StatusWith<ClientCursorPin> CursorManager::pinCursor(OperationContext* opCtx,
         }
     }
 
-    return ClientCursorPin(opCtx, cursor);
+    return ClientCursorPin(opCtx, cursor, this);
 }
 
 void CursorManager::unpin(OperationContext* opCtx,
@@ -576,7 +537,7 @@ ClientCursorPin CursorManager::registerCursor(OperationContext* opCtx,
     auto partition = _cursorMap->lockOnePartition(cursorId);
     ClientCursor* unownedCursor = clientCursor.release();
     partition->emplace(cursorId, unownedCursor);
-    return ClientCursorPin(opCtx, unownedCursor);
+    return ClientCursorPin(opCtx, unownedCursor, this);
 }
 
 void CursorManager::deregisterCursor(ClientCursor* cursor) {
diff --git a/src/mongo/db/cursor_manager.h b/src/mongo/db/cursor_manager.h
index d0e89e1e6de..7a4dadba465 100644
--- a/src/mongo/db/cursor_manager.h
+++ b/src/mongo/db/cursor_manager.h
@@ -137,16 +137,11 @@ public:
 
     CursorManager();
 
-    ~CursorManager();
-
     /**
-     * This method is deprecated. Do not add new call sites.
-     *
-     * TODO SERVER-38288: Delete this method.
+     * Destroys the cursor manager, deleting all managed cursors. Illegal to call if any managed
+     * cursor is pinned.
      */
-    void invalidateAll(OperationContext* opCtx,
-                       bool collectionGoingAway,
-                       const std::string& reason);
+    ~CursorManager();
 
     /**
      * Destroys cursors that have been inactive for too long.
diff --git a/src/mongo/db/s/database_sharding_state.cpp b/src/mongo/db/s/database_sharding_state.cpp
index 809a0ffd9ad..3c8d81010a5 100644
--- a/src/mongo/db/s/database_sharding_state.cpp
+++ b/src/mongo/db/s/database_sharding_state.cpp
@@ -51,9 +51,6 @@ DatabaseShardingState::DatabaseShardingState() = default;
 void DatabaseShardingState::enterCriticalSectionCatchUpPhase(OperationContext* opCtx) {
     invariant(opCtx->lockState()->isDbLockedForMode(get.owner(this)->name(), MODE_X));
     _critSec.enterCriticalSectionCatchUpPhase();
-    // TODO (SERVER-33313): call CursorManager::invalidateAll() on all collections in this database
-    // with 'fromMovePrimary=true' and a predicate to only invalidate the cursor if the opCtx on its
-    // PlanExecutor has a client dbVersion.
 }
 
 void DatabaseShardingState::enterCriticalSectionCommitPhase(OperationContext* opCtx) {
diff --git a/src/mongo/dbtests/cursor_manager_test.cpp b/src/mongo/dbtests/cursor_manager_test.cpp
index dd734401e9d..8e0e5edac41 100644
--- a/src/mongo/dbtests/cursor_manager_test.cpp
+++ b/src/mongo/dbtests/cursor_manager_test.cpp
@@ -91,10 +91,6 @@ public:
             static_cast<ClockSourceMock*>(_opCtx->getServiceContext()->getPreciseClockSource());
     }
 
-    virtual ~CursorManagerTest() {
-        useCursorManager()->invalidateAll(_opCtx.get(), true, "end of test");
-    }
-
     std::unique_ptr<PlanExecutor, PlanExecutor::Deleter> makeFakePlanExecutor() {
         return makeFakePlanExecutor(_opCtx.get());
     }
@@ -128,7 +124,7 @@ public:
     }
 
     CursorManager* useCursorManager() {
-        return CursorManager::getGlobalCursorManager();
+        return &_cursorManager;
     }
 
 protected:
@@ -137,6 +133,7 @@ protected:
 
 private:
     ClockSourceMock* _clock;
+    CursorManager _cursorManager;
 };
 
 class CursorManagerTestCustomOpCtx : public CursorManagerTest {
@@ -164,101 +161,6 @@ TEST_F(CursorManagerTest, GlobalCursorManagerShouldReportOwnershipOfCursorsItCre
 }
 
 /**
- * Tests that invalidating a cursor without dropping the collection while the cursor is not in use
- * will keep the cursor registered. After being invalidated, pinning the cursor should take
- * ownership of the cursor and calling getNext() on its PlanExecutor should return an error
- * including the error message.
- */
-TEST_F(CursorManagerTest, InvalidateCursor) {
-    CursorManager* cursorManager = useCursorManager();
-    auto cursorPin = cursorManager->registerCursor(
-        _opCtx.get(),
-        {makeFakePlanExecutor(),
-         kTestNss,
-         {},
-         repl::ReadConcernArgs(repl::ReadConcernLevel::kLocalReadConcern),
-         BSONObj(),
-         ClientCursorParams::LockPolicy::kLocksInternally});
-
-    auto cursorId = cursorPin.getCursor()->cursorid();
-    cursorPin.release();
-
-    ASSERT_EQUALS(1U, cursorManager->numCursors());
-    auto invalidateReason = "Invalidate Test";
-    const bool collectionGoingAway = false;
-    cursorManager->invalidateAll(_opCtx.get(), collectionGoingAway, invalidateReason);
-    // Since the collection is not going away, the cursor should remain open, but be killed.
-    ASSERT_EQUALS(1U, cursorManager->numCursors());
-
-    // Pinning a killed cursor should result in an error and clean up the cursor.
-    ASSERT_EQ(ErrorCodes::QueryPlanKilled,
-              cursorManager->pinCursor(_opCtx.get(), cursorId).getStatus());
-    ASSERT_EQUALS(0U, cursorManager->numCursors());
-}
-
-/**
- * Tests that invalidating a cursor and dropping the collection while the cursor is not in use will
- * not keep the cursor registered.
- */
-TEST_F(CursorManagerTest, InvalidateCursorWithDrop) {
-    CursorManager* cursorManager = useCursorManager();
-
-    auto cursorPin = cursorManager->registerCursor(
-        _opCtx.get(),
-        {makeFakePlanExecutor(),
-         kTestNss,
-         {},
-         repl::ReadConcernArgs(repl::ReadConcernLevel::kLocalReadConcern),
-         BSONObj(),
-         ClientCursorParams::LockPolicy::kLocksInternally});
-
-    auto cursorId = cursorPin.getCursor()->cursorid();
-    cursorPin.release();
-
-    ASSERT_EQUALS(1U, cursorManager->numCursors());
-    auto invalidateReason = "Invalidate Test";
-    const bool collectionGoingAway = true;
-    cursorManager->invalidateAll(_opCtx.get(), collectionGoingAway, invalidateReason);
-    // Since the collection is going away, the cursor should not remain open.
-    ASSERT_EQ(ErrorCodes::CursorNotFound,
-              cursorManager->pinCursor(_opCtx.get(), cursorId).getStatus());
-    ASSERT_EQUALS(0U, cursorManager->numCursors());
-}
-
-/**
- * Tests that invalidating a cursor while it is in use will deregister it from the cursor manager,
- * transferring ownership to the pinned cursor.
- */
-TEST_F(CursorManagerTest, InvalidatePinnedCursor) {
-    CursorManager* cursorManager = useCursorManager();
-
-    auto cursorPin = cursorManager->registerCursor(
-        _opCtx.get(),
-        {makeFakePlanExecutor(),
-         kTestNss,
-         {},
-         repl::ReadConcernArgs(repl::ReadConcernLevel::kLocalReadConcern),
-         BSONObj(),
-         ClientCursorParams::LockPolicy::kLocksInternally});
-    auto cursorFreer = makeGuard([&] { cursorPin.deleteUnderlying(); });
-
-    // If the cursor is pinned, it sticks around, even after invalidation.
-    ASSERT_EQUALS(1U, cursorManager->numCursors());
-    const std::string invalidateReason("InvalidatePinned Test");
-    cursorManager->invalidateAll(_opCtx.get(), false, invalidateReason);
-    ASSERT_EQUALS(0U, cursorManager->numCursors());
-
-    // The invalidation should have killed the plan executor.
-    BSONObj objOut;
-    ASSERT_EQUALS(PlanExecutor::DEAD, cursorPin.getCursor()->getExecutor()->getNext(&objOut, NULL));
-    ASSERT(WorkingSetCommon::isValidStatusMemberObject(objOut));
-    const Status status = WorkingSetCommon::getMemberObjectStatus(objOut);
-    ASSERT(status.reason().find(invalidateReason) != std::string::npos);
-
-    ASSERT_EQUALS(0U, cursorManager->numCursors());
-}
-
-/**
  * Test that an attempt to kill a pinned cursor succeeds.
  */
 TEST_F(CursorManagerTest, ShouldBeAbleToKillPinnedCursor) {
@@ -374,13 +276,14 @@ TEST_F(CursorManagerTest, InactivePinnedCursorShouldNotTimeout) {
 }
 
 /**
- * Test that client cursors which have been marked as killed time out and get deleted.
+ * A cursor can be left in the CursorManager in a killed state when a pinned cursor is interrupted
+ * with an unusual error code (a code other than ErrorCodes::Interrupted or
+ * ErrorCodes::CursorKilled). Verify that such cursors get deregistered and deleted on an attempt to
+ * pin.
  */
-TEST_F(CursorManagerTest, InactiveKilledCursorsShouldTimeout) {
+TEST_F(CursorManagerTest, MarkedAsKilledCursorsShouldBeDeletedOnCursorPin) {
     CursorManager* cursorManager = useCursorManager();
-    auto clock = useClock();
 
-    // Make a cursor from the plan executor, and immediately kill it.
     auto cursorPin = cursorManager->registerCursor(
         _opCtx.get(),
         {makeFakePlanExecutor(),
@@ -389,26 +292,30 @@ TEST_F(CursorManagerTest, InactiveKilledCursorsShouldTimeout) {
          repl::ReadConcernArgs(repl::ReadConcernLevel::kLocalReadConcern),
          BSONObj(),
          ClientCursorParams::LockPolicy::kLocksInternally});
+    auto cursorId = cursorPin->cursorid();
+
+    // A cursor will stay alive, but be marked as killed, if it is interrupted with a code other
+    // than ErrorCodes::Interrupted or ErrorCodes::CursorKilled and then unpinned.
+    _opCtx->markKilled(ErrorCodes::InternalError);
     cursorPin.release();
-    const bool collectionGoingAway = false;
-    cursorManager->invalidateAll(
-        _opCtx.get(), collectionGoingAway, "KilledCursorsShouldTimeoutTest");
 
-    // Advance the clock to simulate time passing.
-    clock->advance(getDefaultCursorTimeoutMillis());
+    // The cursor should still be present in the manager.
+    ASSERT_EQ(1UL, cursorManager->numCursors());
 
-    ASSERT_EQ(1UL, cursorManager->timeoutCursors(_opCtx.get(), clock->now()));
+    // Pinning the cursor should fail with the same error code that interrupted the OpCtx. The
+    // cursor should no longer be present in the manager.
+    ASSERT_EQ(cursorManager->pinCursor(_opCtx.get(), cursorId).getStatus(),
+              ErrorCodes::InternalError);
     ASSERT_EQ(0UL, cursorManager->numCursors());
 }
 
 /**
- * Test that client cursors which have been marked as killed but are still pinned *do not* time out.
+ * Test that client cursors which have been marked as killed time out and get deleted.
  */
-TEST_F(CursorManagerTest, InactiveKilledCursorsThatAreStillPinnedShouldNotTimeout) {
+TEST_F(CursorManagerTest, InactiveKilledCursorsShouldTimeout) {
     CursorManager* cursorManager = useCursorManager();
     auto clock = useClock();
 
-    // Make a cursor from the plan executor, and immediately kill it.
     auto cursorPin = cursorManager->registerCursor(
         _opCtx.get(),
         {makeFakePlanExecutor(),
@@ -417,16 +324,19 @@ TEST_F(CursorManagerTest, InactiveKilledCursorsThatAreStillPinnedShouldNotTimeou
          repl::ReadConcernArgs(repl::ReadConcernLevel::kLocalReadConcern),
          BSONObj(),
          ClientCursorParams::LockPolicy::kLocksInternally});
-    auto cursorFreer = makeGuard([&] { cursorPin.deleteUnderlying(); });
-    const bool collectionGoingAway = false;
-    cursorManager->invalidateAll(
-        _opCtx.get(), collectionGoingAway, "KilledCursorsShouldTimeoutTest");
 
-    // Advance the clock to simulate time passing.
-    clock->advance(getDefaultCursorTimeoutMillis());
+    // A cursor will stay alive, but be marked as killed, if it is interrupted with a code other
+    // than ErrorCodes::Interrupted or ErrorCodes::CursorKilled and then unpinned.
+    _opCtx->markKilled(ErrorCodes::InternalError);
+    cursorPin.release();
 
-    // The pin is still in scope, so it should not time out.
-    ASSERT_EQ(0UL, cursorManager->timeoutCursors(_opCtx.get(), clock->now()));
+    // The cursor should still be present in the manager.
+    ASSERT_EQ(1UL, cursorManager->numCursors());
+
+    // Advance the clock to simulate time passing, and verify that the cursor times out.
+    clock->advance(getDefaultCursorTimeoutMillis());
+    ASSERT_EQ(1UL, cursorManager->timeoutCursors(_opCtx.get(), clock->now()));
+    ASSERT_EQ(0UL, cursorManager->numCursors());
 }
 
 /**