From 1d644fa898de455bb160200e198c6895d5590409 Mon Sep 17 00:00:00 2001 From: David Storch Date: Wed, 30 Jan 2019 12:18:50 -0500 Subject: SERVER-38288 Delete CursorManager::invalidateAll(). --- src/mongo/db/catalog/index_catalog.h | 4 +- src/mongo/db/clientcursor.cpp | 19 ++-- src/mongo/db/clientcursor.h | 4 +- src/mongo/db/cursor_manager.cpp | 57 ++--------- src/mongo/db/cursor_manager.h | 11 +-- src/mongo/db/s/database_sharding_state.cpp | 3 - src/mongo/dbtests/cursor_manager_test.cpp | 152 ++++++----------------------- 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(_cursor)); + _cursorManager->unpin(_opCtx, std::unique_ptr(_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>>()) {} 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> 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 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(_opCtx->getServiceContext()->getPreciseClockSource()); } - virtual ~CursorManagerTest() { - useCursorManager()->invalidateAll(_opCtx.get(), true, "end of test"); - } - std::unique_ptr 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 { @@ -163,101 +160,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. */ @@ -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()); } /** -- cgit v1.2.1