Fix 3640 | Remove extra connection deactivation.

[mdaigle]

Description

When I originally implemented #3019, due to a misunderstanding about when connections are activated/deactivated, I added an extraneous call to DbConnectionInternal.DeactivateConnection as part of the PutObjectFromTransactedPool. Connections are always deactivated at the time they are returned to the pool via the ReturnInternalConnection method and do not need to be activated/deactivated as they move between the main pool and the transacted pool. The activate and deactivate methods modify a counter value that tracks the number of connections in active use. This extra deactivate led to active connection counts going negative.

However, removing this DeactivateConnection call is also not entirely correct. Part of the deactivation process is setting the "reset" flags via a call to the ResetConnection method. These flags determine the Status value sent to the server when a connection is recycled out of the pool. Connections that are placed in the transacted pool may use status RESETCONNECTIONSKIPTRAN if they are participating in a distributed transaction. This gives extra guarantees that the server will not reset the connection's transaction state. When moving a connection out of the transacted pool and back into the main pool, we no longer want the server to maintain the association to the transaction because the transaction has ended and we want to use the connection for general purpose commands. Therefore, we need to downgrade the connection's status value to RESETCONNECTION. This way, the next time the connection is used its association with the transaction will be removed.

To appropriately set the reset flags without impacting the active connection counter, I exposed the ResetConnection method as internal so that it can be called directly by the pool. All other actions taken in DeactivateConnection are safe to skip at this point.

I hope to clean up this flow as part of adding transaction support to the new pool implementation, but it's out of scope for this PR.

Issues

#3640

Testing

None of our tests cover metrics. Working on adding that as part of the new connection pool work.
In the meantime, this change will be verified manually.

Existing distributed transaction tests show that the reset flags are still reevaluated correctly.

[Copilot]

Pull Request Overview

This PR fixes a bug where active connection counts were going negative due to an extraneous call to DeactivateConnection() when connections were moved from the transacted pool back to the main pool.

• Removes duplicate connection deactivation in PutObjectFromTransactedPool
• Connections are already deactivated when returned to the pool via ReturnInternalConnection
• Moving between transacted and main pools doesn't require re-deactivation

[Copilot]

Pull Request Overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

[benrr101]

Overall, probably pretty decent. Just want to see what's the best choice for whether to make it abstract or only defined on the SqlInternalConnectionTds

[Copilot]

Pull Request Overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs:3395

• The comment says "It is important to do this on activate" but this is misleading. The ResetConnection() method is actually called during deactivation (see InternalDeactivate() at line 1805) and when returning from the transacted pool, not during activation.

Consider updating the comment to accurately reflect when this is called:

// 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
// deactivation so that the connection state is prepared for recycling.

            // 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
            // activate so that the dictionary is correct before SqlConnection obtains a clone.

[Copilot]

Pull Request Overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

[Copilot]

Missing space around the '+' operator. Should be initialActiveHardConnections + 1 for consistency with line 241 and other assertions in the test.

Suggested change

	Assert.Equal(initialActiveHardConnections+1, SqlClientEventSourceProps.ActiveHardConnections);
	Assert.Equal(initialActiveHardConnections + 1, SqlClientEventSourceProps.ActiveHardConnections);

[Copilot]

Inconsistent blank line spacing. The ResetConnection() method should have a blank line before it to match the spacing pattern used for the Deactivate() method above (line 42).

Suggested change