Fix | Fix deadlock in transaction (.NET Framework) (#1242)

Author: cheenamalhotra

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();
                             }
                         }
                     }

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;