Connection pool transaction stress tests #3805

mdaigle · 2025-11-24T19:53:41Z

Description

The flow of transacting connections through the connection pool is tested by manual tests, but these lack coverage of more intricate scenarios:

reuse of transacting connections
concurrent access to the pool
concurrent connection requests for the same transaction
nested transactions
interplay with non-transacting connections
pool saturation (all slots used by transacting connections)
ordering of transaction completion and connection cleanup

This PR adds the missing test coverage. Also includes minor refactors to improve test visibility of properties and types.

Issues

#3356

Testing

Described above.

…ransacted-pool-stress-tests

Copilot

Pull request overview

This PR adds comprehensive stress tests for connection pool transaction handling under high concurrency scenarios. The tests verify that the WaitHandleDbConnectionPool and TransactedConnectionPool correctly manage connections when multiple threads perform transactional operations concurrently. Additionally, the PR removes several transitive package references for vulnerable dependencies that are no longer needed.

Key Changes

Added WaitHandleDbConnectionPoolTransactionStressTest.cs with extensive transaction stress tests covering various concurrency patterns
Exposed internal properties in WaitHandleDbConnectionPool and TransactedConnectionPool for test observability
Removed explicit references to System.Formats.Asn1, System.Text.Json, and System.Text.Encodings.Web packages (transitive dependencies)

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 17 comments.

Show a summary per file

File	Description
`WaitHandleDbConnectionPoolTransactionStressTest.cs`	New comprehensive test file with 1125 lines covering transaction stress scenarios including per-iteration transactions, per-thread transactions, shared transactions, pool saturation, nested transactions, mixed transacted/non-transacted operations, and edge cases
`WaitHandleDbConnectionPool.cs`	Changed `MaxPoolSize` and `MinPoolSize` from private to internal, and exposed `TransactedConnectionPool` property as internal for test observability
`TransactedConnectionPool.cs`	Changed `TransactedConnectionList` to internal and replaced private `_transactedCxns` dictionary with internal `TransactedConnections` property for test access
`Microsoft.Data.SqlClient.ExtUtilities.csproj`	Removed explicit reference to `System.Formats.Asn1` package
`netcore/src/Microsoft.Data.SqlClient.csproj`	Removed explicit reference to `System.Text.Json` package
`netcore/ref/Microsoft.Data.SqlClient.csproj`	Removed explicit reference to `System.Text.Json` package
`AzureKeyVaultProvider.csproj`	Removed explicit reference to `System.Text.Encodings.Web` package

....SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolTransactionStressTest.cs

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.

....SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolTransactionStressTest.cs

benrr101

The big problem here is adding stress tests to the unit test project. I can't approve that without some discussion on it.

benrr101 · 2025-12-01T20:07:34Z

src/Microsoft.Data.SqlClient/netcore/ref/Microsoft.Data.SqlClient.csproj

    <PackageReference Include="System.Security.Cryptography.Pkcs" />
-
-    <!-- Transitive dependencies that would otherwise bring in older, vulnerable versions. -->
-    <PackageReference Include="System.Text.Json" />


Don't we want our ref packages to have the same references as the implementation packages?

I'll clean these up as part of DRI this sprint. Had to include them here to get some build errors addressed.

benrr101 · 2025-12-01T20:08:02Z

.../AzureKeyVaultProvider/Microsoft.Data.SqlClient.AlwaysEncrypted.AzureKeyVaultProvider.csproj

  </ItemGroup>
  <ItemGroup>
    <PackageReference Include="Azure.Core" />
-    <PackageReference Include="System.Text.Encodings.Web" />


How is this related to adding stress tests for the connection pool?

benrr101 · 2025-12-01T20:20:13Z

....SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolTransactionStressTest.cs

+/// These tests verify that pool metrics remain consistent when connections are rapidly opened and closed
+/// with intermingled transactions in a highly concurrent environment.
+/// </summary>
+public class WaitHandleDbConnectionPoolTransactionStressTest : IDisposable


I'm really really really hesitant to approve putting stress tests into the unit test project. Our goal here should be to have unit tests be atomic tests of units of code that can be mocked and stubbed to coerce certain desired behaviors. Stress tests don't fit that bill, since they often are dependent on unexpected behaviors outside the scope of the code itself (system load, primarily).

We've sort of boxed ourselves into a corner here. The test TDS server is way too large to really be considered "unit testing". The code itself isn't structured for proper unit testing. And the stress test project isn't really ready for things to be moved into it.

I'd really like it if we address this from a different angle. Either move it into the stress test project (and I can help with that since I've still got the stress test project plans in my head a bit), or come up with a way to get the fake TDS server tests/code out of (or isolated within) the unit test project.

I agree, but these tests don't use the test TDS server, they use mocked connections.

paulmedynski · 2025-12-01T20:14:11Z

...osoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/TransactedConnectionPool.cs

    /// <value>The IDbConnectionPool instance that owns this transacted pool.</value>
    internal IDbConnectionPool Pool { get; }

