WIP: Fix connection pool concurrency issue

[cheenamalhotra]

Fixes connection pool concurrency issue caused due to waiting on multiple semaphore handles, instead of only creation handle during pool create request.

Issue was first introduced in PR #2390 (https://github.com/dotnet/SqlClient/pull/2390/files#diff-d7c1dbddf2a7e205d7a85cbafbef036899799672dd5d1c486a64a05979e92230R1627) and has affected MDS v6 onwards.
Needs to be backported to v6.0 and v6.1 branches as well.

WIP: Working on adding tests - running CI first.

[Copilot]

Pull Request Overview

This PR fixes a concurrency issue in the connection pool by changing how semaphore handles are waited on during pool creation requests. Instead of waiting on multiple semaphore handles simultaneously, it now waits only on the creation handle to prevent potential race conditions.

• Replaces WaitHandle.WaitAny() with direct WaitOne() on the creation semaphore
• Simplifies the wait result handling logic by removing the intermediate wait result case
• Removes unnecessary error tracing for non-timeout wait failures

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.71%. Comparing base (37d8a3f) to head (1c671db).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3632      +/-   ##
==========================================
+ Coverage   66.13%   66.71%   +0.58%     
==========================================
  Files         276      276              
  Lines       60765    60764       -1     
==========================================
+ Hits        40184    40541     +357     
+ Misses      20581    20223     -358     

Flag	Coverage Δ
addons	90.82% <ø> (ø)
netcore	69.91% <100.00%> (+1.83%)	⬆️
netfx	68.86% <100.00%> (-1.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.

[paulmedynski]

Asking to simplify/clarify the semaphore block.

Also, is there a way to test this?

[paulmedynski]

There's no need to assign WaitHandle.WaitTimeout here. waitResult is only compared to CREATION_HANDLE for the remainder of its scope. You can simplify these 2 lines to:

if (_waitHandles.CreationSemaphore.WaitOne(CreationTimeout))
{
    waitResult = CREATION_HANDLE;
}

[paulmedynski]

Looking closer, I don't think we need waitResult at all. It's being used as a stand-in for bool obtainedSemaphore. I think this would be clearer:

bool obtainedSemaphore = false;

try
{
    obtainedSemaphore = _waitHandles.CreationSemaphore.WaitOne(CreationTimeout);

    if (obtainedSemaphore)
    {
        ...
    }

    ...
}
finally
{
    if (obtainedSemaphore)
    {
        _waitHandles.CreationSemaphore.Release(1);
    }
}

[paulmedynski]

Bonus points if you want to create a SemaphoreScope RAII helper that obtains the semaphore on construction, and releases it on disposal. That would let the compiler write the finally block for you, and would keep the semaphore scoped inside the try block.

[mdaigle]

Agreed, the wait result pattern is only really meaningful when doing a WaitAny to see which semaphore you obtained.

[cheenamalhotra]

I started with bare minimum changes to make it work :P

[edwardneal]

This might also need to be backported to v5.1. #2390 ported the change from netcore, which has exactly the same behaviour:

SqlClient/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/ProviderBase/DbConnectionPool.cs

Line 1482 in ee2d196

waitResult = WaitHandle.WaitAny(_waitHandles.GetHandles(withCreate: true), CreationTimeout);

v6.0 is the first point that it impacted netfx, but this has been the netcore behaviour from the outset of the driver (even when going back to the PR which added System.Data.SqlClient to its first version of netcore.)