Skip to content

Fix flaky connection pool tests for FIFO ordering #3751

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+32 −16
Open

Fix flaky connection pool tests for FIFO ordering #3751

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cbf8ba9
Fix flaky connection pool tests for FIFO ordering
mdaigle Nov 7, 2025
5585b10
Address ben's comments.
mdaigle Nov 10, 2025
d193807
Revert "Address ben's comments."
mdaigle Nov 10, 2025
bd5713b
Use Threa.Sleep instead of SpinWait
mdaigle Nov 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 32 additions & 16 deletions src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,6 @@ out DbConnectionInternal? recycledConnection
}

[Fact]
[ActiveIssue("https://github.com/dotnet/SqlClient/issues/3730")]
public async Task GetConnectionMaxPoolSize_ShouldRespectOrderOfRequest()
{
// Arrange
Expand All @@ -308,29 +307,31 @@ out DbConnectionInternal? internalConnection
Assert.NotNull(internalConnection);
}

// Use ManualResetEventSlim to synchronize the tasks
// and force the request queueing order.
using ManualResetEventSlim mresQueueOrder = new();
using CountdownEvent allRequestsQueued = new(2);
// Use multiple ManualResetEventSlim to ensure proper ordering
using ManualResetEventSlim firstTaskReady = new(false);
Copy link
Contributor

@benrr101 benrr101 Nov 10, 2025

Choose a reason for hiding this comment

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

Mixing async/await and native synchronization code seems a bit ... smelly to me.
Have we considered using TaskCompletionSource instead so we could use async objects for synchronization?

using ManualResetEventSlim secondTaskReady = new(false);
using ManualResetEventSlim startRequests = new(false);

// Act
var recycledTask = Task.Run(() =>
{
mresQueueOrder.Set();
allRequestsQueued.Signal();
firstTaskReady.Set();
startRequests.Wait();
pool.TryGetConnection(
Copy link
Contributor Author

@mdaigle mdaigle Nov 10, 2025

Choose a reason for hiding this comment

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

I think making these async introduced a deadlock. In some conditions, they'll hang on to threads and prevent future async operations from going through. I'm going to revert these changes other than the SpinWait -> Thread.Sleep()

Copy link
Contributor

@benrr101 benrr101 Dec 1, 2025

Choose a reason for hiding this comment

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

It'd be nice to get a better understanding of why the task completion source causes deadlocks in this case. I feel like this might be masking a bigger issue, or a lack of understanding of how the mechanisms are actually working.

new SqlConnection(""),
new SqlConnection("Timeout=5000"),
null,
new DbConnectionOptions("", null),
out DbConnectionInternal? recycledConnection
);
return recycledConnection;
});

var failedTask = Task.Run(() =>
{
// Force this request to be second in the queue.
mresQueueOrder.Wait();
allRequestsQueued.Signal();
secondTaskReady.Set();
startRequests.Wait();
// Add a small delay to ensure this request comes after the first
Thread.Sleep(50);
Copy link
Contributor

@benrr101 benrr101 Nov 10, 2025

Choose a reason for hiding this comment

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

This sleep is a bit fishy, too. (Well ... to me all thread.sleep in tests is fishy). I guess the goal is to make sure the tasks aren't "raced" so much as run one after the other. It probably works, but it seems like a patch.

pool.TryGetConnection(
new SqlConnection("Timeout=1"),
null,
Expand All @@ -340,7 +341,20 @@ out DbConnectionInternal? failedConnection
return failedConnection;
});

allRequestsQueued.Wait();
// Wait for both tasks to be ready before starting the requests
firstTaskReady.Wait();
secondTaskReady.Wait();

// Use SpinWait to ensure both tasks are actually waiting
Copy link

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

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

The comment mentions "Use SpinWait" but the code actually uses Thread.Sleep(100) instead of SpinWait. Consider either:

  1. Updating the comment to accurately reflect the synchronization mechanism: // Wait briefly to ensure both tasks are waiting on startRequests
  2. Or using an actual SpinWait if that's the intended approach

This comment-code mismatch could confuse future maintainers.

Suggested change
// Use SpinWait to ensure both tasks are actually waiting
// Wait briefly to ensure both tasks are waiting on startRequests

Copilot uses AI. Check for mistakes.
SpinWait.SpinUntil(() => false, 100);

// Start both requests
startRequests.Set();

// Give time for both requests to be queued
await Task.Delay(200);

// Return the connection which should satisfy the first queued request
pool.ReturnInternalConnection(firstConnection!, firstOwningConnection);
var recycledConnection = await recycledTask;

Expand All @@ -350,7 +364,6 @@ out DbConnectionInternal? failedConnection
}

[Fact]
[ActiveIssue("https://github.com/dotnet/SqlClient/issues/3730")]
public async Task GetConnectionAsyncMaxPoolSize_ShouldRespectOrderOfRequest()
{
// Arrange
Expand Down Expand Up @@ -382,14 +395,14 @@ out DbConnectionInternal? internalConnection

// Act
var exceeded = pool.TryGetConnection(
new SqlConnection(""),
new SqlConnection("Timeout=5000"),
recycledTaskCompletionSource,
new DbConnectionOptions("", null),
out DbConnectionInternal? recycledConnection
);

// Gives time for the recycled connection to be queued before the failed request is initiated.
await Task.Delay(1000);
// Ensure sufficient time for the recycled connection request to be fully queued
await Task.Delay(200);

var exceeded2 = pool.TryGetConnection(
new SqlConnection("Timeout=1"),
Expand All @@ -398,6 +411,9 @@ out DbConnectionInternal? recycledConnection
out DbConnectionInternal? failedConnection
);

// Ensure the second request is also queued
await Task.Delay(100);

pool.ReturnInternalConnection(firstConnection!, firstOwningConnection);
recycledConnection = await recycledTaskCompletionSource.Task;

Expand Down
Loading