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 80466509a6..2afde02578 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 @@ -977,8 +977,8 @@ protected override void InternalDeactivate() } } - [SuppressMessage("Microsoft.Globalization", "CA1303:DoNotPassLiteralsAsLocalizedParameters")] // copied from Triaged.cs - private void ResetConnection() + /// + internal override void ResetConnection() { // For implicit pooled connections, if connection reset behavior is specified, // reset the database and language properties back to default. It is important 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 4330e5205b..0f8b67301c 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 @@ -983,8 +983,8 @@ protected override void InternalDeactivate() } } - [SuppressMessage("Microsoft.Globalization", "CA1303:DoNotPassLiteralsAsLocalizedParameters")] // copied from Triaged.cs - private void ResetConnection() + /// + internal override void ResetConnection() { // For implicit pooled connections, if connection reset behavior is specified, // reset the database and language properties back to default. It is important diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionClosed.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionClosed.cs index d4ea183312..f4e87010a6 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionClosed.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionClosed.cs @@ -32,6 +32,7 @@ internal override void CloseConnection(DbConnection owningObject, DbConnectionFa // not much to do here... } + /// protected override void Deactivate() => ADP.ClosedConnectionError(); public override void EnlistTransaction(System.Transactions.Transaction transaction) => throw ADP.ClosedConnectionError(); @@ -43,6 +44,9 @@ protected internal override DataTable GetSchema(DbConnectionFactory factory, DbC internal override bool TryOpenConnection(DbConnection outerConnection, DbConnectionFactory connectionFactory, TaskCompletionSource retry, DbConnectionOptions userOptions) => base.TryOpenConnectionInternal(outerConnection, connectionFactory, retry, userOptions); + + /// + internal override void ResetConnection() => throw ADP.ClosedConnectionError(); } internal abstract class DbConnectionBusy : DbConnectionClosed diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs index b4dfb4f214..eafddd8ae2 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs @@ -797,6 +797,13 @@ internal void PrePush(object expectedOwner) internal void RemoveWeakReference(object value) => ReferenceCollection?.Remove(value); + /// + /// Idempotently resets the connection so that it may be recycled without leaking state. + /// May preserve transaction state if the connection is enlisted in a distributed transaction. + /// Should be called before the first action is taken on a recycled connection. + /// + internal abstract void ResetConnection(); + internal void SetInStasis() { IsTxRootWaitingForTxEnd = true; @@ -834,6 +841,11 @@ internal virtual bool TryReplaceConnection( #region Protected Methods + /// + /// Activates the connection, preparing it for active use. + /// An activated connection has an owner and is checked out from the connection pool (if pooling is enabled). + /// + /// The transaction in which the connection should enlist. protected abstract void Activate(Transaction transaction); /// @@ -850,6 +862,11 @@ protected virtual DbReferenceCollection CreateReferenceCollection() throw ADP.InternalError(ADP.InternalErrorCode.AttemptingToConstructReferenceCollectionOnStaticObject); } + /// + /// Deactivates the connection, cleaning up any state as necessary. + /// A deactivated connection is one that is no longer in active use and does not have an owner. + /// A deactivated connection may be open (connected to a server) and is checked into the connection pool (if pooling is enabled). + /// protected abstract void Deactivate(); protected internal void DoNotPoolThisConnection() diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs index 387019910b..cb099e27d3 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs @@ -1682,8 +1682,6 @@ public 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. @@ -1696,6 +1694,7 @@ public void PutObjectFromTransactedPool(DbConnectionInternal obj) if (State is Running && obj.CanBePooled) { + obj.ResetConnection(); PutNewObject(obj); } else diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlInternalConnection.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlInternalConnection.cs index 742c8b2865..0ea6c7c621 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlInternalConnection.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlInternalConnection.cs @@ -301,6 +301,7 @@ override protected DbReferenceCollection CreateReferenceCollection() return new SqlReferenceCollection(); } + /// override protected void Deactivate() { TdsParser bestEffortCleanupTarget = null; diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/TracingTests/MetricsTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/TracingTests/MetricsTest.cs index 4e90bbf6c7..bd70084fab 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/TracingTests/MetricsTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/TracingTests/MetricsTest.cs @@ -4,6 +4,7 @@ using System; using System.Diagnostics; using System.Reflection; +using System.Threading.Tasks; using System.Transactions; using Xunit; @@ -177,6 +178,143 @@ public void StasisCounters_Functional() Assert.Equal(0, SqlClientEventSourceProps.StasisConnections); } + [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup), nameof(DataTestUtility.IsNotAzureSynapse))] + public void TransactedConnectionPool_VerifyActiveConnectionCounters() + { + // This test verifies that the active connection count metric never goes negative + // when connections are returned to the pool while enlisted in a transaction. + // This is a regression test for issue #3640 where an extra DeactivateConnection + // call was causing the active connection count to go negative. + + // Arrange + var stringBuilder = new SqlConnectionStringBuilder(DataTestUtility.TCPConnectionString) + { + Pooling = true, + Enlist = false, + MinPoolSize = 0, + MaxPoolSize = 10 + }; + + // Clear pools to start fresh + ClearConnectionPools(); + + long initialActiveSoftConnections = SqlClientEventSourceProps.ActiveSoftConnections; + long initialActiveHardConnections = SqlClientEventSourceProps.ActiveHardConnections; + long initialActiveConnections = SqlClientEventSourceProps.ActiveConnections; + + // Act and Assert + // Verify counters at each step in the lifecycle of a transacted connection + using (var txScope = new TransactionScope()) + { + using (var conn = new SqlConnection(stringBuilder.ToString())) + { + conn.Open(); + conn.EnlistTransaction(System.Transactions.Transaction.Current); + + if (SupportsActiveConnectionCounters) + { + // Connection should be active + Assert.Equal(initialActiveSoftConnections + 1, SqlClientEventSourceProps.ActiveSoftConnections); + Assert.Equal(initialActiveHardConnections + 1, SqlClientEventSourceProps.ActiveHardConnections); + Assert.Equal(initialActiveConnections + 1, SqlClientEventSourceProps.ActiveConnections); + } + + conn.Close(); + + // Connection is returned to pool but still in transaction (stasis) + if (SupportsActiveConnectionCounters) + { + // Connection should be deactivated (returned to pool) + Assert.Equal(initialActiveSoftConnections, SqlClientEventSourceProps.ActiveSoftConnections); + Assert.Equal(initialActiveHardConnections + 1, SqlClientEventSourceProps.ActiveHardConnections); + Assert.Equal(initialActiveConnections, SqlClientEventSourceProps.ActiveConnections); + } + } + + // Completing the transaction after the connection is closed ensures that the connection + // is in the transacted pool at the time the transaction ends. This verifies that the + // transition from the transacted pool back to the main pool properly updates the counters. + txScope.Complete(); + } + + if (SupportsActiveConnectionCounters) + { + Assert.Equal(initialActiveSoftConnections, SqlClientEventSourceProps.ActiveSoftConnections); + Assert.Equal(initialActiveHardConnections + 1, SqlClientEventSourceProps.ActiveHardConnections); + Assert.Equal(initialActiveConnections, SqlClientEventSourceProps.ActiveConnections); + } + } + + #if NET + // Note: DbConnection.CloseAsync is not available in .NET Framework + [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup), nameof(DataTestUtility.IsNotAzureSynapse))] + public async Task TransactedConnectionPool_VerifyActiveConnectionCounters_Async() + { + // This test verifies that the active connection count metric never goes negative + // when connections are returned to the pool while enlisted in a transaction. + // This is a regression test for issue #3640 where an extra DeactivateConnection + // call was causing the active connection count to go negative. + + // Arrange + var stringBuilder = new SqlConnectionStringBuilder(DataTestUtility.TCPConnectionString) + { + Pooling = true, + Enlist = false, + MinPoolSize = 0, + MaxPoolSize = 10 + }; + + // Clear pools to start fresh + ClearConnectionPools(); + + long initialActiveSoftConnections = SqlClientEventSourceProps.ActiveSoftConnections; + long initialActiveHardConnections = SqlClientEventSourceProps.ActiveHardConnections; + long initialActiveConnections = SqlClientEventSourceProps.ActiveConnections; + + // Act and Assert + // Verify counters at each step in the lifecycle of a transacted connection + using (var txScope = new TransactionScope(TransactionScopeAsyncFlowOption.Enabled)) + { + using (var conn = new SqlConnection(stringBuilder.ToString())) + { + await conn.OpenAsync(); + conn.EnlistTransaction(System.Transactions.Transaction.Current); + + if (SupportsActiveConnectionCounters) + { + // Connection should be active + Assert.Equal(initialActiveSoftConnections + 1, SqlClientEventSourceProps.ActiveSoftConnections); + Assert.Equal(initialActiveHardConnections + 1, SqlClientEventSourceProps.ActiveHardConnections); + Assert.Equal(initialActiveConnections + 1, SqlClientEventSourceProps.ActiveConnections); + } + + await conn.CloseAsync(); + + // Connection is returned to pool but still in transaction (stasis) + if (SupportsActiveConnectionCounters) + { + // Connection should be deactivated (returned to pool) + Assert.Equal(initialActiveSoftConnections, SqlClientEventSourceProps.ActiveSoftConnections); + Assert.Equal(initialActiveHardConnections + 1, SqlClientEventSourceProps.ActiveHardConnections); + Assert.Equal(initialActiveConnections, SqlClientEventSourceProps.ActiveConnections); + } + } + + // Completing the transaction after the connection is closed ensures that the connection + // is in the transacted pool at the time the transaction ends. This verifies that the + // transition from the transacted pool back to the main pool properly updates the counters. + txScope.Complete(); + } + + if (SupportsActiveConnectionCounters) + { + Assert.Equal(initialActiveSoftConnections, SqlClientEventSourceProps.ActiveSoftConnections); + Assert.Equal(initialActiveHardConnections + 1, SqlClientEventSourceProps.ActiveHardConnections); + Assert.Equal(initialActiveConnections, SqlClientEventSourceProps.ActiveConnections); + } + } + #endif + [ActiveIssue("https://github.com/dotnet/SqlClient/issues/3031")] [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))] public void ReclaimedConnectionsCounter_Functional()