SERVER-42987 make it safe to interrupt abortTransaction

3 files changed, 54 insertions, 20 deletions

diff --git a/src/mongo/db/transaction_participant.cpp b/src/mongo/db/transaction_participant.cpp
index 26ab885031a..8e6e9531ac0 100644
--- a/src/mongo/db/transaction_participant.cpp
+++ b/src/mongo/db/transaction_participant.cpp
@@ -1473,27 +1473,56 @@ void TransactionParticipant::Participant::_abortActiveTransaction(
         o(lk).transactionMetricsObserver.onTransactionOperation(
             opCtx, CurOp::get(opCtx)->debug().additiveMetrics, o().txnState.isPrepared());
     }
-    // We reserve an oplog slot before aborting the transaction so that no writes that are causally
-    // related to the transaction abort enter the oplog at a timestamp earlier than the abort oplog
-    // entry. On secondaries, we generate a fake empty oplog slot, since it's not used by the
-    // OpObserver.
-    boost::optional<OplogSlotReserver> oplogSlotReserver;
-    boost::optional<OplogSlot> abortOplogSlot;
-    if (opCtx->writesAreReplicated() && p().needToWriteAbortEntry) {
-        oplogSlotReserver.emplace(opCtx);
-        abortOplogSlot = oplogSlotReserver->getLastSlot();
-    }
-
-    // Clean up the transaction resources on the opCtx even if the transaction resources on the
-    // session were not aborted. This actually aborts the storage-transaction.
-    _cleanUpTxnResourceOnOpCtx(opCtx, TerminationCause::kAborted);
 
-    // Write the abort oplog entry. This must be done after aborting the storage transaction, so
-    // that the lock state is reset, and there is no max lock timeout on the locker.
     auto opObserver = opCtx->getServiceContext()->getOpObserver();
     invariant(opObserver);
-    opObserver->onTransactionAbort(opCtx, abortOplogSlot);
 
+    const bool needToWriteAbortEntry = opCtx->writesAreReplicated() && p().needToWriteAbortEntry;
+    if (needToWriteAbortEntry) {
+        // We reserve an oplog slot before aborting the transaction so that no writes that are
+        // causally related to the transaction abort enter the oplog at a timestamp earlier than the
+        // abort oplog entry.
+        OplogSlotReserver oplogSlotReserver(opCtx);
+
+        // Clean up the transaction resources on the opCtx even if the transaction resources on the
+        // session were not aborted. This actually aborts the storage-transaction.
+        _cleanUpTxnResourceOnOpCtx(opCtx, TerminationCause::kAborted);
+
+        try {
+            // If we need to write an abort oplog entry, this function can no longer be interrupted.
+            UninterruptibleLockGuard noInterrupt(opCtx->lockState());
+
+            // Write the abort oplog entry. This must be done after aborting the storage
+            // transaction, so that the lock state is reset, and there is no max lock timeout on the
+            // locker.
+            opObserver->onTransactionAbort(opCtx, oplogSlotReserver.getLastSlot());
+
+            _finishAbortingActiveTransaction(opCtx, expectedStates);
+        } catch (...) {
+            // It is illegal for aborting a transaction that must write an abort oplog entry to fail
+            // after aborting the storage transaction, so we crash instead.
+            severe()
+                << "Caught exception during abort of transaction that must write abort oplog entry "
+                << opCtx->getTxnNumber() << " on " << _sessionId().toBSON() << ": "
+                << exceptionToStatus();
+            std::terminate();
+        }
+    } else {
+        // Clean up the transaction resources on the opCtx even if the transaction resources on the
+        // session were not aborted. This actually aborts the storage-transaction.
+        //
+        // These functions are allowed to throw. We are not writing an oplog entry, so the only risk
+        // is not cleaning up some internal TransactionParticipant state, updating metrics, or
+        // logging the end of the transaction. That will either be cleaned up in the
+        // ServiceEntryPoint's abortGuard or when the next transaction begins.
+        _cleanUpTxnResourceOnOpCtx(opCtx, TerminationCause::kAborted);
+        opObserver->onTransactionAbort(opCtx, boost::none);
+        _finishAbortingActiveTransaction(opCtx, expectedStates);
+    }
+}
+
+void TransactionParticipant::Participant::_finishAbortingActiveTransaction(
+    OperationContext* opCtx, TransactionState::StateSet expectedStates) {
     // Only abort the transaction in session if it's in expected states.
     // When the state of active transaction on session is not expected, it means another
     // thread has already aborted the transaction on session.
diff --git a/src/mongo/db/transaction_participant.h b/src/mongo/db/transaction_participant.h
index 370d110a134..ccad3a30085 100644
--- a/src/mongo/db/transaction_participant.h
+++ b/src/mongo/db/transaction_participant.h
@@ -658,6 +658,10 @@ public:
         void _abortActiveTransaction(OperationContext* opCtx,
                                      TransactionState::StateSet expectedStates);
 
+        // Factors out code for clarity from _abortActiveTransaction.
+        void _finishAbortingActiveTransaction(OperationContext* opCtx,
+                                              TransactionState::StateSet expectedStates);
+
         // Aborts a prepared transaction.
         void _abortActivePreparedTransaction(OperationContext* opCtx);
 
diff --git a/src/mongo/db/transaction_participant_test.cpp b/src/mongo/db/transaction_participant_test.cpp
index c5e58a96fbf..7bf35155984 100644
--- a/src/mongo/db/transaction_participant_test.cpp
+++ b/src/mongo/db/transaction_participant_test.cpp
@@ -1472,7 +1472,9 @@ TEST_F(TxnParticipantTest, ThrowDuringUnpreparedOnTransactionAbort) {
         txnParticipant.abortTransaction(opCtx()), AssertionException, ErrorCodes::OperationFailed);
 }
 
-TEST_F(TxnParticipantTest, ThrowDuringPreparedOnTransactionAbortIsFatal) {
+DEATH_TEST_F(TxnParticipantTest,
+             ThrowDuringPreparedOnTransactionAbortIsFatal,
+             "Caught exception during abort of transaction") {
     auto sessionCheckout = checkOutSession();
     auto txnParticipant = TransactionParticipant::get(opCtx());
     txnParticipant.unstashTransactionResources(opCtx(), "abortTransaction");
@@ -1480,8 +1482,7 @@ TEST_F(TxnParticipantTest, ThrowDuringPreparedOnTransactionAbortIsFatal) {
 
     _opObserver->onTransactionAbortThrowsException = true;
 
-    ASSERT_THROWS_CODE(
-        txnParticipant.abortTransaction(opCtx()), AssertionException, ErrorCodes::OperationFailed);
+    txnParticipant.abortTransaction(opCtx());
 }
 
 TEST_F(TxnParticipantTest, InterruptedSessionsCannotBePrepared) {