Skip to content

Test | Connection pool transaction tests#3805

Merged
mdaigle merged 26 commits intomainfrom
dev/mdaigle/transacted-pool-stress-tests
Apr 2, 2026
Merged

Test | Connection pool transaction tests#3805
mdaigle merged 26 commits intomainfrom
dev/mdaigle/transacted-pool-stress-tests

Conversation

@mdaigle
Copy link
Copy Markdown
Contributor

@mdaigle mdaigle commented Nov 24, 2025 •
edited
Loading

This pull request introduces several improvements and refactorings across prompt definitions and the SQL client connection pool implementation. The most notable changes include enhanced tool scoping for prompts, improved naming and file references, and a refactor of the transaction-aware connection pool for clarity and maintainability.

Prompt and Documentation Improvements:

  • Added a tools field to prompt YAML frontmatter and updated documentation to recommend explicit tool scoping for each prompt, including a new section detailing tool selection best practices and syntax. This helps ensure prompts only access the tools they need, improving reliability and safety. [1] [2] [3]
  • Renamed prompts for clarity (doc-comments → generate-doc-comments, test-minimize-overlap → refine-test-overlap) and updated skill and script references to use consistent, absolute paths. [1] [2] [3]

Connection Pool Refactoring and Enhancements:

  • Exposed the TransactedConnectionPool through the IDbConnectionPool interface and relevant concrete classes, allowing better management of connections enlisted in transactions. [1] [2] [3] [4]
  • Refactored TransactedConnectionPool to use TransactedConnections (a Dictionary<Transaction, TransactedConnectionList>) with improved naming, access modifiers, and thread safety in lock usage. The TransactedConnectionList is now internal for broader accessibility within the assembly. [1] [2] [3] F2ff1f1cL93R93, F2ff1f1cL122R122, [4] F2ff1f1cL181R181, F2ff1f1cL212R212, F2ff1f1cL237R237, F2ff1f1cL296R296, F2ff1f1cL318R318)
  • Updated the TryGetConnection method signature in IDbConnectionPool to allow a nullable TaskCompletionSource, increasing flexibility in connection retrieval logic.

Other Codebase Improvements:

  • Exposed MaxPoolSize and MinPoolSize as internal in WaitHandleDbConnectionPool for improved testability or cross-class usage.
  • Added a summary comment to the CreationTimeout property in DbConnectionPoolOptions for clearer documentation.

Issues

#3356

Testing

Added comprehensive tests for WaitHandleDbConnectionPool's transacted connection flows. Extended stress tests to include transaction scenarios.

mdaigle added 15 commits November 10, 2025 10:04
Copilot AI review requested due to automatic review settings November 24, 2025 19:53
@github-project-automation github-project-automation bot moved this to To triage in SqlClient Board Nov 24, 2025
Copilot AI reviewed Nov 24, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mdaigle mdaigle marked this pull request as ready for review November 24, 2025 22:33
@mdaigle 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 mdaigle added this to the 7.0.0-preview3 milestone Nov 24, 2025
@mdaigle mdaigle mentioned this pull request Nov 24, 2025
Open
@mdaigle mdaigle changed the title Connection pool | Transaction tests Test | Connection pool transaction tests Mar 20, 2026
@mdaigle mdaigle added this to the 7.1.0-preview1 milestone Mar 20, 2026
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 20, 2026 •
edited
Loading

Codecov Report

❌ Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 66.38%. Comparing base (60d4b92) to head (da057ba).
⚠️ Report is 15 commits behind head on main.

Files with missing lines Patch % Lines
...qlClient/ConnectionPool/ChannelDbConnectionPool.cs 50.00% 1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (60d4b92) and HEAD (da057ba). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (60d4b92) HEAD (da057ba)
CI-SqlClient 1 0
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3805      +/-   ##
==========================================
- Coverage   73.22%   66.38%   -6.85%     
==========================================
  Files         280      274       -6     
  Lines       43000    65782   +22782     
==========================================
+ Hits        31486    43668   +12182     
- Misses      11514    22114   +10600
Flag Coverage Δ
CI-SqlClient ?
PR-SqlClient-Project 66.38% <94.11%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI review requested due to automatic review settings March 23, 2026 18:32
Copilot AI reviewed Mar 23, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

paulmedynski
paulmedynski requested changes Mar 23, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@paulmedynski paulmedynski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great overall - It will be so nice to have a pool that is well documented, tested, and understood.

@github-project-automation github-project-automation bot moved this from Backlog to In progress in SqlClient Board Mar 23, 2026
Copilot AI review requested due to automatic review settings March 26, 2026 21:09
Copilot AI reviewed Mar 26, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

@mdaigle
Copy link
Copy Markdown
Contributor Author

mdaigle commented Apr 1, 2026

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Apr 1, 2026

Azure Pipelines successfully started running 2 pipeline(s).

@mdaigle
Copy link
Copy Markdown
Contributor Author

mdaigle commented Apr 2, 2026

/azp run

@mdaigle mdaigle enabled auto-merge (squash) April 2, 2026 15:30
@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Apr 2, 2026

Azure Pipelines successfully started running 2 pipeline(s).

@mdaigle mdaigle dismissed benrr101’s stale review April 2, 2026 16:42

Comments addressed

@mdaigle mdaigle merged commit 9046321 into main Apr 2, 2026
299 checks passed
@mdaigle mdaigle deleted the dev/mdaigle/transacted-pool-stress-tests branch April 2, 2026 22:39
@github-project-automation github-project-automation bot moved this from In progress to Done in SqlClient Board Apr 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@priyankatiwari08 priyankatiwari08 priyankatiwari08 left review comments

@cheenamalhotra cheenamalhotra cheenamalhotra approved these changes

@paulmedynski paulmedynski paulmedynski approved these changes

@benrr101 benrr101 Awaiting requested review from benrr101

Assignees

@paulmedynski paulmedynski

Labels

Area\Tests Issues that are targeted to tests or test projects

Projects

SqlClient Board
Status: Done

Milestone

7.1.0-preview1

Development

Successfully merging this pull request may close these issues.

6 participants

@mdaigle @benrr101 @cheenamalhotra @paulmedynski @priyankatiwari08