From 9f8117aea271173b8d06611ac689a0de4f423e6f Mon Sep 17 00:00:00 2001
From: Parminder Kaur <88398605+Kaur-Parminder@users.noreply.github.com>
Date: Tue, 31 Jan 2023 11:27:16 -0800
Subject: [PATCH] [3.1.2] Backport: Release connection lock before signaling
SinglePhaseCommit completion (#1912)
[3.1.2] Backport: Release connection lock before signaling SinglePhaseCommit completion #1912
---
.../Data/SqlClient/SqlDelegatedTransaction.cs | 15 ++++---
....Data.SqlClient.ManualTesting.Tests.csproj | 1 +
.../DistributedTransactionTest.cs | 45 +++++++++++++++++++
.../Extensions/PlatformDetection.cs | 2 +
4 files changed, 56 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 3c289bb790..2e4007dc6f 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
@@ -340,6 +340,8 @@ public void SinglePhaseCommit(SinglePhaseEnlistment enlistment)
RuntimeHelpers.PrepareConstrainedRegions();
try
{
+ Exception commitException = null;
+
lock (connection)
{
// If the connection is doomed, we can be certain that the
@@ -354,7 +356,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.
@@ -364,7 +365,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)
{
@@ -412,13 +412,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 6da5a0d951..ba5b44e1a0 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
@@ -202,6 +202,7 @@
+
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..98f548d897
--- /dev/null
+++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/DistributedTransactionTest.cs
@@ -0,0 +1,45 @@
+// 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;
+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
+ {
+ private static bool s_DelegatedTransactionCondition => DataTestUtility.AreConnStringsSetup() && DataTestUtility.IsNotAzureServer() && PlatformDetection.IsNotX86Process;
+
+ [ConditionalFact(nameof(s_DelegatedTransactionCondition), 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
diff --git a/src/Microsoft.Data.SqlClient/tests/tools/Microsoft.DotNet.XUnitExtensions/Extensions/PlatformDetection.cs b/src/Microsoft.Data.SqlClient/tests/tools/Microsoft.DotNet.XUnitExtensions/Extensions/PlatformDetection.cs
index 1d089151aa..24b9e24b4f 100644
--- a/src/Microsoft.Data.SqlClient/tests/tools/Microsoft.DotNet.XUnitExtensions/Extensions/PlatformDetection.cs
+++ b/src/Microsoft.Data.SqlClient/tests/tools/Microsoft.DotNet.XUnitExtensions/Extensions/PlatformDetection.cs
@@ -10,5 +10,7 @@ public static partial class PlatformDetection
{
public static bool IsArmProcess => RuntimeInformation.ProcessArchitecture == Architecture.Arm;
public static bool IsNotArmProcess => !IsArmProcess;
+ public static bool IsX86Process => RuntimeInformation.ProcessArchitecture == Architecture.X86;
+ public static bool IsNotX86Process => !IsX86Process;
}
}