Skip to content

Replace internal connection pool state enum with a boolean - #4796

Open
priyankatiwari08 wants to merge 1 commit into
dotnet:mainfrom
priyankatiwari08:priyankatiwari08-dev/automation/boolean-connection-pool-s
Open

priyankatiwari08 wants to merge 1 commit into
dotnet:mainfrom
priyankatiwari08:priyankatiwari08-dev/automation/boolean-connection-pool-s

Conversation

@priyankatiwari08

Copy link
Copy Markdown
Contributor

Summary

Fixes #4795.

  • Replace DbConnectionPoolState and the internal State property with a volatile boolean exposed through the existing IsRunning contract in both pool implementations.
  • Preserve construction-time running state, terminal shutdown, existing shutdown guards, and each pool's admitted-request/cleanup semantics. Startup and Clear do not resurrect retired pools.
  • Update internal diagnostics, lifecycle documentation, test helpers, and assertions. Public SqlConnection.State and System.Data.ConnectionState are unchanged.
  • Add shared cross-pool lifecycle regressions and strengthen channel shutdown tests to deterministically cover both synchronous and asynchronous parked waiters and the exact shutdown error. Remove two redundant enum-property tests already covered by running-state tests.

Validation

  • Original shutdown baseline: 20 passed, 0 skipped, 0 failed on net8.0.
  • Strengthened shutdown/lifecycle coverage: 27 passed, 0 skipped, 0 failed, both before and after the production refactor on net8.0.
  • Full connection-pool unit suite: 411 passed, 0 skipped, 0 failed per framework/configuration for net462, net8.0, net9.0, and net10.0, in both Debug and Release (3,288 passing executions). Driver dependencies built for their applicable frameworks.
  • Filters verified with --list-tests; no remaining DbConnectionPoolState references; git diff --check clean.

Matrix command (run for Debug and Release):

dotnet test src\Microsoft.Data.SqlClient\tests\UnitTests\Microsoft.Data.SqlClient.UnitTests.csproj --configuration <configuration> --filter 'FullyQualifiedName~Microsoft.Data.SqlClient.UnitTests.ConnectionPool.&category!=failing&category!=flaky&category!=interactive&category!=signed' --blame-hang-timeout 10m

These are isolated/mock pool tests; no external SQL Server credentials were required. Live SQL Server manual tests were not run for this internal representation-only refactor.

Checklist

  • Tests added or updated
  • Public API changes documented (not applicable; public API unchanged)
  • Verified against customer repro (not applicable; behavior-preserving refactor, existing lifecycle behavior verified before/after)
  • Ensure no breaking changes introduced

Fixes dotnet#4795

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 6, 2026 08:21
@priyankatiwari08
priyankatiwari08 requested a review from a team as a code owner October 6, 2026 08:21
@priyankatiwari08 priyankatiwari08 added this to the 8.0.0-preview1 milestone Oct 6, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

Final human review is needed because completed current-head CI and required supplemental policy checks were not verified.

Review effort: Balanced
Findings: None

What changed in this PR

Simplifies internal connection-pool lifecycle tracking for #4795 without changing public connection-state APIs.

Changes:

  • Replaces the state enum with a volatile running flag in both pool implementations.
  • Updates lifecycle documentation, diagnostics, and test helpers.
  • Adds shared lifecycle regressions and strengthens synchronous/asynchronous shutdown tests.
File Description
src/​Microsoft.Data.SqlClient/​tests/​UnitTests/​ConnectionPool/​WaitHandleDbConnectionPoolShutdownTest.cs Uses running-state assertions.
src/​Microsoft.Data.SqlClient/​tests/​UnitTests/​ConnectionPool/​TransactedConnectionPoolTest.cs Removes obsolete mock state property.
src/​Microsoft.Data.SqlClient/​tests/​UnitTests/​ConnectionPool/​DbConnectionPoolRunningTest.cs Adds cross-pool lifecycle tests.
src/​Microsoft.Data.SqlClient/​tests/​UnitTests/​ConnectionPool/​ChannelDbConnectionPoolTest.cs Removes redundant enum-property tests.
src/​Microsoft.Data.SqlClient/​tests/​UnitTests/​ConnectionPool/​ChannelDbConnectionPoolShutdownTest.cs Verifies parked waiters receive the shutdown error.
src/​Microsoft.Data.SqlClient/​tests/​UnitTests/​ConnectionPool/​ChannelDbConnectionPoolPruningTest.cs Updates pruning shutdown assertions.
src/​Microsoft.Data.SqlClient/​src/​Microsoft/​Data/​SqlClient/​ConnectionPool/​WaitHandleDbConnectionPool.cs Replaces enum checks with the running flag.
src/​Microsoft.Data.SqlClient/​src/​Microsoft/​Data/​SqlClient/​ConnectionPool/​IDbConnectionPool.cs Removes State and documents lifecycle semantics.
src/​Microsoft.Data.SqlClient/​src/​Microsoft/​Data/​SqlClient/​ConnectionPool/​DbConnectionPoolState.cs Deletes the internal enum.
src/​Microsoft.Data.SqlClient/​src/​Microsoft/​Data/​SqlClient/​ConnectionPool/​ChannelDbConnectionPool.cs Replaces enum checks with the running flag.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: To triage

Development

Successfully merging this pull request may close these issues.

Replace internal connection pool state enum with a boolean

4 participants