fix flaky Microsoft.AspNetCore.SignalR.Client.Tests.TestServerTests.WebSocketsWorks#65928
fix flaky Microsoft.AspNetCore.SignalR.Client.Tests.TestServerTests.WebSocketsWorks#65928DeagleGross wants to merge 1 commit intodotnet:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes flakiness in SignalR client TestServerTests by ensuring the in-proc TestServer host isn’t disposed while server-side OnDisconnectedAsync is still running, eliminating ObjectDisposedException errors and unquarantining the affected tests.
Changes:
- Removed quarantine annotations from
WebSocketsWorks()andLongPollingWorks(). - Added explicit
connection.StopAsync()andhost.StopAsync()to drive a graceful shutdown order before disposal.
| await connection.StopAsync(); | ||
| await host.StopAsync(); |
There was a problem hiding this comment.
connection.StopAsync() / host.StopAsync() are only executed on the success path. If an earlier await/Assert throws, cleanup falls back to disposal again and can reintroduce the same race (and can also cause additional error logs during StartVerifiableLog() disposal). Consider moving the explicit stop calls into a finally so they run regardless of test failure.
| await connection.StopAsync(); | ||
| await host.StopAsync(); |
There was a problem hiding this comment.
Same as above: these explicit stop calls only run on the success path. To avoid disposal-time races/log noise when the test fails early, wrap the body in a try/finally and perform connection.StopAsync() / host.StopAsync() in the finally (guarding for partially-initialized state as needed).
BrennanConroy
left a comment
There was a problem hiding this comment.
I don't think this change does anything. We already have await using var connection = ...; which will close the connection before closing the host.
I'm guessing an actual server-side change is needed to improve the lifecycle handling of a closing server.
The
WebSocketsWorks()andLongPollingWorks()tests had a race condition where implicitusing/await usingdisposal could dispose the host'sIServiceProviderwhile the server-sideOnDisconnectedAsyncwas still running, causingObjectDisposedException.Fixes #65914