+    internal Dictionary<Transaction, TransactedConnectionList> TransactedConnections { get; private set; }


Don't need private set here.

paulmedynski · 2025-12-01T20:15:53Z

.../AzureKeyVaultProvider/Microsoft.Data.SqlClient.AlwaysEncrypted.AzureKeyVaultProvider.csproj

  </ItemGroup>
  <ItemGroup>
    <PackageReference Include="Azure.Core" />
-    <PackageReference Include="System.Text.Encodings.Web" />


What's the scoop with these package removals? Have the transitive problems been solved somehow?

I'll clean these up. Visual studio was complaining, so I did this to appease it. I think it was just holding on to stale partial build state.

paulmedynski · 2025-12-01T20:24:52Z

....SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolTransactionStressTest.cs

+        Task.WaitAll(tasks);
+
+        // Assert
+        AssertPoolMetrics(_pool);


Should you be checking that the min/max are never exceeded at each iteration? This will just check that the pool currently hasn't exceeded those thresholds, not that it never did.

paulmedynski · 2025-12-01T20:27:49Z

....SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolTransactionStressTest.cs

+                {
+                    var owner = new SqlConnection();
+                    _pool.TryGetConnection(owner, null, new DbConnectionOptions("", null), out var conn);
+                    Assert.NotNull(conn);


Is there a way to confirm that this conn is enlisted with transaction?

paulmedynski · 2025-12-01T20:32:37Z

....SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolTransactionStressTest.cs

+            {
+                tasks[t] = Task.Run(() =>
+                {
+                    using (var innerScope = new TransactionScope(transaction))


What effect does innerScope have here? Should conn be associated to the inner scope, the outer scope, something else?

paulmedynski · 2025-12-01T20:40:31Z

....SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolTransactionStressTest.cs

+        }
+
+        // Shutdown pool while operations are in progress
+        _pool.Shutdown();


Are we certain there are operations in progress here? It's possible all tasks have run to completion already.

mdaigle added 15 commits November 10, 2025 10:04

Fix test failures.

8c66406

Expose transacted connection in debug to assert transacted pool state.

3c74365

Remove unnecessary context messages.

39b7851

Test cleanup

1f4bc05

Convert TransactedConnections to auto-property.

fab9a43

Make TransactedConnectionPool accessible when not in DEBUG.

cddc9eb

Clean up and fix tests. Expose min and max pool size properties.

9ba0b9c

Merge branch 'main' of github.com:dotnet/SqlClient into dev/mdaigle/t…

e1853da

…ransacted-pool-stress-tests

Clean up dependencies.

d4533c6

Add missing method override.

6cd9e82

Fix shared transaction test case.

d4b1e21

Fix async single transaction test.

e68ea14

Remove uninteresting test cases. Add todos for missing test cases.

1191ca4

Add more test cases. Speed up tests.

b969832

Clean up new tests.

42c8fbd

Copilot AI review requested due to automatic review settings November 24, 2025 19:53

Copilot started reviewing on behalf of mdaigle November 24, 2025 19:54 View session

Copilot finished reviewing on behalf of mdaigle November 24, 2025 19:57

Copilot AI reviewed Nov 24, 2025

View reviewed changes

mdaigle added 2 commits November 24, 2025 12:24

Improve cleanup

1f31bd2

Fix copilot issues. Add doc comment.

90f2686

mdaigle marked this pull request as ready for review November 24, 2025 22:33

mdaigle requested a review from a team as a code owner November 24, 2025 22:33

Copilot AI review requested due to automatic review settings November 24, 2025 22:33

mdaigle added this to the 7.0.0-preview3 milestone Nov 24, 2025

Copilot started reviewing on behalf of mdaigle November 24, 2025 22:33 View session

mdaigle mentioned this pull request Nov 24, 2025

Redesign the SqlClient Connection Pool to Improve Performance and Async Support #3356

Open

Copilot finished reviewing on behalf of mdaigle November 24, 2025 22:37

Copilot AI reviewed Nov 24, 2025

View reviewed changes

Address copilot comments.

861b0e7

Fix tests. Expose TransactedConnectionPool.

017353c

paulmedynski self-assigned this Nov 27, 2025

benrr101 requested changes Dec 1, 2025

View reviewed changes

paulmedynski requested changes Dec 1, 2025

View reviewed changes

benrr101 added the Area\Tests Issues that are targeted to tests or test projects label Dec 2, 2025

benrr101 modified the milestones: 7.0.0-preview3, 7.0.0-preview4 Dec 2, 2025

Connection pool transaction stress tests #3805

Are you sure you want to change the base?

Connection pool transaction stress tests #3805

Uh oh!

Conversation

mdaigle commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benrr101 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdaigle Dec 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mdaigle commented Nov 24, 2025 •

edited

Loading

mdaigle Dec 1, 2025 •

edited

Loading