From 68a10759eba2c2ed3d02fe0b917dc2066b648036 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 15 Jan 2025 10:38:24 -0800 Subject: [PATCH] Preserve distributed transactions on pooled connection reset (#3019) --- .../SqlClient/SqlInternalConnectionTds.cs | 10 +- .../SqlClient/SqlInternalConnectionTds.cs | 8 +- .../Data/ProviderBase/DbConnectionPool.cs | 2 + ....Data.SqlClient.ManualTesting.Tests.csproj | 2 +- .../DistributedTransactionTest.Windows.cs | 181 ++++++++++++++++++ .../DistributedTransactionTest.cs | 46 ----- 6 files changed, 194 insertions(+), 55 deletions(-) create mode 100644 src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/DistributedTransactionTest.Windows.cs delete mode 100644 src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/DistributedTransactionTest.cs diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs index 1f7cb869b1..9b65f20d6f 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs @@ -946,11 +946,11 @@ private void ResetConnection() if (_fResetConnection) { - // Ensure we are either going against 2000, or we are not enlisted in a - // distributed transaction - otherwise don't reset! - // Prepare the parser for the connection reset - the next time a trip - // to the server is made. - _parser.PrepareResetConnection(IsTransactionRoot && !IsNonPoolableTransactionRoot); + // Pooled connections that are enlisted in a transaction must have their transaction + // preserved when reseting the connection state. Otherwise, future uses of the connection + // from the pool will execute outside of the transaction, in auto-commit mode. + // https://github.com/dotnet/SqlClient/issues/2970 + _parser.PrepareResetConnection(EnlistedTransaction is not null && Pool is not null); // Reset dictionary values, since calling reset will not send us env_changes. CurrentDatabase = _originalDatabase; diff --git a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs index d0c64d07e3..5152e1ff50 100644 --- a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs +++ b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs @@ -1075,9 +1075,11 @@ private void ResetConnection() // distributed transaction - otherwise don't reset! if (Is2000) { - // Prepare the parser for the connection reset - the next time a trip - // to the server is made. - _parser.PrepareResetConnection(IsTransactionRoot && !IsNonPoolableTransactionRoot); + // Pooled connections that are enlisted in a transaction must have their transaction + // preserved when reseting the connection state. Otherwise, future uses of the connection + // from the pool will execute outside of the transaction, in auto-commit mode. + // https://github.com/dotnet/SqlClient/issues/2970 + _parser.PrepareResetConnection(EnlistedTransaction is not null && Pool is not null); } else if (!IsEnlistedInTransaction) { diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionPool.cs index 336f690776..8a41b3d428 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionPool.cs @@ -1805,6 +1805,8 @@ internal void PutObjectFromTransactedPool(DbConnectionInternal obj) Debug.Assert(obj != null, "null pooledObject?"); Debug.Assert(obj.EnlistedTransaction == null, "pooledObject is still enlisted?"); + obj.DeactivateConnection(); + // called by the transacted connection pool , once it's removed the // connection from it's list. We put the connection back in general // circulation. 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 a5b217f1cd..112814212e 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 @@ -200,7 +200,7 @@ - + diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/DistributedTransactionTest.Windows.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/DistributedTransactionTest.Windows.cs new file mode 100644 index 0000000000..9cda941762 --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/DistributedTransactionTest.Windows.cs @@ -0,0 +1,181 @@ +// 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.Data; +using System.Runtime.InteropServices; +using System.Threading.Tasks; +using System.Transactions; +using Microsoft.Data.SqlClient.TestUtilities; +using Xunit; + +namespace Microsoft.Data.SqlClient.ManualTesting.Tests +{ + + [PlatformSpecific(TestPlatforms.Windows)] + public class DistributedTransactionTestWindows + { + +#if NET + private static bool s_DelegatedTransactionCondition => DataTestUtility.AreConnStringsSetup() && DataTestUtility.IsNotAzureServer() && DataTestUtility.IsNotX86Architecture; + + [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 + + private static bool s_EnlistedTransactionPreservedWhilePooledCondition => DataTestUtility.AreConnStringsSetup() && DataTestUtility.IsNotX86Architecture; + + [ConditionalFact(nameof(s_EnlistedTransactionPreservedWhilePooledCondition), Timeout = 10000)] + public void Test_EnlistedTransactionPreservedWhilePooled() + { +#if NET + TransactionManager.ImplicitDistributedTransactions = true; +#endif + RunTestSet(EnlistedTransactionPreservedWhilePooled); + } + + private void EnlistedTransactionPreservedWhilePooled() + { + Exception commandException = null; + Exception transactionException = null; + + try + { + using (TransactionScope txScope = new TransactionScope(TransactionScopeOption.Required, TimeSpan.MaxValue)) + { + // Leave first connection open so that the transaction is promoted + SqlConnection rootConnection = new SqlConnection(ConnectionString); + rootConnection.Open(); + using (SqlCommand command = rootConnection.CreateCommand()) + { + command.CommandText = $"INSERT INTO {TestTableName} VALUES ({InputCol1}, '{InputCol2}')"; + command.ExecuteNonQuery(); + } + + // Closing and reopening cycles the connection through the pool. + // We want to verify that the transaction state is preserved through this cycle. + SqlConnection enlistedConnection = new SqlConnection(ConnectionString); + enlistedConnection.Open(); + enlistedConnection.Close(); + enlistedConnection.Open(); + + // Forcibly kill the root connection to mimic gateway's behavior when using the proxy connection policy + // https://techcommunity.microsoft.com/blog/azuredbsupport/azure-sql-database-idle-sessions-are-killed-after-about-30-minutes-when-proxy-co/3268601 + // Can also represent a general server-side, process failure + KillProcess(rootConnection.ServerProcessId); + + + using (SqlCommand command = enlistedConnection.CreateCommand()) + { + command.CommandText = $"INSERT INTO {TestTableName} VALUES ({InputCol1}, '{InputCol2}')"; + try + { + command.ExecuteNonQuery(); + } + catch (Exception ex) + { + commandException = ex; + } + } + + txScope.Complete(); + } + } + catch (Exception ex) + { + transactionException = ex; + } + + if (Utils.IsAzureSqlServer(new SqlConnectionStringBuilder((ConnectionString)).DataSource)) + { + // Even if an application swallows the command exception, completing the transaction should indicate that it failed. + Assert.IsType(transactionException); + // See https://learn.microsoft.com/en-us/sql/relational-databases/errors-events/database-engine-events-and-errors-3000-to-3999?view=sql-server-ver16 + // Error 3971 corresponds to "The server failed to resume the transaction." + Assert.Equal(3971, ((SqlException)commandException).Number); + } + else + { + Assert.IsType(transactionException); + +#if NETFRAMEWORK + // See https://learn.microsoft.com/en-us/sql/relational-databases/errors-events/database-engine-events-and-errors-8000-to-8999?view=sql-server-ver16 + // The distributed transaction failed + Assert.Equal(8525, ((SqlException)commandException).Number); +#else + Assert.IsType(commandException); +#endif + } + + // Verify that nothing made it into the database + DataTable result = DataTestUtility.RunQuery(ConnectionString, $"select col2 from {TestTableName} where col1 = {InputCol1}"); + Assert.True(result.Rows.Count == 0); + } + + private void KillProcess(int serverProcessId) + { + using (TransactionScope txScope = new TransactionScope(TransactionScopeOption.Suppress)) + { + using (SqlConnection connection = new SqlConnection(ConnectionString)) + { + connection.Open(); + using (SqlCommand command = connection.CreateCommand()) + { + command.CommandText = $"KILL {serverProcessId}"; + command.ExecuteNonQuery(); + } + } + txScope.Complete(); + } + } + + private static string TestTableName; + private static string ConnectionString; + private const int InputCol1 = 1; + private const string InputCol2 = "One"; + + private static void RunTestSet(Action TestCase) + { + SqlConnectionStringBuilder builder = new SqlConnectionStringBuilder(DataTestUtility.TCPConnectionString); + + builder.Pooling = true; + builder.MaxPoolSize = 5; + builder.Enlist = true; + ConnectionString = builder.ConnectionString; + + TestTableName = DataTestUtility.GenerateObjectName(); + DataTestUtility.RunNonQuery(ConnectionString, $"create table {TestTableName} (col1 int, col2 text)"); + try + { + TestCase(); + } + finally + { + DataTestUtility.RunNonQuery(ConnectionString, $"drop table {TestTableName}"); + } + } + } +} + diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/DistributedTransactionTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/DistributedTransactionTest.cs deleted file mode 100644 index 658104e193..0000000000 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/DistributedTransactionTest.cs +++ /dev/null @@ -1,46 +0,0 @@ -// 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.Runtime.InteropServices; -using System.Threading.Tasks; -using System.Transactions; -using Xunit; - -#if NET8_0_OR_GREATER - -namespace Microsoft.Data.SqlClient.ManualTesting.Tests -{ - [PlatformSpecific(TestPlatforms.Windows)] - public class DistributedTransactionTest - { - private static bool s_DelegatedTransactionCondition => DataTestUtility.AreConnStringsSetup() && DataTestUtility.IsNotAzureServer() && DataTestUtility.IsNotX86Architecture; - - [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