From 4532c96449378da03e9e9f3c56bc42cf0b69b092 Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Mon, 26 Apr 2021 14:06:40 -0700 Subject: [PATCH] [Release 2.1] Fix | Fix race condition issues between SinglePhaseCommit and TransactionEnded events (#1042) (#1049) --- .../Data/SqlClient/SqlDelegatedTransaction.cs | 81 +++++++++---------- .../Data/SqlClient/SqlDelegatedTransaction.cs | 81 +++++++++---------- 2 files changed, 78 insertions(+), 84 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlDelegatedTransaction.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlDelegatedTransaction.cs index d806dc814a..fe63845be9 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlDelegatedTransaction.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlDelegatedTransaction.cs @@ -321,24 +321,21 @@ public void SinglePhaseCommit(SinglePhaseEnlistment enlistment) RuntimeHelpers.PrepareConstrainedRegions(); try { - // If the connection is doomed, we can be certain that the - // transaction will eventually be rolled back, and we shouldn't - // attempt to commit it. - if (connection.IsConnectionDoomed) + lock (connection) { - lock (connection) + // If the connection is doomed, we can be certain that the + // transaction will eventually be rolled back or has already been aborted externally, and we shouldn't + // attempt to commit it. + if (connection.IsConnectionDoomed) { _active = false; // set to inactive first, doesn't matter how the rest completes, this transaction is done. _connection = null; - } - enlistment.Aborted(SQL.ConnectionDoomed()); - } - else - { - Exception commitException; - lock (connection) + enlistment.Aborted(SQL.ConnectionDoomed()); + } + else { + Exception commitException; try { // Now that we've acquired the lock, make sure we still have valid state for this operation. @@ -364,40 +361,40 @@ public void SinglePhaseCommit(SinglePhaseEnlistment enlistment) commitException = e; connection.DoomThisConnection(); } - } - if (commitException != null) - { - // connection.ExecuteTransaction failed with exception - if (_internalTransaction.IsCommitted) - { - // Even though we got an exception, the transaction - // was committed by the server. - enlistment.Committed(); - } - else if (_internalTransaction.IsAborted) + if (commitException != null) { - // The transaction was aborted, report that to - // SysTx. - enlistment.Aborted(commitException); + // connection.ExecuteTransaction failed with exception + if (_internalTransaction.IsCommitted) + { + // Even though we got an exception, the transaction + // was committed by the server. + enlistment.Committed(); + } + else if (_internalTransaction.IsAborted) + { + // The transaction was aborted, report that to + // SysTx. + enlistment.Aborted(commitException); + } + else + { + // The transaction is still active, we cannot + // know the state of the transaction. + enlistment.InDoubt(commitException); + } + + // We eat the exception. This is called on the SysTx + // thread, not the applications thread. If we don't + // eat the exception an UnhandledException will occur, + // causing the process to FailFast. } - else + + connection.CleanupConnectionOnTransactionCompletion(_atomicTransaction); + if (commitException == null) { - // The transaction is still active, we cannot - // know the state of the transaction. - enlistment.InDoubt(commitException); + // connection.ExecuteTransaction succeeded + enlistment.Committed(); } - - // We eat the exception. This is called on the SysTx - // thread, not the applications thread. If we don't - // eat the exception an UnhandledException will occur, - // causing the process to FailFast. - } - - connection.CleanupConnectionOnTransactionCompletion(_atomicTransaction); - if (commitException == null) - { - // connection.ExecuteTransaction succeeded - enlistment.Committed(); } } } diff --git a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlDelegatedTransaction.cs b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlDelegatedTransaction.cs index 082de06d3f..7bf191e837 100644 --- a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlDelegatedTransaction.cs +++ b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlDelegatedTransaction.cs @@ -391,24 +391,21 @@ public void SinglePhaseCommit(SysTx.SinglePhaseEnlistment enlistment) #else { #endif //DEBUG - // If the connection is doomed, we can be certain that the - // transaction will eventually be rolled back, and we shouldn't - // attempt to commit it. - if (connection.IsConnectionDoomed) + lock (connection) { - lock (connection) + // If the connection is doomed, we can be certain that the + // transaction will eventually be rolled back or has already been aborted externally, and we shouldn't + // attempt to commit it. + if (connection.IsConnectionDoomed) { _active = false; // set to inactive first, doesn't matter how the rest completes, this transaction is done. _connection = null; - } - enlistment.Aborted(SQL.ConnectionDoomed()); - } - else - { - Exception commitException; - lock (connection) + enlistment.Aborted(SQL.ConnectionDoomed()); + } + else { + Exception commitException; try { // Now that we've acquired the lock, make sure we still have valid state for this operation. @@ -437,40 +434,40 @@ public void SinglePhaseCommit(SysTx.SinglePhaseEnlistment enlistment) ADP.TraceExceptionWithoutRethrow(e); connection.DoomThisConnection(); } - } - if (commitException != null) - { - // connection.ExecuteTransaction failed with exception - if (_internalTransaction.IsCommitted) - { - // Even though we got an exception, the transaction - // was committed by the server. - enlistment.Committed(); - } - else if (_internalTransaction.IsAborted) + if (commitException != null) { - // The transaction was aborted, report that to - // SysTx. - enlistment.Aborted(commitException); + // connection.ExecuteTransaction failed with exception + if (_internalTransaction.IsCommitted) + { + // Even though we got an exception, the transaction + // was committed by the server. + enlistment.Committed(); + } + else if (_internalTransaction.IsAborted) + { + // The transaction was aborted, report that to + // SysTx. + enlistment.Aborted(commitException); + } + else + { + // The transaction is still active, we cannot + // know the state of the transaction. + enlistment.InDoubt(commitException); + } + + // We eat the exception. This is called on the SysTx + // thread, not the applications thread. If we don't + // eat the exception an UnhandledException will occur, + // causing the process to FailFast. } - else + + connection.CleanupConnectionOnTransactionCompletion(_atomicTransaction); + if (commitException == null) { - // The transaction is still active, we cannot - // know the state of the transaction. - enlistment.InDoubt(commitException); + // connection.ExecuteTransaction succeeded + enlistment.Committed(); } - - // We eat the exception. This is called on the SysTx - // thread, not the applications thread. If we don't - // eat the exception an UnhandledException will occur, - // causing the process to FailFast. - } - - connection.CleanupConnectionOnTransactionCompletion(_atomicTransaction); - if (commitException == null) - { - // connection.ExecuteTransaction succeeded - enlistment.Committed(); } } }