Repository navigation
Make SSE idle-timeout tests deterministic with FakeTimeProvider - #122
Conversation
KeepAliveComments_PreventIdleTimeout used real wall-clock margins (1 s timeout, 100 ms keep-alive gaps) and failed in CI when thread-pool load delayed the gaps past the timeout. IdleTimeoutStream now takes an optional TimeProvider (default System), exposed internally on SSEClient. The tests advance a FakeTimeProvider only after the reader has provably consumed each push and re-armed, so scheduling cannot fire the timer. KeepAliveComments also asserts the fake clock does fire the timeout once data stops, and SlowButRegularData_DoesNotTimeOut and IdleTimeoutSet_StalledServer use the same clock. No public API or runtime behaviour change.
ricred
left a comment
ricred
left a comment
There was a problem hiding this comment.
Nice change — the ParkedReads handshake is sound: a read only parks after IdleTimeoutStream has re-armed, so the fake clock can't fire the timer early, and the control step in KeepAliveComments_PreventIdleTimeout is a good addition.
Verified locally at dd1279e:
restore --locked-mode+ build: 0 errors, 1 new warning (see inline comment).- SSE + IdleTimeoutStream tests (28): 15 consecutive green runs.
- Mutation check reproduced: with the per-read
idleCts.CancelAfter(idleTimeout)removed,SlowButRegularData_DoesNotTimeOutandKeepAliveComments_PreventIdleTimeoutfail. - Full CI suite not re-run locally (CI is green).
| internal IdleTimeoutStream(Stream inner, TimeSpan idleTimeout) | ||
| /// <param name="timeProvider">Clock driving the idle timer; <see cref="TimeProvider.System" /> when null. | ||
| /// Tests pass a fake provider so the timer only advances when they say so.</param> | ||
| internal IdleTimeoutStream(Stream inner, TimeSpan idleTimeout, TimeProvider? timeProvider = null) |
There was a problem hiding this comment.
Should Fix — TimeProvider? timeProvider = null introduces a new build warning:
CS8632: The annotation for nullable reference types should only be used in code within a '#nullable' annotations context.
The Subscriptions project has no <Nullable> setting or #nullable directive, and main builds it warning-free. Dropping the annotation removes the warning without changing behaviour:
internal IdleTimeoutStream(Stream inner, TimeSpan idleTimeout, TimeProvider timeProvider = null)(Alternatively #nullable enable for this file, but dropping the ? matches the rest of the project.)
There was a problem hiding this comment.
Fixed in 51ce6e1, thanks: dropped the ? to match the rest of the project. A clean build of Linq2GraphQL.Client.Subscriptions is warning-free again.
| // One byte every 500ms (fake clock) for 10s with a 750ms idle timeout: the whole read | ||
| // outlasts the timeout, so it only passes if the timer re-arms per read. The clock only | ||
| // moves inside each read, so scheduling delays cannot fire the timer. | ||
| var time = new FakeTimeProvider(); |
There was a problem hiding this comment.
Optional — NoDataWithinTimeout_FailsWithIOException (line 12) and NonCancelableToken_StillSubjectToIdleTimeout (line 90) still use the real clock with a 100 ms timeout. They only assert that the timer does fire and have a 5 s outer budget, so they can't flake the same way. Was leaving them on the real clock intentional? If you'd like all idle-timeout tests on the same clock, Advance would also make them instant. Fine to ignore.
There was a problem hiding this comment.
Yes, that was intentional. I'd rather keep them on the real clock, for two reasons:
- They can't flake under load. They only assert that the timer does fire, with a 5 s budget for a 100 ms timeout. A busy machine can only delay the timeout, not stop it. The flaky test was the opposite case: it had to prove the timer didn't fire.
- They're the only coverage of the default clock. In production nobody passes a
TimeProvider, soIdleTimeoutStreamusesTimeProvider.System. If every test usedFakeTimeProvider, nothing would check that the default timeout actually works.
So the split is: fake clock where scheduling could break the test, real clock where it can't.
There was a problem hiding this comment.
Good to hear! Everything fine then! Waiting for you to resolve the conflicts with main to get the fix out!
There was a problem hiding this comment.
Conflicts with main are resolved in c5a9d0e. Your per-read timer from ae08e55 is kept as is. The only change on top is that each read's timer is created from the optional TimeProvider, so the fake-clock tests drive it. Since a linked token source can't take a TimeProvider, each read now creates a timer source bound to the provider and links it with the caller's token.
While re-checking after the merge, I found the SSE fake-clock tests still passed with the fake clock disconnected, because their 1 s timeout also ran out on the real clock while the test was waiting. They now use a 30 s timeout, so only the fake clock can fire the timer, and disconnecting it makes both tests fail.
Full CI suite green locally (489 tests); the SSE and idle-timeout tests passed 30 runs in a row.
The Subscriptions project is not in a nullable context, so the '?' raised a warning that main does not have.
…fake-clock # Conflicts: # src/Linq2GraphQL.Client.Subscriptions/IdleTimeoutStream.cs
SseClientTests.KeepAliveComments_PreventIdleTimeoutflaked in CI (PR run 39017126921, "Condition was not met before the timeout") because it relied on real wall-clock margins: a 1 s idle timeout kept alive by: kacomments pushed every 100 ms. When the thread pool is busy, a gap can stretch past 1 s.Change
IdleTimeoutStreamtakes an optionalTimeProvider(defaultTimeProvider.System); its CTS is created with that provider, soCancelAfteruses the provider's timers.SSEClientexposes it as aninternalinit property. No public API or runtime behaviour change.FakeTimeProvider(new test-only packageMicrosoft.Extensions.TimeProvider.Testing10.10.0; lock file refreshed).ScriptableSseStream.ParkedReadscounts reads parked on an empty queue. Tests advance the clock only after the reader has consumed a push and issued its next read, so the timer has provably re-armed and scheduling delays can't fire it.KeepAliveComments_PreventIdleTimeout(15 × 900 ms fake time with a 1 s timeout, plus a control step checking that silence past the timeout does fail the stream),IdleTimeoutSet_StalledServer_FailsTheStream, andIdleTimeoutStreamTests.SlowButRegularData_DoesNotTimeOut, which had the same real-clock margin pattern.Verification
Linq2GraphQL.CI.slnfsuite passed (446 tests).Task.Delay(100)calls only sequence a server abort and don't assert a timing margin, so they're unchanged.