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 8549c48a8d..15c32053d6 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionClosed.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionClosed.cs @@ -33,6 +33,7 @@ internal override void CloseConnection(DbConnection owningObject, SqlConnectionF // not much to do here... } + /// protected override void Deactivate() => ADP.ClosedConnectionError(); public override void EnlistTransaction(System.Transactions.Transaction transaction) => throw ADP.ClosedConnectionError(); @@ -55,6 +56,9 @@ internal override bool TryOpenConnection( TaskCompletionSource retry, DbConnectionOptions userOptions) => 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 34b8ec7c23..3e27bf5305 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs @@ -767,6 +767,13 @@ internal void PrePush(DbConnection 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; @@ -804,6 +811,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); /// @@ -820,6 +832,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 c588fcd3f7..ae56af3bcd 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 @@ -1379,8 +1379,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. @@ -1393,6 +1391,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 3d312430d8..46852a5df4 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlInternalConnection.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlInternalConnection.cs @@ -246,6 +246,7 @@ override protected DbReferenceCollection CreateReferenceCollection() return new SqlReferenceCollection(); } + /// override protected void Deactivate() { try diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs index 9f7858f2c9..80864716a3 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs @@ -3387,9 +3387,8 @@ private void OpenLoginEnlist( #endif } - // @TODO: Is this suppression still required - [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 to do this on diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/TracingTests/MetricsTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/TracingTests/MetricsTest.cs index debb5e7cce..f0b1b53b48 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/TracingTests/MetricsTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/TracingTests/MetricsTest.cs @@ -177,6 +177,73 @@ 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); + } + } + [ActiveIssue("https://github.com/dotnet/SqlClient/issues/3031")] [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))] public void ReclaimedConnectionsCounter_Functional() diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs index 5e47d618ed..219bb1fdf6 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -777,6 +777,11 @@ protected override void Deactivate() { return; } + + internal override void ResetConnection() + { + return; + } #endregion } #endregion diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ConnectionPoolSlotsTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ConnectionPoolSlotsTest.cs index 90dd4fd8e1..c4f437d981 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ConnectionPoolSlotsTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ConnectionPoolSlotsTest.cs @@ -43,6 +43,11 @@ protected override void Deactivate() { // Mock implementation - do nothing } + + internal override void ResetConnection() + { + // Mock implementation - do nothing + } } [Fact] diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/TransactedConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/TransactedConnectionPoolTest.cs index 8139767c74..3fc31338cd 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/TransactedConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/TransactedConnectionPoolTest.cs @@ -731,6 +731,10 @@ protected override void Deactivate() // Mock implementation - do nothing } + internal override void ResetConnection() + { + // Mock implementation - do nothing + } public override string ToString() => $"MockConnection_{MockId}"; }