From 8116d72f24d780847ca9b0e31e56f4621bd32276 Mon Sep 17 00:00:00 2001 From: Shay Rojansky Date: Sun, 9 Oct 2022 07:04:55 +0200 Subject: [PATCH 1/2] Release lock before signaling SinglePhaseCommit completion Fixes #1800 --- .../Data/SqlClient/SqlDelegatedTransaction.cs | 15 ++++---- .../DistributedTransactionTest.cs | 38 +++++++++++++++++++ 2 files changed, 46 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/SQL/TransactionTest/DistributedTransactionTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/DistributedTransactionTest.cs new file mode 100644 index 0000000000..5ac202623b --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/DistributedTransactionTest.cs @@ -0,0 +1,38 @@ +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] + 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); + + transaction.Commit(); + // We never get here because of the deadlock + } + } +} + +#endif From 3552268822f787738c8c80ab4778aafc9ba4198f Mon Sep 17 00:00:00 2001 From: Davoud Eshtehari Date: Tue, 29 Nov 2022 18:21:50 -0800 Subject: [PATCH 2/2] Fix test --- ...Microsoft.Data.SqlClient.ManualTesting.Tests.csproj | 1 + .../SQL/TransactionTest/DistributedTransactionTest.cs | 10 +++++++--- 2 files changed, 8 insertions(+), 3 deletions(-) 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 1efdfeb5fc..6a50720b18 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 @@ -192,6 +192,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 index 5ac202623b..a85f64e42d 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/DistributedTransactionTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/DistributedTransactionTest.cs @@ -1,4 +1,8 @@ -using System.Threading.Tasks; +// 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; @@ -9,7 +13,7 @@ namespace Microsoft.Data.SqlClient.ManualTesting.Tests [PlatformSpecific(TestPlatforms.Windows)] public class DistributedTransactionTest { - [ConditionalFact] + [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup), nameof(DataTestUtility.IsNotAzureServer), Timeout = 10000)] public async Task Delegated_transaction_deadlock_in_SinglePhaseCommit() { TransactionManager.ImplicitDistributedTransactions = true; @@ -29,8 +33,8 @@ public async Task Delegated_transaction_deadlock_in_SinglePhaseCommit() await conn2.OpenAsync(); conn2.EnlistTransaction(transaction); + // Possible deadlock transaction.Commit(); - // We never get here because of the deadlock } } }