Bound the idle wait so a busy page cannot hang select() - #87
Merged
Conversation
DefaultTab.select()/find()/findAll() open with wait(), which lands in the t == null branch of DefaultConnection.wait() — the one commented "Wait indefinitely". Its only exit is 100 ms without a single CDP event, so on a page that streams continuously (polling, SSE, ads, a refreshing interstitial) it never returns, and select() blocks before it ever evaluates its own timeout. The timeout parameter is inert in that state, including timeout = 0. Two changes: - wait() takes an idleTimeout, capping how long it watches for the connection to settle. t stays a floor and keeps its meaning, so callers asking for breathing room still get it. Splitting the two makes the signature say what the body used to hide: one number served as both cap and floor, which is why the two branches existed at all. They are now one path. - Idleness is read from the timestamp of the last frame the receive loop saw, instead of re-subscribing to events on every poll. The old check only ever observed the 100 ms window it had just opened, so a connection that had been quiet for a while still looked busy, and a busy one could look quiet. A clock seam mirrors the existing createTransport() seam so the tests decide "traffic kept arriving" and "nothing arrived" off the scheduler's virtual clock rather than off how fast the machine runs.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DefaultTab.select()/find()/findAll()all open withwait(), which lands in thet == nullbranch ofDefaultConnection.wait()— the one commented "Wait indefinitely". Its only exit is 100 ms without a single CDP event.On a page that streams continuously — polling, server-sent events, ads, a refreshing interstitial — that quiet window never comes, so
wait()never returns andselect()blocks before it ever evaluates its own timeout. Thetimeoutparameter is inert in that state,timeout = 0included.Changes
wait()takes anidleTimeout, capping how long it watches for the connection to settle. Waiting is best-effort — a page that never settles is not an error — so past the bound it stops watching and returns.tstays a floor and keeps its meaning. Splitting the two makes the signature say what the body used to hide: one number served as both cap and floor, which is why the two branches existed. They are now a single path.ta non-null default does not work. Thet != nullbranch guarantees a wait of at leastt, andselect()callswait()once per poll — every selector would cost that much. It would trade a hang for a slowdown.Idleness is now read, not guessed. The receive loop timestamps the last frame it saw, and
isIdle()compares against it. The previous check re-subscribed toeventson every poll and only observed the 100 ms window it had just opened, so a connection quiet for a while still looked busy, and a busy one could look quiet.Tests
A clock seam mirrors the existing
createTransport()seam, so the tests decide "traffic kept arriving" and "nothing arrived" off the scheduler's virtual clock rather than off how fast the machine runs.wait()returns, and only at the boundwait(t)on an already-idle connection — the floor is still honouredAll three fail on
main. Mutation-checked: removing the bound, removing the floor, never writing the timestamp, treating "nothing received" as busy, and dropping the idle threshold each turn one red.Full
:core:jvmTest+:opentelemetry:jvmTestgreen (184 tests).:cdp:detektandkotlinStoreYarnLockfail identically onmainand are untouched;:coredetekt violations are unchanged at 142.