Fix 3640 | Remove extra connection deactivation. (#3758)

Author: mdaigle

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...
         }
 
+        /// <inheritdoc/>
         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<DbConnectionInternal> retry,
             DbConnectionOptions userOptions) =>
             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

@@ -767,6 +767,13 @@ internal void PrePush(DbConnection 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;
@@ -804,6 +811,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>
@@ -820,6 +832,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

@@ -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

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

@@ -246,6 +246,7 @@ override protected DbReferenceCollection CreateReferenceCollection()
             return new SqlReferenceCollection();
         }
 
+        /// <inheritdoc/>
         override protected void Deactivate()
         {
             try

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()
+        /// <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 to do this on

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

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

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]

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}";
     }