diff options
23 files changed, 119 insertions, 607 deletions
diff --git a/jstests/core/txns/kill_transaction_cursors_after_commit.js b/jstests/core/txns/kill_transaction_cursors_after_commit.js new file mode 100644 index 00000000000..50aa93b749f --- /dev/null +++ b/jstests/core/txns/kill_transaction_cursors_after_commit.js @@ -0,0 +1,35 @@ +// Tests that cursors created in transactions may be killed outside of the transaction. +// @tags: [uses_transactions] +(function() { + "use strict"; + + const dbName = "test"; + const collName = "kill_transaction_cursors"; + const testDB = db.getSiblingDB(dbName); + const session = db.getMongo().startSession({causalConsistency: false}); + const sessionDb = session.getDatabase(dbName); + const sessionColl = sessionDb[collName]; + + sessionColl.drop(); + for (let i = 0; i < 4; ++i) { + assert.commandWorked(sessionColl.insert({_id: i})); + } + + jsTest.log("Test that cursors created in transactions may be kill outside of the transaction."); + session.startTransaction(); + let res = assert.commandWorked(sessionDb.runCommand({find: collName, batchSize: 2})); + assert(res.hasOwnProperty("cursor"), tojson(res)); + assert(res.cursor.hasOwnProperty("id"), tojson(res)); + session.commitTransaction(); + assert.commandWorked(sessionDb.runCommand({killCursors: collName, cursors: [res.cursor.id]})); + + jsTest.log("Test that cursors created in transactions may be kill outside of the session."); + session.startTransaction(); + res = assert.commandWorked(sessionDb.runCommand({find: collName, batchSize: 2})); + assert(res.hasOwnProperty("cursor"), tojson(res)); + assert(res.cursor.hasOwnProperty("id"), tojson(res)); + session.commitTransaction(); + assert.commandWorked(testDB.runCommand({killCursors: collName, cursors: [res.cursor.id]})); + + session.endSession(); +}()); diff --git a/jstests/core/txns/multi_statement_transaction.js b/jstests/core/txns/multi_statement_transaction.js index 8f182716c1f..e43a243300c 100644 --- a/jstests/core/txns/multi_statement_transaction.js +++ b/jstests/core/txns/multi_statement_transaction.js @@ -155,36 +155,5 @@ assert.eq(null, testColl.findOne({_id: "doc-2"})); assert.eq(null, testColl.findOne({_id: "doc-3"})); - // Open a client cursor under a new transaction. - assert.commandWorked(testColl.remove({}, {writeConcern: {w: "majority"}})); - - assert.commandWorked( - testColl.insert([{_id: "doc-1"}, {_id: "doc-2"}], {writeConcern: {w: "majority"}})); - - session.startTransaction(); - - let res = sessionDb.runCommand({ - find: collName, - batchSize: 0, - }); - assert.commandWorked(res); - assert(res.hasOwnProperty("cursor")); - assert(res.cursor.hasOwnProperty("firstBatch")); - assert.eq(0, res.cursor.firstBatch.length); - assert(res.cursor.hasOwnProperty("id")); - const cursorId = res.cursor.id; - assert.neq(0, cursorId); - - // Commit the transaction. - session.commitTransaction(); - - // Perform a getMore using the previous transaction's open cursorId. We expect to receive - // CursorNotFound if the cursor was properly closed on commit. - assert.commandFailedWithCode(testDB.runCommand({ - getMore: cursorId, - collection: collName, - }), - ErrorCodes.CursorNotFound); - session.endSession(); }()); diff --git a/jstests/core/txns/multi_statement_transaction_abort.js b/jstests/core/txns/multi_statement_transaction_abort.js index 42f582fad52..470fd3bd57a 100644 --- a/jstests/core/txns/multi_statement_transaction_abort.js +++ b/jstests/core/txns/multi_statement_transaction_abort.js @@ -251,76 +251,5 @@ autocommit: false })); - function parseCursor(cmdResult) { - assert(cmdResult.hasOwnProperty("cursor"), tojson(cmdResult)); - assert(cmdResult.cursor.hasOwnProperty("id")); - assert(cmdResult.cursor.hasOwnProperty("firstBatch") || - cmdResult.cursor.hasOwnProperty("nextBatch"), - tojson(cmdResult)); - return cmdResult.cursor; - } - - function testCursorCleanupOnAbort(cursorCmd) { - // Perform a snapshot read which returns an open cursor. - let cmdResult = sessionDb.runCommand(cursorCmd); - assert.commandWorked(cmdResult); - let cursor = parseCursor(cmdResult); - assert.eq(0, cursor.firstBatch.length); - let cursorId = cursor.id; - assert.neq(0, cursorId); - - // Read the first document. - cmdResult = assert.commandWorked(sessionDb.runCommand({ - getMore: cursorId, - collection: collName, - batchSize: 1, - txnNumber: NumberLong(txnNumber), - autocommit: false - })); - assert.commandWorked(cmdResult); - cursor = parseCursor(cmdResult); - assert.eq(1, cursor.nextBatch.length); - cursorId = cursor.id; - assert.neq(0, cursorId); - - // Abort the transaction. - assert.commandWorked(sessionDb.adminCommand({ - abortTransaction: 1, - writeConcern: {w: "majority"}, - txnNumber: NumberLong(txnNumber), - autocommit: false - })); - - // Perform a getMore using the previous transaction's open cursorId. We expect to receive - // CursorNotFound if the cursor was properly closed on abort. - assert.commandFailedWithCode(testDB.runCommand({ - getMore: cursorId, - collection: collName, - }), - ErrorCodes.CursorNotFound); - } - - txnNumber++; - testCursorCleanupOnAbort({ - find: collName, - sort: {_id: 1}, - batchSize: 0, - txnNumber: NumberLong(txnNumber), - readConcern: {level: "snapshot"}, - startTransaction: true, - autocommit: false, - }); - - txnNumber++; - testCursorCleanupOnAbort({ - aggregate: collName, - pipeline: [], - cursor: {batchSize: 0}, - txnNumber: NumberLong(txnNumber), - readConcern: {level: "snapshot"}, - startTransaction: true, - autocommit: false - }); - session.endSession(); }()); diff --git a/jstests/noPassthrough/snapshot_cursor_integrity.js b/jstests/noPassthrough/snapshot_cursor_integrity.js index 7e99581bc2d..d5d0678a641 100644 --- a/jstests/noPassthrough/snapshot_cursor_integrity.js +++ b/jstests/noPassthrough/snapshot_cursor_integrity.js @@ -96,7 +96,7 @@ txnNumber: NumberLong(1), batchSize: 2 }), - ErrorCodes.CursorNotFound); + 50741); // The cursor can no longer be iterated because its transaction has ended. assert.commandFailedWithCode(sessionDB1.runCommand({ @@ -108,6 +108,10 @@ }), ErrorCodes.TransactionTooOld); + // Kill the cursor. + assert.commandWorked( + sessionDB1.runCommand({killCursors: sessionDB1.coll.getName(), cursors: [cursorID]})); + // Establish a cursor outside of any transaction in session1. res = assert.commandWorked(sessionDB1.runCommand({find: collName, batchSize: 2})); assert(res.hasOwnProperty("cursor")); diff --git a/jstests/noPassthrough/snapshot_cursor_shutdown_stepdown.js b/jstests/noPassthrough/snapshot_cursor_shutdown_stepdown.js index c07442dbff9..8d60e290d7d 100644 --- a/jstests/noPassthrough/snapshot_cursor_shutdown_stepdown.js +++ b/jstests/noPassthrough/snapshot_cursor_shutdown_stepdown.js @@ -75,11 +75,8 @@ stepdownFunc(rst); rst.waitForState(primary, ReplSetTest.State.SECONDARY); - // Perform a getMore using the previous transaction's open cursorId. We expect to receive - // CursorNotFound if the cursor was properly closed on step down. - assert.commandFailedWithCode( - sessionDB.runCommand({getMore: cursorId, collection: collName}), - ErrorCodes.CursorNotFound); + // Kill the cursor. + assert.commandWorked(sessionDB.runCommand({killCursors: collName, cursors: [cursorId]})); rst.stopSet(); } diff --git a/src/mongo/db/cursor_manager.cpp b/src/mongo/db/cursor_manager.cpp index 5f6d5d754b8..cfd58e44202 100644 --- a/src/mongo/db/cursor_manager.cpp +++ b/src/mongo/db/cursor_manager.cpp @@ -59,13 +59,8 @@ namespace mongo { -MONGO_INITIALIZER(RegisterCursorKillFunction) +MONGO_INITIALIZER(RegisterCursorExistsFunction) (InitializerContext* const) { - Session::registerCursorKillFunction( - [](OperationContext* opCtx, LogicalSessionId lsid, TxnNumber txnNumber) { - return CursorManager::killAllCursorsForTransaction(opCtx, lsid, txnNumber); - }); - Session::registerCursorExistsFunction([](LogicalSessionId lsid, TxnNumber txnNumber) { return CursorManager::hasTransactionCursorReference(lsid, txnNumber); }); @@ -134,10 +129,6 @@ public: size_t numOpenCursorsForTransaction(LogicalSessionId lsid, TxnNumber txnNumber); - size_t killAllCursorsForTransaction(OperationContext* opCtx, - LogicalSessionId lsid, - TxnNumber txnNumber); - private: // '_mutex' must not be held when acquiring a CursorManager mutex to avoid deadlock. SimpleMutex _mutex; @@ -213,40 +204,6 @@ size_t GlobalCursorIdCache::numOpenCursorsForTransaction(LogicalSessionId lsid, return _lsidToTxnCursorMap[lsid][txnNumber].size(); } -size_t GlobalCursorIdCache::killAllCursorsForTransaction(OperationContext* opCtx, - LogicalSessionId lsid, - TxnNumber txnNumber) { - size_t killCount = 0; - CursorIdToNssMap cursorToNssMap; - { - // Copy the cursorId/NamespaceString map to a local structure to avoid obtaining the - // CursorManager mutex while holding the GlobalCursorIdCache mutex. - stdx::lock_guard<SimpleMutex> lk(_mutex); - for (auto&& cursorIdAndNss : _lsidToTxnCursorMap[lsid][txnNumber]) { - cursorToNssMap.insert(cursorIdAndNss); - } - } - - for (auto&& cursorIdAndNss : cursorToNssMap) { - const auto cursorId = cursorIdAndNss.first; - const auto nss = cursorIdAndNss.second; - - auto status = CursorManager::withCursorManager( - opCtx, cursorId, nss, [opCtx, cursorId, lsid, txnNumber](CursorManager* manager) { - const auto shouldAudit = false; - return manager->killCursor(opCtx, cursorId, shouldAudit, lsid, txnNumber); - }); - - if (status.isOK()) { - ++killCount; - } - - invariant(status.isOK() || status.code() == ErrorCodes::CursorNotFound); - } - - return killCount; -} - uint32_t GlobalCursorIdCache::registerCursorManager(const NamespaceString& nss) { static const uint32_t kMaxIds = 1000 * 1000 * 1000; static_assert((kMaxIds & (0b11 << 30)) == 0, @@ -477,12 +434,6 @@ std::size_t CursorManager::timeoutCursorsGlobal(OperationContext* opCtx, Date_t return globalCursorIdCache->timeoutCursors(opCtx, now); } -size_t CursorManager::killAllCursorsForTransaction(OperationContext* opCtx, - LogicalSessionId lsid, - TxnNumber txnNumber) { - return globalCursorIdCache->killAllCursorsForTransaction(opCtx, lsid, txnNumber); -} - bool CursorManager::hasTransactionCursorReference(LogicalSessionId lsid, TxnNumber txnNumber) { return globalCursorIdCache->numOpenCursorsForTransaction(lsid, txnNumber) > 0; } @@ -881,11 +832,7 @@ void CursorManager::deregisterAndDestroyCursor( cursor->dispose(opCtx); } -Status CursorManager::killCursor(OperationContext* opCtx, - CursorId id, - bool shouldAudit, - boost::optional<LogicalSessionId> lsid, - boost::optional<TxnNumber> txnNumber) { +Status CursorManager::killCursor(OperationContext* opCtx, CursorId id, bool shouldAudit) { auto lockedPartition = _cursorMap->lockOnePartition(id); auto it = lockedPartition->find(id); if (it == lockedPartition->end()) { @@ -897,17 +844,6 @@ Status CursorManager::killCursor(OperationContext* opCtx, } auto cursor = it->second; - if (lsid && lsid != cursor->getSessionId()) { - return { - ErrorCodes::CursorNotFound, - str::stream() << "killCursor LogicalSessionId must match that of cursor when provided"}; - } - - if (txnNumber && txnNumber != cursor->getTxnNumber()) { - return {ErrorCodes::CursorNotFound, - str::stream() << "killCursor TxnNumber must match that of cursor when provided"}; - } - if (cursor->_operationUsingCursor) { // Rather than removing the cursor directly, kill the operation that's currently using the // cursor. It will stop on its own (and remove the cursor) when it sees that it's been diff --git a/src/mongo/db/cursor_manager.h b/src/mongo/db/cursor_manager.h index 350e23b62d3..5ca69676757 100644 --- a/src/mongo/db/cursor_manager.h +++ b/src/mongo/db/cursor_manager.h @@ -101,14 +101,6 @@ public: OperationContext* opCtx, const SessionKiller::Matcher& matcher); /** - * Kills all cursors with matching logical session and transaction number. Returns the number of - * cursors successfully killed. - */ - static size_t killAllCursorsForTransaction(OperationContext* opCtx, - LogicalSessionId lsid, - TxnNumber txnNumber); - - /** * Returns true if the CursorManager has cursor references for the given session ID and * transaction number. */ @@ -197,14 +189,9 @@ public: * Returns ErrorCodes::CursorNotFound if the cursor id is not owned by this manager. Returns * ErrorCodes::OperationFailed if attempting to erase a pinned cursor. * - * If 'shouldAudit' is true, will perform audit logging. If 'lsid' or 'txnNumber' are provided - * we will confirm that the cursor is owned by the given session or transaction. + * If 'shouldAudit' is true, will perform audit logging. */ - Status killCursor(OperationContext* opCtx, - CursorId id, - bool shouldAudit, - boost::optional<LogicalSessionId> lsid = boost::none, - boost::optional<TxnNumber> txnNumber = boost::none); + Status killCursor(OperationContext* opCtx, CursorId id, bool shouldAudit); /** * Returns an OK status if we're authorized to erase the cursor. Otherwise, returns diff --git a/src/mongo/db/db_raii.cpp b/src/mongo/db/db_raii.cpp index 8669e6da56d..0b95c28efe4 100644 --- a/src/mongo/db/db_raii.cpp +++ b/src/mongo/db/db_raii.cpp @@ -369,7 +369,8 @@ LockMode getLockModeForQuery(OperationContext* opCtx) { invariant(opCtx); // Use IX locks for autocommit:false multi-statement transactions; otherwise, use IS locks. - if (Session::TransactionState::get(opCtx).requiresIXReadUpgrade) { + auto session = OperationContextSession::get(opCtx); + if (session && session->inMultiDocumentTransaction()) { return MODE_IX; } return MODE_IS; diff --git a/src/mongo/db/kill_sessions_local.cpp b/src/mongo/db/kill_sessions_local.cpp index 426c264d599..29562cc0a67 100644 --- a/src/mongo/db/kill_sessions_local.cpp +++ b/src/mongo/db/kill_sessions_local.cpp @@ -51,19 +51,10 @@ void killSessionsLocalKillCursors(OperationContext* opCtx, const SessionKiller:: } // namespace void killSessionsLocalKillTransactions(OperationContext* opCtx, - const SessionKiller::Matcher& matcher, - bool shouldKillClientCursors) { - SessionCatalog::get(opCtx)->scanSessions( - opCtx, matcher, [shouldKillClientCursors](OperationContext* opCtx, Session* session) { - session->abortArbitraryTransaction(opCtx, shouldKillClientCursors); - }); -} - -void killSessionsLocalKillTransactionCursors(OperationContext* opCtx, - const SessionKiller::Matcher& matcher) { + const SessionKiller::Matcher& matcher) { SessionCatalog::get(opCtx)->scanSessions( opCtx, matcher, [](OperationContext* opCtx, Session* session) { - session->killTransactionCursors(opCtx); + session->abortArbitraryTransaction(); }); } @@ -82,7 +73,7 @@ void killAllExpiredTransactions(OperationContext* opCtx) { SessionCatalog::get(opCtx)->scanSessions( opCtx, matcherAllSessions, [](OperationContext* opCtx, Session* session) { try { - session->abortArbitraryTransactionIfExpired(opCtx); + session->abortArbitraryTransactionIfExpired(); } catch (const DBException& ex) { Status status = ex.toStatus(); std::string errmsg = str::stream() diff --git a/src/mongo/db/kill_sessions_local.h b/src/mongo/db/kill_sessions_local.h index 2022a0fda05..a21bcec4ae5 100644 --- a/src/mongo/db/kill_sessions_local.h +++ b/src/mongo/db/kill_sessions_local.h @@ -46,14 +46,7 @@ SessionKiller::Result killSessionsLocal(OperationContext* opCtx, * Kills all transactions on mongod for sessions matching 'matcher'. */ void killSessionsLocalKillTransactions(OperationContext* opCtx, - const SessionKiller::Matcher& matcher, - bool shouldKillClientCursors = true); - -/** - * Kills all transactions cursors on mongod for sessions matching 'matcher'. - */ -void killSessionsLocalKillTransactionCursors(OperationContext* opCtx, - const SessionKiller::Matcher& matcher); + const SessionKiller::Matcher& matcher); /** * Aborts any expired transactions. diff --git a/src/mongo/db/query/query_test_service_context.cpp b/src/mongo/db/query/query_test_service_context.cpp index 407a0341792..d752db15664 100644 --- a/src/mongo/db/query/query_test_service_context.cpp +++ b/src/mongo/db/query/query_test_service_context.cpp @@ -46,12 +46,9 @@ ServiceContext::UniqueOperationContext QueryTestServiceContext::makeOperationCon } ServiceContext::UniqueOperationContext QueryTestServiceContext::makeOperationContext( - LogicalSessionId lsid, boost::optional<TxnNumber> txnNumber) { + LogicalSessionId lsid) { auto opCtx = makeOperationContext(); opCtx->setLogicalSessionId(lsid); - if (txnNumber) { - opCtx->setTxnNumber(*txnNumber); - } return opCtx; } diff --git a/src/mongo/db/query/query_test_service_context.h b/src/mongo/db/query/query_test_service_context.h index bbf265a06ac..c52944cf2f5 100644 --- a/src/mongo/db/query/query_test_service_context.h +++ b/src/mongo/db/query/query_test_service_context.h @@ -45,8 +45,7 @@ public: ServiceContext::UniqueOperationContext makeOperationContext(); - ServiceContext::UniqueOperationContext makeOperationContext( - LogicalSessionId lsid, boost::optional<TxnNumber> txnNumber); + ServiceContext::UniqueOperationContext makeOperationContext(LogicalSessionId lsid); Client* getClient() const; diff --git a/src/mongo/db/repl/replication_coordinator_external_state.h b/src/mongo/db/repl/replication_coordinator_external_state.h index fa85660bdd4..a7a2c3637be 100644 --- a/src/mongo/db/repl/replication_coordinator_external_state.h +++ b/src/mongo/db/repl/replication_coordinator_external_state.h @@ -213,11 +213,6 @@ public: virtual void killAllUserOperations(OperationContext* opCtx) = 0; /** - * Kills all transaction owned client cursors. Used during stepdown. - */ - virtual void killAllTransactionCursors(OperationContext* opCtx) = 0; - - /** * Resets any active sharding metadata on this server and stops any sharding-related threads * (such as the balancer). It is called after stepDown to ensure that if the node becomes * primary again in the future it will recover its state from a clean slate. diff --git a/src/mongo/db/repl/replication_coordinator_external_state_impl.cpp b/src/mongo/db/repl/replication_coordinator_external_state_impl.cpp index 1a1e4bbddd2..d86129a2699 100644 --- a/src/mongo/db/repl/replication_coordinator_external_state_impl.cpp +++ b/src/mongo/db/repl/replication_coordinator_external_state_impl.cpp @@ -662,14 +662,7 @@ void ReplicationCoordinatorExternalStateImpl::killAllUserOperations(OperationCon // Destroy all stashed transaction resources, in order to release locks. SessionKiller::Matcher matcherAllSessions( KillAllSessionsByPatternSet{makeKillAllSessionsByPattern(opCtx)}); - bool killCursors = false; - killSessionsLocalKillTransactions(opCtx, matcherAllSessions, killCursors); -} - -void ReplicationCoordinatorExternalStateImpl::killAllTransactionCursors(OperationContext* opCtx) { - SessionKiller::Matcher matcherAllSessions( - KillAllSessionsByPatternSet{makeKillAllSessionsByPattern(opCtx)}); - killSessionsLocalKillTransactionCursors(opCtx, matcherAllSessions); + killSessionsLocalKillTransactions(opCtx, matcherAllSessions); } void ReplicationCoordinatorExternalStateImpl::shardingOnStepDownHook() { diff --git a/src/mongo/db/repl/replication_coordinator_external_state_impl.h b/src/mongo/db/repl/replication_coordinator_external_state_impl.h index 32479c8d2ee..98f209e3135 100644 --- a/src/mongo/db/repl/replication_coordinator_external_state_impl.h +++ b/src/mongo/db/repl/replication_coordinator_external_state_impl.h @@ -92,7 +92,6 @@ public: virtual HostAndPort getClientHostAndPort(const OperationContext* opCtx); virtual void closeConnections(); virtual void killAllUserOperations(OperationContext* opCtx); - virtual void killAllTransactionCursors(OperationContext* opCtx); virtual void shardingOnStepDownHook(); virtual void signalApplierToChooseNewSyncSource(); virtual void stopProducer(); diff --git a/src/mongo/db/repl/replication_coordinator_external_state_mock.cpp b/src/mongo/db/repl/replication_coordinator_external_state_mock.cpp index bccf4d27c2c..23280156d26 100644 --- a/src/mongo/db/repl/replication_coordinator_external_state_mock.cpp +++ b/src/mongo/db/repl/replication_coordinator_external_state_mock.cpp @@ -201,8 +201,6 @@ void ReplicationCoordinatorExternalStateMock::closeConnections() { void ReplicationCoordinatorExternalStateMock::killAllUserOperations(OperationContext* opCtx) {} -void ReplicationCoordinatorExternalStateMock::killAllTransactionCursors(OperationContext* opCtx) {} - void ReplicationCoordinatorExternalStateMock::shardingOnStepDownHook() {} void ReplicationCoordinatorExternalStateMock::signalApplierToChooseNewSyncSource() {} diff --git a/src/mongo/db/repl/replication_coordinator_external_state_mock.h b/src/mongo/db/repl/replication_coordinator_external_state_mock.h index 30086dae707..8440b286661 100644 --- a/src/mongo/db/repl/replication_coordinator_external_state_mock.h +++ b/src/mongo/db/repl/replication_coordinator_external_state_mock.h @@ -81,7 +81,6 @@ public: virtual StatusWith<OpTime> loadLastOpTime(OperationContext* opCtx); virtual void closeConnections(); virtual void killAllUserOperations(OperationContext* opCtx); - virtual void killAllTransactionCursors(OperationContext* opCtx); virtual void shardingOnStepDownHook(); virtual void signalApplierToChooseNewSyncSource(); virtual void stopProducer(); diff --git a/src/mongo/db/repl/replication_coordinator_impl.cpp b/src/mongo/db/repl/replication_coordinator_impl.cpp index 4df3752c4ca..64e05ec1eec 100644 --- a/src/mongo/db/repl/replication_coordinator_impl.cpp +++ b/src/mongo/db/repl/replication_coordinator_impl.cpp @@ -1642,10 +1642,6 @@ Status ReplicationCoordinatorImpl::stepDown(OperationContext* opCtx, "specified that we should step down for"}; } - // TODO SERVER-34395: Remove this method and kill cursors as part of killAllUserOperations call - // when the CursorManager no longer requires collection locks to kill cursors. - _externalState->killAllTransactionCursors(opCtx); - stdx::unique_lock<stdx::mutex> lk(_mutex); auto status = opCtx->checkForInterruptNoAssert(); diff --git a/src/mongo/db/repl/replication_coordinator_impl_heartbeat.cpp b/src/mongo/db/repl/replication_coordinator_impl_heartbeat.cpp index a8e9187618a..7ef149b7224 100644 --- a/src/mongo/db/repl/replication_coordinator_impl_heartbeat.cpp +++ b/src/mongo/db/repl/replication_coordinator_impl_heartbeat.cpp @@ -403,10 +403,6 @@ void ReplicationCoordinatorImpl::_stepDownFinish( globalExclusiveLock.waitForLockUntil(Date_t::max()); invariant(globalExclusiveLock.isLocked()); - // TODO SERVER-34395: Remove this method and kill cursors as part of killAllUserOperations call - // when the CursorManager no longer requires collection locks to kill cursors. - _externalState->killAllTransactionCursors(opCtx.get()); - stdx::unique_lock<stdx::mutex> lk(_mutex); _topCoord->finishUnconditionalStepDown(); diff --git a/src/mongo/db/session.cpp b/src/mongo/db/session.cpp index dc2997a7a13..aa7ee6a1f0b 100644 --- a/src/mongo/db/session.cpp +++ b/src/mongo/db/session.cpp @@ -71,10 +71,6 @@ namespace mongo { // abort a deadlocked transaction operation to eliminate the deadlock for however long has been set. MONGO_EXPORT_SERVER_PARAMETER(maxTransactionLockRequestTimeoutMillis, int, 0); -const OperationContext::Decoration<Session::TransactionState> Session::TransactionState::get = - OperationContext::declareDecoration<Session::TransactionState>(); - -Session::CursorKillFunction Session::_cursorKillFunction; Session::CursorExistsFunction Session::_cursorExistsFunction; // Server parameter that dictates the lifetime given to each transaction. @@ -366,33 +362,16 @@ void Session::beginOrContinueTxn(OperationContext* opCtx, (dbName != "admin"_sd || txnAdminCommands.find(cmdName) != txnAdminCommands.cend()))); - TxnNumber txnNumberAtStart; - bool canKillCursors = false; - { - stdx::lock_guard<stdx::mutex> lg(_mutex); - txnNumberAtStart = _activeTxnNumber; - _beginOrContinueTxn(lg, opCtx, txnNumber, autocommit, startTransaction, &canKillCursors); - } - - if (canKillCursors) { - _killTransactionCursors(opCtx, _sessionId, txnNumberAtStart); - } + stdx::lock_guard<stdx::mutex> lg(_mutex); + _beginOrContinueTxn(lg, txnNumber, autocommit, startTransaction); } void Session::beginOrContinueTxnOnMigration(OperationContext* opCtx, TxnNumber txnNumber) { invariant(!opCtx->getClient()->isInDirectClient()); invariant(!opCtx->lockState()->isLocked()); - TxnNumber txnNumberAtStart; - bool canKillCursors = false; - { - stdx::lock_guard<stdx::mutex> lg(_mutex); - txnNumberAtStart = _activeTxnNumber; - _beginOrContinueTxnOnMigration(lg, opCtx, txnNumber, &canKillCursors); - } - if (canKillCursors) { - _killTransactionCursors(opCtx, _sessionId, txnNumberAtStart); - } + stdx::lock_guard<stdx::mutex> lg(_mutex); + _beginOrContinueTxnOnMigration(lg, txnNumber); } void Session::setSpeculativeTransactionOpTimeToLastApplied(OperationContext* opCtx) { @@ -554,11 +533,9 @@ bool Session::checkStatementExecutedNoOplogEntryFetch(TxnNumber txnNumber, StmtI } void Session::_beginOrContinueTxn(WithLock wl, - OperationContext* opCtx, TxnNumber txnNumber, boost::optional<bool> autocommit, - boost::optional<bool> startTransaction, - bool* canKillCursors) { + boost::optional<bool> startTransaction) { // Check whether the session information needs to be refreshed from disk. _checkValid(wl); @@ -567,11 +544,6 @@ void Session::_beginOrContinueTxn(WithLock wl, // be >= the active transaction number. _checkTxnValid(wl, txnNumber); - ON_BLOCK_EXIT([this, opCtx] { - Session::TransactionState::get(opCtx).requiresIXReadUpgrade = - _txnState == MultiDocumentTransactionState::kInProgress; - }); - // // Continue an active transaction. // @@ -606,7 +578,7 @@ void Session::_beginOrContinueTxn(WithLock wl, // implicitly abort the transaction. It is not safe to continue the transaction, in // particular because we have not saved the readConcern from the first statement of // the transaction. - _abortTransaction(wl, opCtx, canKillCursors); + _abortTransaction(wl); uasserted(ErrorCodes::NoSuchTransaction, str::stream() << "Transaction " << txnNumber << " has been aborted."); } @@ -640,7 +612,7 @@ void Session::_beginOrContinueTxn(WithLock wl, serverGlobalParams.featureCompatibility.getVersion() == ServerGlobalParams::FeatureCompatibility::Version::kFullyUpgradedTo40)); - _setActiveTxn(wl, opCtx, txnNumber, canKillCursors); + _setActiveTxn(wl, txnNumber); _autocommit = false; _txnState = MultiDocumentTransactionState::kInProgress; _transactionExpireDate = @@ -648,7 +620,7 @@ void Session::_beginOrContinueTxn(WithLock wl, } else { // Execute a retryable write or snapshot read. invariant(startTransaction == boost::none); - _setActiveTxn(wl, opCtx, txnNumber, canKillCursors); + _setActiveTxn(wl, txnNumber); _autocommit = true; _txnState = MultiDocumentTransactionState::kNone; _singleTransactionStats = boost::none; @@ -868,90 +840,51 @@ void Session::unstashTransactionResources(OperationContext* opCtx, const std::st } } -void Session::abortArbitraryTransaction(OperationContext* opCtx, bool shouldKillClientCursors) { - TxnNumber txnNumberAtStart; - bool canKillCursors = false; - { - stdx::lock_guard<stdx::mutex> lock(_mutex); - txnNumberAtStart = _activeTxnNumber; - _abortArbitraryTransaction(lock, opCtx, &canKillCursors); - } - - if (shouldKillClientCursors && canKillCursors) { - _killTransactionCursors(opCtx, _sessionId, txnNumberAtStart); - } +void Session::abortArbitraryTransaction() { + stdx::lock_guard<stdx::mutex> lock(_mutex); + _abortArbitraryTransaction(lock); } -void Session::abortArbitraryTransactionIfExpired(OperationContext* opCtx) { - TxnNumber txnNumberAtStart; - bool canKillCursors = false; - { - stdx::lock_guard<stdx::mutex> lock(_mutex); - if (!_transactionExpireDate || _transactionExpireDate >= Date_t::now()) { - return; - } - txnNumberAtStart = _activeTxnNumber; - _abortArbitraryTransaction(lock, opCtx, &canKillCursors); - } - - if (canKillCursors) { - _killTransactionCursors(opCtx, _sessionId, txnNumberAtStart); +void Session::abortArbitraryTransactionIfExpired() { + stdx::lock_guard<stdx::mutex> lock(_mutex); + if (!_transactionExpireDate || _transactionExpireDate >= Date_t::now()) { + return; } + _abortArbitraryTransaction(lock); } -void Session::_abortArbitraryTransaction(WithLock lock, - OperationContext* opCtx, - bool* canKillCursors) { +void Session::_abortArbitraryTransaction(WithLock lock) { if (_txnState != MultiDocumentTransactionState::kInProgress && _txnState != MultiDocumentTransactionState::kInSnapshotRead) { return; } - _abortTransaction(lock, opCtx, canKillCursors); + _abortTransaction(lock); } void Session::abortActiveTransaction(OperationContext* opCtx) { - TxnNumber txnNumberAtStart; - bool canKillCursors = false; - { - stdx::unique_lock<Client> clientLock(*opCtx->getClient()); - stdx::lock_guard<stdx::mutex> lock(_mutex); - txnNumberAtStart = _activeTxnNumber; - - if (_txnState != MultiDocumentTransactionState::kInProgress && - _txnState != MultiDocumentTransactionState::kInSnapshotRead) { - return; - } - - _abortTransaction(lock, opCtx, &canKillCursors); + stdx::unique_lock<Client> clientLock(*opCtx->getClient()); + stdx::lock_guard<stdx::mutex> lock(_mutex); - // Abort the WUOW. We should be able to abort empty transactions that don't have WUOW. - if (opCtx->getWriteUnitOfWork()) { - opCtx->setWriteUnitOfWork(nullptr); - } - // We must clear the recovery unit and locker so any post-transaction writes can run without - // transactional settings such as a read timestamp. - opCtx->setRecoveryUnit(opCtx->getServiceContext()->getStorageEngine()->newRecoveryUnit(), - WriteUnitOfWork::RecoveryUnitState::kNotInUnitOfWork); - opCtx->lockState()->unsetMaxLockTimeout(); - } - if (canKillCursors) { - _killTransactionCursors(opCtx, _sessionId, txnNumberAtStart); + if (_txnState != MultiDocumentTransactionState::kInProgress && + _txnState != MultiDocumentTransactionState::kInSnapshotRead) { + return; } -} -void Session::killTransactionCursors(OperationContext* opCtx) { - TxnNumber txnNumberAtStart; - { - stdx::lock_guard<stdx::mutex> lk(_mutex); - txnNumberAtStart = _activeTxnNumber; - } + _abortTransaction(lock); - _killTransactionCursors(opCtx, _sessionId, txnNumberAtStart); + // Abort the WUOW. We should be able to abort empty transactions that don't have WUOW. + if (opCtx->getWriteUnitOfWork()) { + opCtx->setWriteUnitOfWork(nullptr); + } + // We must clear the recovery unit and locker so any post-transaction writes can run without + // transactional settings such as a read timestamp. + opCtx->setRecoveryUnit(opCtx->getServiceContext()->getStorageEngine()->newRecoveryUnit(), + WriteUnitOfWork::RecoveryUnitState::kNotInUnitOfWork); + opCtx->lockState()->unsetMaxLockTimeout(); } -void Session::_abortTransaction(WithLock wl, OperationContext* opCtx, bool* canKillCursors) { - invariant(canKillCursors); +void Session::_abortTransaction(WithLock wl) { // TODO SERVER-33432 Disallow aborting committed transaction after we implement implicit abort. // A transaction in kCommitting state will either commit or abort for storage-layer reasons; it // is too late to abort externally. @@ -964,13 +897,9 @@ void Session::_abortTransaction(WithLock wl, OperationContext* opCtx, bool* canK _transactionOperations.clear(); _txnState = MultiDocumentTransactionState::kAborted; _speculativeTransactionReadOpTime = repl::OpTime(); - *canKillCursors = true; } -void Session::_beginOrContinueTxnOnMigration(WithLock wl, - OperationContext* opCtx, - TxnNumber txnNumber, - bool* canKillCursors) { +void Session::_beginOrContinueTxnOnMigration(WithLock wl, TxnNumber txnNumber) { _checkValid(wl); _checkTxnValid(wl, txnNumber); @@ -978,17 +907,14 @@ void Session::_beginOrContinueTxnOnMigration(WithLock wl, if (txnNumber == _activeTxnNumber) return; - _setActiveTxn(wl, opCtx, txnNumber, canKillCursors); + _setActiveTxn(wl, txnNumber); } -void Session::_setActiveTxn(WithLock wl, - OperationContext* opCtx, - TxnNumber txnNumber, - bool* canKillCursors) { +void Session::_setActiveTxn(WithLock wl, TxnNumber txnNumber) { // Abort the existing transaction if it's not committed or aborted. if (_txnState == MultiDocumentTransactionState::kInProgress || _txnState == MultiDocumentTransactionState::kInSnapshotRead) { - _abortTransaction(wl, opCtx, canKillCursors); + _abortTransaction(wl); } _activeTxnNumber = txnNumber; _activeTxnCommittedStatements.clear(); @@ -1037,19 +963,14 @@ std::vector<repl::ReplOperation> Session::endTransactionAndRetrieveOperations( } void Session::commitTransaction(OperationContext* opCtx) { - TxnNumber txnNumberAtStart; - { - stdx::unique_lock<stdx::mutex> lk(_mutex); - txnNumberAtStart = _activeTxnNumber; + stdx::unique_lock<stdx::mutex> lk(_mutex); - // Always check '_activeTxnNumber' and '_txnState', since they can be modified by - // session kill and migration, which do not check out the session. - _checkIsActiveTransaction(lk, *opCtx->getTxnNumber(), true); + // Always check '_activeTxnNumber' and '_txnState', since they can be modified by session kill + // and migration, which do not check out the session. + _checkIsActiveTransaction(lk, *opCtx->getTxnNumber(), true); - invariant(_txnState != MultiDocumentTransactionState::kCommitted); - _commitTransaction(std::move(lk), opCtx); - } - _killTransactionCursors(opCtx, _sessionId, txnNumberAtStart); + invariant(_txnState != MultiDocumentTransactionState::kCommitted); + _commitTransaction(std::move(lk), opCtx); } void Session::_commitTransaction(stdx::unique_lock<stdx::mutex> lk, OperationContext* opCtx) { @@ -1135,19 +1056,6 @@ void Session::reportStashedState(BSONObjBuilder* builder) const { } } -// TODO SERVER-34395: Remove opCtx from this interface once no longer required. -void Session::_killTransactionCursors(OperationContext* opCtx, - LogicalSessionId lsid, - TxnNumber txnNumber) { - invariant(_cursorKillFunction); - - if (!opCtx) { - return; - } - - _cursorKillFunction(opCtx, lsid, txnNumber); -} - void Session::_checkValid(WithLock) const { uassert(ErrorCodes::ConflictingOperationInProgress, str::stream() << "Session " << getSessionId() @@ -1264,10 +1172,7 @@ void Session::_registerUpdateCacheOnCommit(OperationContext* opCtx, // entry gets invalidated and immediately refreshed while there were no writes for // newTxnNumber yet. In this case _activeTxnNumber will be less than newTxnNumber // and we will fail to update the cache even though the write was successful. - OperationContext* opCtx = nullptr; - bool ignoredCanKillCursors = false; - _beginOrContinueTxn( - lg, opCtx, newTxnNumber, boost::none, boost::none, &ignoredCanKillCursors); + _beginOrContinueTxn(lg, newTxnNumber, boost::none, boost::none); } if (newTxnNumber == _activeTxnNumber) { diff --git a/src/mongo/db/session.h b/src/mongo/db/session.h index 9ca248a2c08..e6e2a9984f2 100644 --- a/src/mongo/db/session.h +++ b/src/mongo/db/session.h @@ -64,12 +64,6 @@ class Session { MONGO_DISALLOW_COPYING(Session); public: - struct TransactionState { - static const OperationContext::Decoration<TransactionState> get; - - bool requiresIXReadUpgrade = false; - }; - /** * Holds state for a snapshot read or multi-statement transaction in between network operations. */ @@ -127,8 +121,6 @@ public: }; using CommittedStatementTimestampMap = stdx::unordered_map<StmtId, repl::OpTime>; - using CursorKillFunction = - std::function<size_t(OperationContext*, LogicalSessionId, TxnNumber)>; using CursorExistsFunction = std::function<bool(LogicalSessionId, TxnNumber)>; static const BSONObj kDeadEndSentinel; @@ -275,14 +267,6 @@ public: */ void unstashTransactionResources(OperationContext* opCtx, const std::string& cmdName); - /** - * Registers a function that will be used to kill client cursors on transaction commit or abort. - * TODO SERVER-34395: Move cursor kill function into Session instead of registering. - */ - static void registerCursorKillFunction(CursorKillFunction cursorKillFunc) { - _cursorKillFunction = cursorKillFunc; - } - // TODO SERVER-34113: Remove the "cursor exists" mechanism from both Session and CursorManager // once snapshot reads outside of multi-statement transcactions are no longer supported. static void registerCursorExistsFunction(CursorExistsFunction cursorExistsFunc) { @@ -298,13 +282,13 @@ public: /** * Aborts the transaction outside the transaction, releasing transaction resources. */ - void abortArbitraryTransaction(OperationContext* opCtx, bool shouldKillClientCursors); + void abortArbitraryTransaction(); /** * Same as abortArbitraryTransaction, except only executes if _transactionExpireDate indicates * that the transaction has expired. */ - void abortArbitraryTransactionIfExpired(OperationContext* opCtx); + void abortArbitraryTransactionIfExpired(); /* * Aborts the transaction inside the transaction, releasing transaction resources. @@ -313,11 +297,6 @@ public: */ void abortActiveTransaction(OperationContext* opCtx); - /** - * Kills any open client cursors associated with the current transaction. - */ - void killTransactionCursors(OperationContext* opCtx); - bool getAutocommit() const { return _autocommit; } @@ -405,23 +384,16 @@ public: const repl::OplogEntry& entry); private: - // Holds function to be used to kill client cursors. - static CursorKillFunction _cursorKillFunction; // Holds function which determines whether the CursorManager has client cursor references for a // given transaction. static CursorExistsFunction _cursorExistsFunction; void _beginOrContinueTxn(WithLock, - OperationContext* opCtx, TxnNumber txnNumber, boost::optional<bool> autocommit, - boost::optional<bool> startTransaction, - bool* canKillCursors); + boost::optional<bool> startTransaction); - void _beginOrContinueTxnOnMigration(WithLock, - OperationContext* opCtx, - TxnNumber txnNumber, - bool* canKillCursors); + void _beginOrContinueTxnOnMigration(WithLock, TxnNumber txnNumber); // Checks if there is a conflicting operation on the current Session void _checkValid(WithLock) const; @@ -430,10 +402,7 @@ private: // we don't start a txn that is too old. void _checkTxnValid(WithLock, TxnNumber txnNumber) const; - void _setActiveTxn(WithLock, - OperationContext* opCtx, - TxnNumber txnNumber, - bool* canKillCursors); + void _setActiveTxn(WithLock, TxnNumber txnNumber); void _checkIsActiveTransaction(WithLock, TxnNumber txnNumber, bool checkAbort) const; @@ -457,12 +426,10 @@ private: std::vector<StmtId> stmtIdsWritten, const repl::OpTime& lastStmtIdWriteTs); - void _abortArbitraryTransaction(WithLock, OperationContext* opCtx, bool* canKillCursors); + void _abortArbitraryTransaction(WithLock); // Releases stashed transaction resources to abort the transaction. - // 'canKillCursors' is an output parameter, which when set to true indicates that transaction - // client cursors may be killed. - void _abortTransaction(WithLock, OperationContext* opCtx, bool* canKillCursors); + void _abortTransaction(WithLock); // Committing a transaction first changes its state to "Committing" and writes to the oplog, // then it changes the state to "Committed". @@ -478,10 +445,6 @@ private: // 3) Migration. Should be able to skip committing transactions. void _commitTransaction(stdx::unique_lock<stdx::mutex> lk, OperationContext* opCtx); - void _killTransactionCursors(OperationContext* opCtx, - LogicalSessionId lsid, - TxnNumber txnNumber); - const LogicalSessionId _sessionId; // Protects the member variables below. diff --git a/src/mongo/db/session_test.cpp b/src/mongo/db/session_test.cpp index a85e37899ce..a7d64b0c52c 100644 --- a/src/mongo/db/session_test.cpp +++ b/src/mongo/db/session_test.cpp @@ -41,7 +41,6 @@ #include "mongo/db/stats/fill_locker_info.h" #include "mongo/stdx/future.h" #include "mongo/stdx/memory.h" -#include "mongo/unittest/death_test.h" #include "mongo/unittest/unittest.h" #include "mongo/util/net/socket_utils.h" @@ -50,7 +49,6 @@ namespace { const NamespaceString kNss("TestDB", "TestColl"); const OptionalCollectionUUID kUUID; -const bool kKillCursors = true; /** * Creates an OplogEntry with given parameters and preset defaults for this test suite. @@ -163,10 +161,6 @@ protected: } }; -size_t noopKillCursorFunction(OperationContext*, LogicalSessionId, TxnNumber) { - return 0; -}; - bool noopCursorExistsFunction(LogicalSessionId, TxnNumber) { return false; }; @@ -640,21 +634,6 @@ TEST_F(SessionTest, TransactionThrowsLockTimeoutIfLockIsUnavailable) { Client::setCurrent(std::move(clientWithDatabaseXLock)); } -DEATH_TEST_F(SessionTest, CommitWithoutCursorKillFunctionInvariants, "_cursorKillFunction") { - Session::registerCursorKillFunction(nullptr); - const auto sessionId = makeLogicalSessionIdForTest(); - Session session(sessionId); - session.refreshFromStorageIfNeeded(opCtx()); - - const TxnNumber txnNum = 26; - opCtx()->setLogicalSessionId(sessionId); - opCtx()->setTxnNumber(txnNum); - session.beginOrContinueTxn(opCtx(), txnNum, false, true, "testDB", "find"); - session.unstashTransactionResources(opCtx(), "find"); - - session.commitTransaction(opCtx()); -} - TEST_F(SessionTest, StashAndUnstashResources) { const auto sessionId = makeLogicalSessionIdForTest(); const TxnNumber txnNum = 20; @@ -666,7 +645,6 @@ TEST_F(SessionTest, StashAndUnstashResources) { ASSERT(originalLocker); ASSERT(originalRecoveryUnit); - Session::registerCursorKillFunction(noopKillCursorFunction); Session::registerCursorExistsFunction(noopCursorExistsFunction); Session session(sessionId); session.refreshFromStorageIfNeeded(opCtx()); @@ -712,7 +690,6 @@ TEST_F(SessionTest, StashAndUnstashResources) { } TEST_F(SessionTest, ReportStashedResources) { - Session::registerCursorKillFunction(noopKillCursorFunction); Session::registerCursorExistsFunction(noopCursorExistsFunction); const auto sessionId = makeLogicalSessionIdForTest(); const TxnNumber txnNum = 20; @@ -785,7 +762,6 @@ TEST_F(SessionTest, ReportStashedResources) { } TEST_F(SessionTest, CannotSpecifyStartTransactionOnInProgressTxn) { - Session::registerCursorKillFunction(noopKillCursorFunction); Session::registerCursorExistsFunction(noopCursorExistsFunction); const auto sessionId = makeLogicalSessionIdForTest(); Session session(sessionId); @@ -847,7 +823,6 @@ TEST_F(SessionTest, AutocommitRequiredOnEveryTxnOp) { } TEST_F(SessionTest, SameTransactionPreservesStoredStatements) { - Session::registerCursorKillFunction(noopKillCursorFunction); Session::registerCursorExistsFunction(noopCursorExistsFunction); const auto sessionId = makeLogicalSessionIdForTest(); Session session(sessionId); @@ -876,7 +851,6 @@ TEST_F(SessionTest, SameTransactionPreservesStoredStatements) { } TEST_F(SessionTest, AbortClearsStoredStatements) { - Session::registerCursorKillFunction(noopKillCursorFunction); Session::registerCursorExistsFunction(noopCursorExistsFunction); const auto sessionId = makeLogicalSessionIdForTest(); Session session(sessionId); @@ -893,7 +867,7 @@ TEST_F(SessionTest, AbortClearsStoredStatements) { // The transaction machinery cannot store an empty locker. { Lock::GlobalLock lk(opCtx(), MODE_IX, Date_t::now(), Lock::InterruptBehavior::kThrow); } session.stashTransactionResources(opCtx()); - session.abortArbitraryTransaction(opCtx(), kKillCursors); + session.abortArbitraryTransaction(); ASSERT_TRUE(session.transactionOperationsForTest().empty()); ASSERT_TRUE(session.transactionIsAborted()); } @@ -901,7 +875,6 @@ TEST_F(SessionTest, AbortClearsStoredStatements) { // This test makes sure the commit machinery works even when no operations are done on the // transaction. TEST_F(SessionTest, EmptyTransactionCommit) { - Session::registerCursorKillFunction(noopKillCursorFunction); Session::registerCursorExistsFunction(noopCursorExistsFunction); const auto sessionId = makeLogicalSessionIdForTest(); Session session(sessionId); @@ -922,7 +895,6 @@ TEST_F(SessionTest, EmptyTransactionCommit) { // This test makes sure the abort machinery works even when no operations are done on the // transaction. TEST_F(SessionTest, EmptyTransactionAbort) { - Session::registerCursorKillFunction(noopKillCursorFunction); Session::registerCursorExistsFunction(noopCursorExistsFunction); const auto sessionId = makeLogicalSessionIdForTest(); Session session(sessionId); @@ -936,12 +908,11 @@ TEST_F(SessionTest, EmptyTransactionAbort) { // The transaction machinery cannot store an empty locker. { Lock::GlobalLock lk(opCtx(), MODE_IX, Date_t::now(), Lock::InterruptBehavior::kThrow); } session.stashTransactionResources(opCtx()); - session.abortArbitraryTransaction(opCtx(), kKillCursors); + session.abortArbitraryTransaction(); ASSERT_TRUE(session.transactionIsAborted()); } TEST_F(SessionTest, ConcurrencyOfUnstashAndAbort) { - Session::registerCursorKillFunction(noopKillCursorFunction); Session::registerCursorExistsFunction(noopCursorExistsFunction); const auto sessionId = makeLogicalSessionIdForTest(); Session session(sessionId); @@ -953,7 +924,7 @@ TEST_F(SessionTest, ConcurrencyOfUnstashAndAbort) { session.beginOrContinueTxn(opCtx(), txnNum, false, true, "testDB", "find"); // The transaction may be aborted without checking out the session. - session.abortArbitraryTransaction(opCtx(), kKillCursors); + session.abortArbitraryTransaction(); // An unstash after an abort should uassert. ASSERT_THROWS_CODE(session.unstashTransactionResources(opCtx(), "find"), @@ -962,7 +933,6 @@ TEST_F(SessionTest, ConcurrencyOfUnstashAndAbort) { } TEST_F(SessionTest, ConcurrencyOfUnstashAndMigration) { - Session::registerCursorKillFunction(noopKillCursorFunction); Session::registerCursorExistsFunction(noopCursorExistsFunction); const auto sessionId = makeLogicalSessionIdForTest(); Session session(sessionId); @@ -991,7 +961,6 @@ TEST_F(SessionTest, ConcurrencyOfUnstashAndMigration) { } TEST_F(SessionTest, ConcurrencyOfStashAndAbort) { - Session::registerCursorKillFunction(noopKillCursorFunction); Session::registerCursorExistsFunction(noopCursorExistsFunction); const auto sessionId = makeLogicalSessionIdForTest(); Session session(sessionId); @@ -1005,14 +974,13 @@ TEST_F(SessionTest, ConcurrencyOfStashAndAbort) { session.unstashTransactionResources(opCtx(), "find"); // The transaction may be aborted without checking out the session. - session.abortArbitraryTransaction(opCtx(), kKillCursors); + session.abortArbitraryTransaction(); // A stash after an abort should be a noop. session.stashTransactionResources(opCtx()); } TEST_F(SessionTest, ConcurrencyOfStashAndMigration) { - Session::registerCursorKillFunction(noopKillCursorFunction); Session::registerCursorExistsFunction(noopCursorExistsFunction); const auto sessionId = makeLogicalSessionIdForTest(); Session session(sessionId); @@ -1038,7 +1006,6 @@ TEST_F(SessionTest, ConcurrencyOfStashAndMigration) { } TEST_F(SessionTest, ConcurrencyOfAddTransactionOperationAndAbort) { - Session::registerCursorKillFunction(noopKillCursorFunction); Session::registerCursorExistsFunction(noopCursorExistsFunction); const auto sessionId = makeLogicalSessionIdForTest(); Session session(sessionId); @@ -1052,7 +1019,7 @@ TEST_F(SessionTest, ConcurrencyOfAddTransactionOperationAndAbort) { session.unstashTransactionResources(opCtx(), "insert"); // The transaction may be aborted without checking out the session. - session.abortArbitraryTransaction(opCtx(), kKillCursors); + session.abortArbitraryTransaction(); // An addTransactionOperation() after an abort should uassert. auto operation = repl::OplogEntry::makeInsertOperation(kNss, kUUID, BSON("TestValue" << 0)); @@ -1062,7 +1029,6 @@ TEST_F(SessionTest, ConcurrencyOfAddTransactionOperationAndAbort) { } TEST_F(SessionTest, ConcurrencyOfAddTransactionOperationAndMigration) { - Session::registerCursorKillFunction(noopKillCursorFunction); Session::registerCursorExistsFunction(noopCursorExistsFunction); const auto sessionId = makeLogicalSessionIdForTest(); Session session(sessionId); @@ -1089,7 +1055,6 @@ TEST_F(SessionTest, ConcurrencyOfAddTransactionOperationAndMigration) { } TEST_F(SessionTest, ConcurrencyOfEndTransactionAndRetrieveOperationsAndAbort) { - Session::registerCursorKillFunction(noopKillCursorFunction); Session::registerCursorExistsFunction(noopCursorExistsFunction); const auto sessionId = makeLogicalSessionIdForTest(); Session session(sessionId); @@ -1103,7 +1068,7 @@ TEST_F(SessionTest, ConcurrencyOfEndTransactionAndRetrieveOperationsAndAbort) { session.unstashTransactionResources(opCtx(), "insert"); // The transaction may be aborted without checking out the session. - session.abortArbitraryTransaction(opCtx(), kKillCursors); + session.abortArbitraryTransaction(); // An endTransactionAndRetrieveOperations() after an abort should uassert. ASSERT_THROWS_CODE(session.endTransactionAndRetrieveOperations(opCtx()), @@ -1112,7 +1077,6 @@ TEST_F(SessionTest, ConcurrencyOfEndTransactionAndRetrieveOperationsAndAbort) { } TEST_F(SessionTest, ConcurrencyOfEndTransactionAndRetrieveOperationsAndMigration) { - Session::registerCursorKillFunction(noopKillCursorFunction); Session::registerCursorExistsFunction(noopCursorExistsFunction); const auto sessionId = makeLogicalSessionIdForTest(); Session session(sessionId); @@ -1139,7 +1103,6 @@ TEST_F(SessionTest, ConcurrencyOfEndTransactionAndRetrieveOperationsAndMigration } TEST_F(SessionTest, ConcurrencyOfCommitTransactionAndAbort) { - Session::registerCursorKillFunction(noopKillCursorFunction); Session::registerCursorExistsFunction(noopCursorExistsFunction); const auto sessionId = makeLogicalSessionIdForTest(); Session session(sessionId); @@ -1153,7 +1116,7 @@ TEST_F(SessionTest, ConcurrencyOfCommitTransactionAndAbort) { session.unstashTransactionResources(opCtx(), "commitTransaction"); // The transaction may be aborted without checking out the session. - session.abortArbitraryTransaction(opCtx(), kKillCursors); + session.abortArbitraryTransaction(); // An commitTransaction() after an abort should uassert. ASSERT_THROWS_CODE( @@ -1161,7 +1124,6 @@ TEST_F(SessionTest, ConcurrencyOfCommitTransactionAndAbort) { } TEST_F(SessionTest, ConcurrencyOfCommitTransactionAndMigration) { - Session::registerCursorKillFunction(noopKillCursorFunction); Session::registerCursorExistsFunction(noopCursorExistsFunction); const auto sessionId = makeLogicalSessionIdForTest(); Session session(sessionId); diff --git a/src/mongo/dbtests/cursor_manager_test.cpp b/src/mongo/dbtests/cursor_manager_test.cpp index 48271d24758..ee3933dfa35 100644 --- a/src/mongo/dbtests/cursor_manager_test.cpp +++ b/src/mongo/dbtests/cursor_manager_test.cpp @@ -541,7 +541,7 @@ TEST_F(CursorManagerTestCustomOpCtx, LogicalSessionIdOnOperationCtxTest) { // Cursors created on an op ctx with a session id have a session id. { auto lsid = makeLogicalSessionIdForTest(); - auto opCtx2 = _queryServiceContext->makeOperationContext(lsid, boost::none); + auto opCtx2 = _queryServiceContext->makeOperationContext(lsid); auto pinned2 = makeCursor(opCtx2.get()); ASSERT_EQUALS(pinned2.getCursor()->getSessionId(), lsid); @@ -569,7 +569,7 @@ TEST_F(CursorManagerTestCustomOpCtx, CursorsWithoutSessions) { TEST_F(CursorManagerTestCustomOpCtx, OneCursorWithASession) { // Add a cursor with a session to the cursor manager. auto lsid = makeLogicalSessionIdForTest(); - auto opCtx = _queryServiceContext->makeOperationContext(lsid, boost::none); + auto opCtx = _queryServiceContext->makeOperationContext(lsid); auto pinned = makeCursor(opCtx.get()); // Retrieve all sessions active in manager - set should contain just lsid. @@ -595,145 +595,13 @@ TEST_F(CursorManagerTestCustomOpCtx, OneCursorWithASession) { ASSERT(useCursorManager()->getCursorsForSession(lsid).empty()); } -TEST_F(CursorManagerTestCustomOpCtx, KillCursorRespectsSessionId) { - // Add a cursor with a session to the cursor manager. - auto lsid = makeLogicalSessionIdForTest(); - auto opCtx = _queryServiceContext->makeOperationContext(lsid, boost::none); - auto pinned = makeCursor(opCtx.get()); - auto cursorId = pinned.getCursor()->cursorid(); - - // Killing the cursor with incorrect LogicalSessionId fails. - pinned.release(); - auto wrongLsid = makeLogicalSessionIdForTest(); - auto status = - useCursorManager()->killCursor(opCtx.get(), cursorId, false, wrongLsid, boost::none); - ASSERT_NOT_OK(status); - ASSERT_EQ(status.code(), ErrorCodes::CursorNotFound); - - // Killing the cursor with the correct LogicalSessionId works. - ASSERT_OK(useCursorManager()->killCursor(opCtx.get(), cursorId, false, lsid, boost::none)); -} - -TEST_F(CursorManagerTestCustomOpCtx, - KillCursorWithSessionDoesNotKillCursorCreatedOutsideOfSession) { - // Add a cursor with a session to the cursor manager. - auto opCtx = _queryServiceContext->makeOperationContext(); - auto pinned = makeCursor(opCtx.get()); - auto cursorId = pinned.getCursor()->cursorid(); - - // Killing the cursor with the correct cursorId but with an unrelated LogicalSessionId fails. - auto lsid = makeLogicalSessionIdForTest(); - auto status = useCursorManager()->killCursor(opCtx.get(), cursorId, false, lsid, boost::none); - ASSERT_NOT_OK(status); - ASSERT_EQ(status.code(), ErrorCodes::CursorNotFound); -} - -TEST_F(CursorManagerTestCustomOpCtx, KillCursorRespectsTxnNumber) { - // Add a cursor with a session to the cursor manager. - auto lsid = makeLogicalSessionIdForTest(); - const TxnNumber txnNumber = 0; - auto opCtx = _queryServiceContext->makeOperationContext(lsid, txnNumber); - auto pinned = makeCursor(opCtx.get()); - auto cursorId = pinned.getCursor()->cursorid(); - - // Killing the cursor with incorrect TxnNumber fails. - pinned.release(); - const TxnNumber wrongTxnNumber = 1; - auto status = - useCursorManager()->killCursor(opCtx.get(), cursorId, false, lsid, wrongTxnNumber); - ASSERT_NOT_OK(status); - ASSERT_EQ(status.code(), ErrorCodes::CursorNotFound); - - // Kill the cursor with the correct TxnNumber works. - ASSERT_OK(useCursorManager()->killCursor(opCtx.get(), cursorId, false, lsid, txnNumber)); -} - -TEST_F(CursorManagerTestCustomOpCtx, - KillAllCursorsForTransactionRemovesCorrectEntryFromTransactionMap) { - CursorManager* cursorManager = CursorManager::getGlobalCursorManager(); - - // Create 3 sets of cursors, each with a unique LogicalSessionId/TxnNumber pair, but each - // sharing either LogicalSessionId or TxnNumber with another set. - auto lsid1 = makeLogicalSessionIdForTest(); - TxnNumber txnNumber1 = 0; - { - auto opCtx = _queryServiceContext->makeOperationContext(lsid1, txnNumber1); - auto pinned = cursorManager->registerCursor(opCtx.get(), - {makeFakePlanExecutor(), - NamespaceString{"test.collection"}, - {}, - repl::ReadConcernLevel::kLocalReadConcern, - BSONObj()}); - pinned.release(); - } - - auto lsid2 = lsid1; - TxnNumber txnNumber2 = 1; - { - auto opCtx = _queryServiceContext->makeOperationContext(lsid2, txnNumber2); - auto pinned = cursorManager->registerCursor(opCtx.get(), - {makeFakePlanExecutor(), - NamespaceString{"test.collection"}, - {}, - repl::ReadConcernLevel::kLocalReadConcern, - BSONObj()}); - pinned.release(); - } - - auto lsid3 = makeLogicalSessionIdForTest(); - TxnNumber txnNumber3 = txnNumber1; - { - auto opCtx = _queryServiceContext->makeOperationContext(lsid3, txnNumber3); - // Create 2 cursors for the third set to confirm multiple cursor deregistration. - auto pinned = cursorManager->registerCursor(opCtx.get(), - {makeFakePlanExecutor(), - NamespaceString{"test.collection"}, - {}, - repl::ReadConcernLevel::kLocalReadConcern, - BSONObj()}); - pinned.release(); - pinned = cursorManager->registerCursor(opCtx.get(), - {makeFakePlanExecutor(), - NamespaceString{"test.collection"}, - {}, - repl::ReadConcernLevel::kLocalReadConcern, - BSONObj()}); - pinned.release(); - } - - auto opCtx = _queryServiceContext->makeOperationContext(); - - // Transaction reference exists for all 3 sets. - ASSERT_TRUE(cursorManager->hasTransactionCursorReference(lsid1, txnNumber1)); - ASSERT_TRUE(cursorManager->hasTransactionCursorReference(lsid2, txnNumber2)); - ASSERT_TRUE(cursorManager->hasTransactionCursorReference(lsid3, txnNumber3)); - - // Transaction reference does not exist for LogicalSessionId/TxnNumber that has no cursors. - ASSERT_FALSE(cursorManager->hasTransactionCursorReference(makeLogicalSessionIdForTest(), 99)); - - // Kill cursors for set 1. - ASSERT_EQ(1ul, cursorManager->killAllCursorsForTransaction(opCtx.get(), lsid1, txnNumber1)); - ASSERT_FALSE(cursorManager->hasTransactionCursorReference(lsid1, txnNumber1)); - ASSERT_TRUE(cursorManager->hasTransactionCursorReference(lsid2, txnNumber2)); - ASSERT_TRUE(cursorManager->hasTransactionCursorReference(lsid3, txnNumber3)); - - // Kill cursors for set 2. - ASSERT_EQ(1ul, cursorManager->killAllCursorsForTransaction(opCtx.get(), lsid2, txnNumber2)); - ASSERT_FALSE(cursorManager->hasTransactionCursorReference(lsid2, txnNumber2)); - ASSERT_TRUE(cursorManager->hasTransactionCursorReference(lsid3, txnNumber3)); - - // Kill cursors for set 3. - ASSERT_EQ(2ul, cursorManager->killAllCursorsForTransaction(opCtx.get(), lsid3, txnNumber3)); - ASSERT_FALSE(cursorManager->hasTransactionCursorReference(lsid3, txnNumber3)); -} - /** * Test a manager with multiple cursors running inside of the same session. */ TEST_F(CursorManagerTestCustomOpCtx, MultipleCursorsWithSameSession) { // Add two cursors on the same session to the cursor manager. auto lsid = makeLogicalSessionIdForTest(); - auto opCtx = _queryServiceContext->makeOperationContext(lsid, boost::none); + auto opCtx = _queryServiceContext->makeOperationContext(lsid); auto pinned = makeCursor(opCtx.get()); auto pinned2 = makeCursor(opCtx.get()); @@ -780,13 +648,13 @@ TEST_F(CursorManagerTestCustomOpCtx, MultipleCursorsMultipleSessions) { // Cursor with session 1. { - auto opCtx1 = _queryServiceContext->makeOperationContext(lsid1, boost::none); + auto opCtx1 = _queryServiceContext->makeOperationContext(lsid1); cursor1 = makeCursor(opCtx1.get()).getCursor()->cursorid(); } // Cursor with session 2. { - auto opCtx2 = _queryServiceContext->makeOperationContext(lsid2, boost::none); + auto opCtx2 = _queryServiceContext->makeOperationContext(lsid2); cursor2 = makeCursor(opCtx2.get()).getCursor()->cursorid(); } |