From 2e31167d0e2cf179a228afeec71853cfb0f0683f Mon Sep 17 00:00:00 2001 From: Siyuan Zhou Date: Wed, 28 Mar 2018 23:41:12 -0400 Subject: [PATCH] SERVER-33501 Aborted transaction number cannot be reused. --- .../txns/multi_statement_transaction_abort.js | 51 ++++++++++++++++++- .../no_implicit_collection_creation_in_txn.js | 6 +-- .../libs/global_snapshot_reads_helpers.js | 8 +-- src/mongo/db/session.cpp | 35 ++++++------- src/mongo/db/session.h | 2 +- src/mongo/db/session_test.cpp | 2 +- 6 files changed, 75 insertions(+), 29 deletions(-) diff --git a/jstests/core/txns/multi_statement_transaction_abort.js b/jstests/core/txns/multi_statement_transaction_abort.js index c1839a3970c..01ef761aa6b 100644 --- a/jstests/core/txns/multi_statement_transaction_abort.js +++ b/jstests/core/txns/multi_statement_transaction_abort.js @@ -117,7 +117,7 @@ txnNumber: NumberLong(txnNumber), autocommit: false }), - ErrorCodes.TransactionAborted); + ErrorCodes.NoSuchTransaction); // Verify the documents are the same. assert.eq({_id: "insert-1"}, testColl.findOne({_id: "insert-1"})); assert.eq(null, testColl.findOne({_id: "insert-2"})); @@ -248,5 +248,54 @@ // TODO: SERVER-33690 Test the old cursor has been killed when the transaction is aborted. + jsTest.log("Aborted transaction number cannot be reused."); + txnNumber++; + assert.commandWorked(sessionDb.runCommand({ + insert: collName, + documents: [{_id: "abort-txn-1"}], + readConcern: {level: "snapshot"}, + txnNumber: NumberLong(txnNumber), + startTransaction: true, + autocommit: false + })); + assert.commandWorked(sessionDb.adminCommand({ + abortTransaction: 1, + writeConcern: {w: "majority"}, + txnNumber: NumberLong(txnNumber), + autocommit: false + })); + + // Cannot commit the aborted transaction. + assert.commandFailedWithCode( + sessionDb.adminCommand( + {commitTransaction: 1, txnNumber: NumberLong(txnNumber), autocommit: false}), + ErrorCodes.NoSuchTransaction); + + // Cannot abort the aborted transaction. + assert.commandFailedWithCode( + sessionDb.adminCommand( + {abortTransaction: 1, txnNumber: NumberLong(txnNumber), autocommit: false}), + ErrorCodes.NoSuchTransaction); + + // Cannot continue the aborted transaction. + assert.commandFailedWithCode(sessionDb.runCommand({ + insert: collName, + documents: [{_id: "abort-txn-2"}], + txnNumber: NumberLong(txnNumber), + autocommit: false + }), + ErrorCodes.NoSuchTransaction); + + // Cannot restart the aborted transaction. + assert.commandFailedWithCode(sessionDb.runCommand({ + insert: collName, + documents: [{_id: "abort-txn-2"}], + readConcern: {level: "snapshot"}, + txnNumber: NumberLong(txnNumber), + startTransaction: true, + autocommit: false + }), + ErrorCodes.ConflictingOperationInProgress); + session.endSession(); }()); diff --git a/jstests/core/txns/no_implicit_collection_creation_in_txn.js b/jstests/core/txns/no_implicit_collection_creation_in_txn.js index 4b4fc5bda25..17496f2eff3 100644 --- a/jstests/core/txns/no_implicit_collection_creation_in_txn.js +++ b/jstests/core/txns/no_implicit_collection_creation_in_txn.js @@ -50,7 +50,7 @@ assert.commandFailedWithCode( sessionDb.adminCommand( {commitTransaction: 1, txnNumber: NumberLong(txnNumber), autocommit: false}), - ErrorCodes.TransactionAborted); + ErrorCodes.NoSuchTransaction); assert.eq(null, testColl.findOne({_id: "doc"})); jsTest.log("Cannot implicitly create a collection in a transaction using update."); @@ -86,7 +86,7 @@ assert.commandFailedWithCode( sessionDb.adminCommand( {commitTransaction: 1, txnNumber: NumberLong(txnNumber), autocommit: false}), - ErrorCodes.TransactionAborted); + ErrorCodes.NoSuchTransaction); assert.eq(null, testColl.findOne({_id: "doc"})); // Update without upsert=true succeeds when the collection does not exist. @@ -140,7 +140,7 @@ assert.commandFailedWithCode( sessionDb.adminCommand( {commitTransaction: 1, txnNumber: NumberLong(txnNumber), autocommit: false}), - ErrorCodes.TransactionAborted); + ErrorCodes.NoSuchTransaction); assert.eq(null, testColl.findOne({_id: "doc"})); // findAndModify without upsert=true succeeds when the collection does not exist. diff --git a/jstests/multiVersion/libs/global_snapshot_reads_helpers.js b/jstests/multiVersion/libs/global_snapshot_reads_helpers.js index 32bdfcb5129..4e5ccdbac6b 100644 --- a/jstests/multiVersion/libs/global_snapshot_reads_helpers.js +++ b/jstests/multiVersion/libs/global_snapshot_reads_helpers.js @@ -22,16 +22,16 @@ function runCommandAndVerifyResponse(sessionDb, txnNumber, cmdObj, expectSuccess // noop writes may advance the majority commit point past the given atClusterTime // resulting in a SnapshotTooOld error. Eventually the read should succeed, when all // targeted shards are at the same cluster time, so retry until it does. - // A snapshot read may also fail with TransactionAborted if it encountered a StaleEpoch + // A snapshot read may also fail with NoSuchTransaction if it encountered a StaleEpoch // error while it was running. assert.soon(() => { const res = sessionDb.runCommand(cmdObj); if (!res.ok) { assert(res.code === ErrorCodes.SnapshotTooOld || - res.code === ErrorCodes.TransactionAborted, - "expected command to fail with SnapshotTooOld or TransactionAborted, cmd: " + + res.code === ErrorCodes.NoSuchTransaction, + "expected command to fail with SnapshotTooOld or NoSuchTransaction, cmd: " + tojson(cmdObj) + ", result: " + tojson(res)); - print("Retrying because of SnapshotTooOld or TransactionAborted error."); + print("Retrying because of SnapshotTooOld or NoSuchTransaction error."); txnNumber++; cmdObj.txnNumber = NumberLong(txnNumber); return false; diff --git a/src/mongo/db/session.cpp b/src/mongo/db/session.cpp index 0f600d613da..30bb3dfd51c 100644 --- a/src/mongo/db/session.cpp +++ b/src/mongo/db/session.cpp @@ -349,7 +349,7 @@ void Session::onMigrateCompletedOnPrimary(OperationContext* opCtx, stdx::unique_lock ul(_mutex); _checkValid(ul); - _checkIsActiveTransaction(ul, txnNumber); + _checkIsActiveTransaction(ul, txnNumber, false); const auto updateRequest = _makeUpdateRequest(ul, txnNumber, lastStmtIdWriteOpTime, lastStmtIdWriteDate); @@ -378,7 +378,7 @@ void Session::invalidate() { repl::OpTime Session::getLastWriteOpTime(TxnNumber txnNumber) const { stdx::lock_guard lg(_mutex); _checkValid(lg); - _checkIsActiveTransaction(lg, txnNumber); + _checkIsActiveTransaction(lg, txnNumber, false); if (!_lastWrittenSessionRecord || _lastWrittenSessionRecord->getTxnNum() != txnNumber) return {}; @@ -578,7 +578,7 @@ void Session::stashTransactionResources(OperationContext* opCtx) { // Always check '_activeTxnNumber', since it can be modified by migration, which does not check // out the session. We intentionally do not error if _txnState=kAborted, since we expect this // function to be called at the end of the 'abortTransaction' command. - _checkIsActiveTransaction(lg, *opCtx->getTxnNumber()); + _checkIsActiveTransaction(lg, *opCtx->getTxnNumber(), false); if (_txnState != MultiDocumentTransactionState::kInProgress && _txnState != MultiDocumentTransactionState::kInSnapshotRead) { @@ -621,8 +621,10 @@ void Session::unstashTransactionResources(OperationContext* opCtx) { // Always check '_activeTxnNumber' and '_txnState', since they can be modified by session // kill and migration, which do not check out the session. - _checkIsActiveTransaction(lg, *opCtx->getTxnNumber()); - uassert(ErrorCodes::TransactionAborted, + _checkIsActiveTransaction(lg, *opCtx->getTxnNumber(), false); + // Throw NoSuchTransaction error instead of TransactionAborted error since this is the entry + // point of transaction execution. + uassert(ErrorCodes::NoSuchTransaction, str::stream() << "Transaction " << *opCtx->getTxnNumber() << " has been aborted.", _txnState != MultiDocumentTransactionState::kAborted); @@ -728,10 +730,7 @@ void Session::addTransactionOperation(OperationContext* opCtx, // 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()); - uassert(ErrorCodes::TransactionAborted, - str::stream() << "Transaction " << *opCtx->getTxnNumber() << " has been aborted.", - _txnState != MultiDocumentTransactionState::kAborted); + _checkIsActiveTransaction(lk, *opCtx->getTxnNumber(), true); invariant(_txnState == MultiDocumentTransactionState::kInProgress); invariant(!_autocommit && _activeTxnNumber != kUninitializedTxnNumber); @@ -745,10 +744,7 @@ std::vector Session::endTransactionAndRetrieveOperations( // 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()); - uassert(ErrorCodes::TransactionAborted, - str::stream() << "Transaction " << *opCtx->getTxnNumber() << " has been aborted.", - _txnState != MultiDocumentTransactionState::kAborted); + _checkIsActiveTransaction(lk, *opCtx->getTxnNumber(), true); invariant(!_autocommit); return std::move(_transactionOperations); @@ -759,10 +755,7 @@ void Session::commitTransaction(OperationContext* opCtx) { // 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()); - uassert(ErrorCodes::TransactionAborted, - str::stream() << "Transaction " << *opCtx->getTxnNumber() << " has been aborted.", - _txnState != MultiDocumentTransactionState::kAborted); + _checkIsActiveTransaction(lk, *opCtx->getTxnNumber(), true); if (_txnState == MultiDocumentTransactionState::kCommitted) return; @@ -819,7 +812,7 @@ void Session::_checkValid(WithLock) const { _isValid); } -void Session::_checkIsActiveTransaction(WithLock, TxnNumber txnNumber) const { +void Session::_checkIsActiveTransaction(WithLock, TxnNumber txnNumber, bool checkAbort) const { uassert(ErrorCodes::ConflictingOperationInProgress, str::stream() << "Cannot perform operations on transaction " << txnNumber << " on session " @@ -828,13 +821,17 @@ void Session::_checkIsActiveTransaction(WithLock, TxnNumber txnNumber) const { << _activeTxnNumber << " is now active.", txnNumber == _activeTxnNumber); + + uassert(ErrorCodes::TransactionAborted, + str::stream() << "Transaction " << txnNumber << " has been aborted.", + !checkAbort || _txnState != MultiDocumentTransactionState::kAborted); } boost::optional Session::_checkStatementExecuted(WithLock wl, TxnNumber txnNumber, StmtId stmtId) const { _checkValid(wl); - _checkIsActiveTransaction(wl, txnNumber); + _checkIsActiveTransaction(wl, txnNumber, false); // Retries are not detected for multi-document transactions. if (_txnState == MultiDocumentTransactionState::kInProgress) return boost::none; diff --git a/src/mongo/db/session.h b/src/mongo/db/session.h index 486ff569053..95e48ddc7a6 100644 --- a/src/mongo/db/session.h +++ b/src/mongo/db/session.h @@ -334,7 +334,7 @@ private: void _setActiveTxn(WithLock, TxnNumber txnNumber); - void _checkIsActiveTransaction(WithLock, TxnNumber txnNumber) const; + void _checkIsActiveTransaction(WithLock, TxnNumber txnNumber, bool checkAbort) const; boost::optional _checkStatementExecuted(WithLock, TxnNumber txnNumber, diff --git a/src/mongo/db/session_test.cpp b/src/mongo/db/session_test.cpp index 6e00de8159f..89b4cd773b3 100644 --- a/src/mongo/db/session_test.cpp +++ b/src/mongo/db/session_test.cpp @@ -792,7 +792,7 @@ TEST_F(SessionTest, ConcurrencyOfUnstashAndAbort) { // An unstash after an abort should uassert. ASSERT_THROWS_CODE(session.unstashTransactionResources(opCtx()), AssertionException, - ErrorCodes::TransactionAborted); + ErrorCodes::NoSuchTransaction); } TEST_F(SessionTest, ConcurrencyOfUnstashAndMigration) {