fix: stop polling the debug port once the browser has answered - #85
Merged
Merged
Conversation
The wait loop in start() read as "try until it connects, up to maxTries times":
repeat(config.browserConnectionMaxTries) {
if (testConnection()) return@repeat
delay(config.browserConnectionTimeout)
}
`return@repeat` returns from the lambda, not from the loop — it is a `continue`,
not a `break`. So the loop never stopped early, and because the line it skipped
was the `delay`, a browser that answered on the very first try still got all 60
remaining attempts fired at it back to back with no wait in between. That is on
the *successful* path, not an error path: every single browser start did it.
Measured on a standalone reproduction of the loop: 60 calls to testConnection(),
0 delays.
The polling rule moves to awaitConnection() so it can be tested without a real
browser, and start() now logs "Connection to browser established" only when the
connection was actually established — it used to log it unconditionally, one
line above the error saying the browser never opened its port.
No change to the timeouts themselves: 60 tries x 500ms is still 30s of budget.
Worth revisiting separately (zendriver waits 2.5s, pydoll 10s), but that is a
behaviour change and this is a bug fix.
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.
The bug
The wait loop in
DefaultBrowser.start()reads as "try until it connects, up tomaxTriestimes":delay(config.browserConnectionTimeout) repeat(config.browserConnectionMaxTries) { if (testConnection()) return@repeat delay(config.browserConnectionTimeout) }return@repeatreturns from the lambda, not from the loop — it is acontinue, not abreak. The loop never stopped early. And because the line it skipped is thedelay, a browser that answered on the first try still got every remaining attempt fired at it back to back, with no wait in between.This is the successful path, not an error path. Every browser start did it.
Standalone reproduction of the loop, run as-is:
So with the defaults, each successful start sends 60
GET /json/versionin a burst at a Chrome that has just come up.The fix
The polling rule moves to
awaitConnection(), so it can be tested without a real browser, and uses a genuine early return.start()also now logs"Connection to browser established"only when the connection actually was established. It used to log it unconditionally — one line above the error saying the browser never opened its port, which is confusing when reading logs of a failed start.Not changed
The timeouts themselves. 60 tries × 500 ms is still 30 s of budget. It is worth revisiting (zendriver waits 2.5 s, pydoll 10 s), but that is a behaviour change and this is a bug fix — happy to do it in a follow-up.
Tests
AwaitConnectionTest, written red first against a copy of the old loop:maxTriesattemptsThe last two passed against the buggy version too — they pin the behaviour that must not change, so the fix cannot trade one bug for another.
Locally: all five targets compile,
:core:jvmTestgreen, detekt findings identical tomain(317, same set).