From ef11f4e0c35c55e307e4705b52929aa4888aca70 Mon Sep 17 00:00:00 2001 From: Parminder Kaur Date: Mon, 30 Jan 2023 18:12:38 -0800 Subject: [PATCH 1/2] Backport: Release connection lock before signaling SinglePhaseCommit completion --- .../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 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 0db90a4686..763cff1620 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 @@ -201,6 +201,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..d10cda85a5 --- /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 From 27dc4b880f968ba37ec4f5097931b856e2edc268 Mon Sep 17 00:00:00 2001 From: Parminder Kaur Date: Tue, 31 Jan 2023 10:38:43 -0800 Subject: [PATCH 2/2] Backport changes: Tests | Skip unsupported platform x86 test and bump up Microsoft.NET.Test.Sdk version --- .../SQL/TransactionTest/DistributedTransactionTest.cs | 5 ++++- .../Extensions/PlatformDetection.cs | 2 ++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/DistributedTransactionTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/DistributedTransactionTest.cs index d10cda85a5..98f548d897 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/DistributedTransactionTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/DistributedTransactionTest.cs @@ -2,6 +2,7 @@ // 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; @@ -13,7 +14,9 @@ namespace Microsoft.Data.SqlClient.ManualTesting.Tests [PlatformSpecific(TestPlatforms.Windows)] public class DistributedTransactionTest { - [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup), nameof(DataTestUtility.IsNotAzureServer), Timeout = 10000)] + 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; 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; } }