Fix flaky TestResourceManagerIsSafeForConcurrentAccessAndEnumeration timeout#125573
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-resources |
There was a problem hiding this comment.
Pull request overview
This PR aims to de-flake PreserializedResourceWriterTests.TestResourceManagerIsSafeForConcurrentAccessAndEnumeration by improving how the test waits for concurrent worker tasks to complete, increasing timeout headroom and (intended to) improve exception surfacing during failures.
Changes:
- Converted the test from
voidtoasync Task. - Replaced
Task.WaitAll(..., 30s)withawait Task.WhenAll(...).WaitAsync(120s).
You can also share your feedback on Copilot code review. Take the survey.
src/libraries/System.Resources.Extensions/tests/BinaryResourceWriterUnitTest.cs
Outdated
Show resolved
Hide resolved
…timeout The test uses Task.WaitAll with a 30-second timeout to wait for 10 threads concurrently enumerating resources. On loaded CI agents, 30s is not always enough, causing Assert.True(false) with no useful diagnostic. Switch to await Task.WhenAll().WaitAsync(120s): - WhenAll propagates actual thread-safety exceptions instead of swallowing them behind Assert.True(false) - 120s gives 4x more headroom for slow CI agents while still providing a timeout safety net This test has been filed as a Known Build Error three times (dotnet#80277, dotnet#86013, dotnet#125448) across 3+ years. Fixes dotnet#125448 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
7f04738 to
edc976e
Compare
lewing
left a comment
There was a problem hiding this comment.
Fixed — replaced Task.WhenAll(...).WaitAsync(120s) with Task.WhenAny(Task.WhenAll(tasks), Task.Delay(120s)) pattern that compiles on all TFMs including $(NetFrameworkCurrent). Thanks for catching that!
There was a problem hiding this comment.
Pull request overview
Reduces flakiness in TestResourceManagerIsSafeForConcurrentAccessAndEnumeration by increasing the timeout and improving exception propagation when concurrent enumeration fails.
Changes:
- Makes the test method
async Taskto enable async waiting patterns in xUnit. - Replaces
Task.WaitAll(..., 30s)with an async timeout-based wait aroundTask.WhenAll(...). - Adds a clearer assertion message on timeout and awaits the aggregated task to propagate real exceptions.
You can also share your feedback on Copilot code review. Take the survey.
src/libraries/System.Resources.Extensions/tests/BinaryResourceWriterUnitTest.cs
Show resolved
Hide resolved
src/libraries/System.Resources.Extensions/tests/BinaryResourceWriterUnitTest.cs
Show resolved
Hide resolved
|
/ba-g generic chrome crash. |
|
merging -- i did /ba-g and it still won't pass 5 mins later. |
|
ah, I apparently don't have the power 😐 |
|
/ba-g chrome generic crash. |
Problem
TestResourceManagerIsSafeForConcurrentAccessAndEnumerationis flaky (#125448, 3 hits in 7 days). This same test has been filed as a Known Build Error three times over 3+ years (#80277, #86013, #125448).The test spins up 10 threads that concurrently enumerate a resource set with
Thread.Sleep(1)between entries, then waits with:On loaded CI agents, 30 seconds isn't always enough. The failure message is unhelpful — just
Assert.True() Failure: Expected: True, Actual: False— becauseTask.WaitAllswallows any actual thread-safety exceptions.Fix
Three improvements:
Task.WhenAllinstead ofTask.WaitAll— if a real thread-safety bug is hit, the actual exception propagates instead of being swallowed behindAssert.True(false)Assert.TruefailureThe
WhenAny/Task.Delaypattern is used instead ofTask.WhenAll(...).WaitAsync(...)for .NET Framework TFM compatibility. The method signature changes fromvoidtoasync Task, which xUnit handles natively.Fixes #125448