[6.1] Fix 3640 | Remove extra connection deactivation. (#3776)

Author: mdaigle

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()
+        /// <inheritdoc/>
+        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

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()
+        /// <inheritdoc/>
+        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

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...
         }
 
+        /// <inheritdoc/>
         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<DbConnectionInternal> retry, DbConnectionOptions userOptions)
             => base.TryOpenConnectionInternal(outerConnection, connectionFactory, retry, userOptions);
+
+        /// <inheritdoc/>
+        internal override void ResetConnection() => throw ADP.ClosedConnectionError();
     }
 
     internal abstract class DbConnectionBusy : DbConnectionClosed

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);
 
+        /// <summary>
+        /// 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.
+        /// </summary>
+        internal abstract void ResetConnection();
+
         internal void SetInStasis()
         {
             IsTxRootWaitingForTxEnd = true;
@@ -834,6 +841,11 @@ internal virtual bool TryReplaceConnection(
 
         #region Protected Methods
 
+        /// <summary>
+        /// 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).
+        /// </summary>
+        /// <param name="transaction">The transaction in which the connection should enlist.</param>
         protected abstract void Activate(Transaction transaction);
 
         /// <summary>
@@ -850,6 +862,11 @@ protected virtual DbReferenceCollection CreateReferenceCollection()
             throw ADP.InternalError(ADP.InternalErrorCode.AttemptingToConstructReferenceCollectionOnStaticObject);
         }
 
+        /// <summary>
+        /// 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).
+        /// </summary>
         protected abstract void Deactivate();
 
         protected internal void DoNotPoolThisConnection()

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

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlInternalConnection.cs

@@ -301,6 +301,7 @@ override protected DbReferenceCollection CreateReferenceCollection()
             return new SqlReferenceCollection();
         }
 
+        /// <inheritdoc/>
         override protected void Deactivate()
         {
             TdsParser bestEffortCleanupTarget = null;

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()