From b689674a1cbc675220ed4f3facbec544842c8ad3 Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Fri, 17 Sep 2021 16:03:13 -0700 Subject: [PATCH] [Release 3.0] Fix deadlock in transaction (.NET Framework) (#1243) --- .../Data/SqlClient/SqlDelegatedTransaction.cs | 80 ++++++++++--------- .../TransactionEnlistmentTest.cs | 35 ++++++++ 2 files changed, 76 insertions(+), 39 deletions(-) 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 7bf191e837..5b53034f80 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,21 +391,23 @@ public void SinglePhaseCommit(SysTx.SinglePhaseEnlistment enlistment) #else { #endif //DEBUG - lock (connection) + // 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) { - // 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) + lock (connection) { _active = false; // set to inactive first, doesn't matter how the rest completes, this transaction is done. _connection = null; - - enlistment.Aborted(SQL.ConnectionDoomed()); } - else + enlistment.Aborted(SQL.ConnectionDoomed()); + } + else + { + Exception commitException; + lock (connection) { - Exception commitException; try { // Now that we've acquired the lock, make sure we still have valid state for this operation. @@ -434,40 +436,40 @@ public void SinglePhaseCommit(SysTx.SinglePhaseEnlistment enlistment) ADP.TraceExceptionWithoutRethrow(e); connection.DoomThisConnection(); } - if (commitException != null) + } + if (commitException != null) + { + // connection.ExecuteTransaction failed with exception + if (_internalTransaction.IsCommitted) { - // 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. + // Even though we got an exception, the transaction + // was committed by the server. + enlistment.Committed(); } - - connection.CleanupConnectionOnTransactionCompletion(_atomicTransaction); - if (commitException == null) + else if (_internalTransaction.IsAborted) { - // connection.ExecuteTransaction succeeded - enlistment.Committed(); + // 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. + } + + connection.CleanupConnectionOnTransactionCompletion(_atomicTransaction); + if (commitException == null) + { + // connection.ExecuteTransaction succeeded + enlistment.Committed(); } } } diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/TransactionEnlistmentTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/TransactionEnlistmentTest.cs index 98a4cb755d..34061606f4 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/TransactionEnlistmentTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/TransactionEnlistmentTest.cs @@ -4,6 +4,7 @@ using System; using System.Data; +using System.Threading.Tasks; using System.Transactions; using Xunit; @@ -46,6 +47,30 @@ public static void TestManualEnlistment_Enlist_TxScopeComplete() RunTestSet(TestCase_ManualEnlistment_Enlist_TxScopeComplete); } + [SkipOnTargetFramework(TargetFrameworkMonikers.Netcoreapp)] + [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup), nameof(DataTestUtility.IsNotAzureServer))] + public static void TestEnlistmentPrepare_TxScopeComplete() + { + try + { + using TransactionScope txScope = new(TransactionScopeOption.RequiresNew, new TransactionOptions() + { + IsolationLevel = System.Transactions.IsolationLevel.ReadCommitted, + Timeout = TransactionManager.DefaultTimeout + }, TransactionScopeAsyncFlowOption.Enabled); + + using SqlConnection connection = new(DataTestUtility.TCPConnectionString); + connection.Open(); + System.Transactions.Transaction.Current.EnlistDurable(EnlistmentForPrepare.s_id, new EnlistmentForPrepare(), EnlistmentOptions.None); + txScope.Complete(); + Assert.False(true, "Expected exception not thrown."); + } + catch (Exception e) + { + Assert.True(e is TransactionAbortedException); + } + } + private static void TestCase_AutoEnlistment_TxScopeComplete() { SqlConnectionStringBuilder builder = new SqlConnectionStringBuilder(ConnectionString); @@ -168,6 +193,16 @@ private static void TestCase_ManualEnlistment_Enlist_TxScopeComplete() Assert.True(string.Equals(result.Rows[0][0], InputCol2)); } + class EnlistmentForPrepare : IEnlistmentNotification + { + public static readonly Guid s_id = Guid.NewGuid(); + // fail during prepare, this will cause scope.Complete to throw + public void Prepare(PreparingEnlistment preparingEnlistment) => preparingEnlistment.ForceRollback(); + public void Commit(Enlistment enlistment) => enlistment.Done(); + public void Rollback(Enlistment enlistment) => enlistment.Done(); + public void InDoubt(Enlistment enlistment) => enlistment.Done(); + } + private static string TestTableName; private static string ConnectionString; private const int InputCol1 = 1;