From 064c835b6bd5edbd103c9435ea9a3ff5bc1f6f2f Mon Sep 17 00:00:00 2001
From: Shay Rojansky <roji@roji.org>
Date: Tue, 20 Dec 2022 22:07:09 +0100
Subject: [PATCH] Release connection lock before signaling SinglePhaseCommit
 completion (#1801)

---
 .../Data/SqlClient/SqlDelegatedTransaction.cs | 15 +++----
 ....Data.SqlClient.ManualTesting.Tests.csproj |  1 +
 .../DistributedTransactionTest.cs             | 42 +++++++++++++++++++
 3 files changed, 51 insertions(+), 7 deletions(-)
 create mode 100644 src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/DistributedTransactionTest.cs

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 2b9fc6f472..680b34079d 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
@@ -348,6 +348,8 @@ public void SinglePhaseCommit(SinglePhaseEnlistment enlistment)
 #endif
                 try
                 {
+                    Exception commitException = null;
+
                     lock (connection)
                     {
                         // If the connection is doomed, we can be certain that the
@@ -362,7 +364,6 @@ public void SinglePhaseCommit(SinglePhaseEnlistment enlistment)
                         }
                         else
                         {
-                            Exception commitException;
                             try
                             {
                                 // Now that we've acquired the lock, make sure we still have valid state for this operation.
@@ -372,7 +373,6 @@ public void SinglePhaseCommit(SinglePhaseEnlistment enlistment)
                                 _connection = null;   // Set prior to ExecuteTransaction call in case this initiates a TransactionEnd event
 
                                 connection.ExecuteTransaction(SqlInternalConnection.TransactionRequest.Commit, null, System.Data.IsolationLevel.Unspecified, _internalTransaction, true);
-                                commitException = null;
                             }
                             catch (SqlException e)
                             {
@@ -420,13 +420,14 @@ public void SinglePhaseCommit(SinglePhaseEnlistment enlistment)
                             }
 
                             connection.CleanupConnectionOnTransactionCompletion(_atomicTransaction);
-                            if (commitException == null)
-                            {
-                                // connection.ExecuteTransaction succeeded
-                                enlistment.Committed();
-                            }
                         }
                     }
+
+                    if (commitException == null)
+                    {
+                        // connection.ExecuteTransaction succeeded
+                        enlistment.Committed();
+                    }
                 }
                 catch (System.OutOfMemoryException e)
                 {
diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTesting.Tests.csproj b/src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTesting.Tests.csproj
index 7dcdbda147..4f5cdb378c 100644
--- a/src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTesting.Tests.csproj
+++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTesting.Tests.csproj
@@ -193,6 +193,7 @@
     <Compile Include="SQL\SqlNotificationTest\SqlNotificationTest.cs" />
     <Compile Include="SQL\SqlSchemaInfoTest\SqlSchemaInfoTest.cs" />
     <Compile Include="SQL\SqlStatisticsTest\SqlStatisticsTest.cs" />
+    <Compile Include="SQL\TransactionTest\DistributedTransactionTest.cs" />
     <Compile Include="SQL\TransactionTest\TransactionTest.cs" />
     <Compile Include="SQL\TransactionTest\TransactionEnlistmentTest.cs" />
     <Compile Include="SQL\UdtTest\SqlServerTypesTest.cs" />
diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/DistributedTransactionTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/DistributedTransactionTest.cs
new file mode 100644
index 0000000000..a85f64e42d
--- /dev/null
+++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/DistributedTransactionTest.cs
@@ -0,0 +1,42 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+using System.Threading.Tasks;
+using System.Transactions;
+using Xunit;
+
+#if NET7_0_OR_GREATER
+
+namespace Microsoft.Data.SqlClient.ManualTesting.Tests
+{
+    [PlatformSpecific(TestPlatforms.Windows)]
+    public class DistributedTransactionTest
+    {
+        [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup), nameof(DataTestUtility.IsNotAzureServer), Timeout = 10000)]
+        public async Task Delegated_transaction_deadlock_in_SinglePhaseCommit()
+        {
+            TransactionManager.ImplicitDistributedTransactions = true;
+            using var transaction = new CommittableTransaction();
+
+            // Uncommenting the following makes the deadlock go away as a workaround. If the transaction is promoted before
+            // the first SqlClient enlistment, it never goes into the delegated state.
+            // _ = TransactionInterop.GetTransmitterPropagationToken(transaction);
+            await using var conn = new SqlConnection(DataTestUtility.TCPConnectionString);
+            await conn.OpenAsync();
+            conn.EnlistTransaction(transaction);
+
+            // Enlisting the transaction in second connection causes the transaction to be promoted.
+            // After this, the transaction state will be "delegated" (delegated to SQL Server), and the commit below will
+            // trigger a call to SqlDelegatedTransaction.SinglePhaseCommit.
+            await using var conn2 = new SqlConnection(DataTestUtility.TCPConnectionString);
+            await conn2.OpenAsync();
+            conn2.EnlistTransaction(transaction);
+
+            // Possible deadlock
+            transaction.Commit();
+        }
+    }
+}
+
+#endif