Fix flaky connection pool tests for FIFO ordering #3751
Conversation
- Fixed GetConnectionMaxPoolSize_ShouldRespectOrderOfRequest with improved synchronization - Replaced unreliable ManualResetEventSlim + CountdownEvent with multiple sync primitives - Added SpinWait to ensure proper task coordination - Increased timeout to 5000ms and added strategic delays for reliable ordering - Removed [ActiveIssue] attribute - Fixed GetConnectionAsyncMaxPoolSize_ShouldRespectOrderOfRequest - Increased timeout to 5000ms for first connection request - Optimized delays (200ms + 100ms) to ensure proper request queueing - Removed [ActiveIssue] attribute Both tests now pass consistently (100% success rate over 5 runs x 3 frameworks) Fixes #3730
mdaigle
force-pushed
the
dev/mdaigle/fix-flaky-connection-pool-tests
branch
from
9a8a85d to
cbf8ba9
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3751 +/- ##
==========================================
+ Coverage 76.58% 76.63% +0.04%
==========================================
Files 272 273 +1
Lines 44177 44180 +3
==========================================
+ Hits 33835 33857 +22
+ Misses 10342 10323 -19
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
benrr101
left a comment
benrr101
left a comment
There was a problem hiding this comment.
Adding sleeps in a test is always a bit of a smell. Let's double check that the right sleeps are being used and that sleeping is the best choice.
| secondTaskReady.Wait(); | ||
|
|
||
| // Use SpinWait to ensure both tasks are actually waiting | ||
| SpinWait.SpinUntil(() => false, 100); |
There was a problem hiding this comment.
Using SpinUntil is a weird choice here - by using () => false it's saying spin forever, up to a timeout of 100ms. But using Spin instead of Thread.Sleep you're hanging the processor doing nothing for 100ms. SpinWait is OK for things that are going to happen very quickly, but for a 0.1s delay, it's really wasteful, and even in other areas, we're using Thread.Sleep for a 0.05s delay. I'd encourage replacing this with Thread.Sleep (if the sleep is really necessary, as manual sleep in tests is a smell), or if SpinWait is the best solution, it should documented why it is the best solution.
| using ManualResetEventSlim mresQueueOrder = new(); | ||
| using CountdownEvent allRequestsQueued = new(2); | ||
| // Use multiple ManualResetEventSlim to ensure proper ordering | ||
| using ManualResetEventSlim firstTaskReady = new(false); |
There was a problem hiding this comment.
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?
| secondTaskReady.Set(); | ||
| startRequests.Wait(); | ||
| // Add a small delay to ensure this request comes after the first | ||
| Thread.Sleep(50); |
There was a problem hiding this comment.
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.
mdaigle
dismissed stale reviews from cheenamalhotra and paulmedynski
via
5585b10
| // Add a small delay to ensure this request comes after the first. | ||
| // This is necessary because the channel-based pool queues requests in FIFO order, | ||
| // and we need to guarantee the order for this test to be deterministic. | ||
| await Task.Delay(50); |
There was a problem hiding this comment.
This is a tough delay to choose. Is 50ms long enough to guarantee that recycledTask has started its call to TryGetConnection? Hard to say since it depends entirely on scheduling. How obvious will it be if this test fails because failedTask actually calls TryGetConnection first? I'm thinking about future pain diagnosing intermittent test failures. Is there any way to test this scenario without using delays - even if it means adding an internal API that lets you control things explicitly?
There was a problem hiding this comment.
It's hard because there's no way to run something after getting in the queue. The thread or Task is blocked, so we have no way to signal to another thread that our request to the channel is in. Using another thread to do the signaling just introduces another level of scheduling uncertainty. We would need access to the internals of Channel itself.
|
Wow, something is really broken with that latest commit. Occasionally it hangs indefinitely until the stage times out. This should be an interesting one. |
| startRequests.Wait(); | ||
| firstTaskReady.SetResult(true); | ||
| await startRequests.Task; | ||
| pool.TryGetConnection( |
There was a problem hiding this comment.
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()
This reverts commit 5585b10.
| firstTaskReady.Wait(); | ||
| secondTaskReady.Wait(); | ||
|
|
||
| // Use SpinWait to ensure both tasks are actually waiting |
There was a problem hiding this comment.
The comment mentions "Use SpinWait" but the code actually uses Thread.Sleep(100) instead of SpinWait. Consider either:
- Updating the comment to accurately reflect the synchronization mechanism:
// Wait briefly to ensure both tasks are waiting on startRequests - Or using an actual
SpinWaitif that's the intended approach
This comment-code mismatch could confuse future maintainers.
| // Use SpinWait to ensure both tasks are actually waiting | |
| // Wait briefly to ensure both tasks are waiting on startRequests |
This is a test. Using copilot and the github mcp to fix a github issue
Description
This PR fixes two flaky unit tests in
ChannelDbConnectionPoolTestthat were failing intermittently due to race conditions in their synchronization logic.Changes
GetConnectionMaxPoolSize_ShouldRespectOrderOfRequest(sync version)ManualResetEventSlim+CountdownEventsynchronization with more robust coordination using multipleManualResetEventSliminstancesSpinWaitto ensure tasks are properly ready before starting requests[ActiveIssue]attribute - test is now stableGetConnectionAsyncMaxPoolSize_ShouldRespectOrderOfRequest(async version)[ActiveIssue]attribute - test is now stableRoot Cause
The original tests had race conditions where:
Validation
✅ Both tests now pass consistently across all target frameworks:
Total: 30/30 test executions passed (100% success rate)
The tests now reliably validate that
ChannelDbConnectionPoolcorrectly respects FIFO ordering when the pool reaches maximum capacity and connections need to be queued.Fixes
Fixes #3730