From 85c422bacd0744f8a79fd089ae4dab44f0f562a1 Mon Sep 17 00:00:00 2001 From: NathanFallet Date: Thu, 27 Aug 2026 10:57:46 +0200 Subject: [PATCH] fix: stop polling the debug port once the browser has answered MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../kdriver/core/browser/AwaitConnection.kt | 28 ++++++++ .../kdriver/core/browser/DefaultBrowser.kt | 7 +- .../core/browser/AwaitConnectionTest.kt | 67 +++++++++++++++++++ 3 files changed, 98 insertions(+), 4 deletions(-) create mode 100644 core/src/commonMain/kotlin/dev/kdriver/core/browser/AwaitConnection.kt create mode 100644 core/src/jvmTest/kotlin/dev/kdriver/core/browser/AwaitConnectionTest.kt diff --git a/core/src/commonMain/kotlin/dev/kdriver/core/browser/AwaitConnection.kt b/core/src/commonMain/kotlin/dev/kdriver/core/browser/AwaitConnection.kt new file mode 100644 index 000000000..ae9ff2db9 --- /dev/null +++ b/core/src/commonMain/kotlin/dev/kdriver/core/browser/AwaitConnection.kt @@ -0,0 +1,28 @@ +package dev.kdriver.core.browser + +import kotlinx.coroutines.delay + +/** + * Polls [isConnected] until it succeeds, at most [maxTries] times, waiting [intervalMillis] after + * each failed attempt. + * + * Extracted from `DefaultBrowser.start` so the polling rule can be tested without a real browser. + * It used to be written as `repeat(maxTries) { if (testConnection()) return@repeat; delay(…) }`, + * which does not do what it reads like: `return@repeat` returns from the *lambda*, so it is a + * `continue`, not a `break`. The loop therefore never stopped early — and since the skipped line was + * the `delay`, a browser that answered on the first try still got all the remaining attempts fired + * at it back to back, with no wait in between. + * + * @return true as soon as [isConnected] succeeded, false if all [maxTries] attempts failed. + */ +internal suspend fun awaitConnection( + maxTries: Int, + intervalMillis: Long, + isConnected: suspend () -> Boolean, +): Boolean { + repeat(maxTries) { + if (isConnected()) return true + delay(intervalMillis) + } + return false +} diff --git a/core/src/commonMain/kotlin/dev/kdriver/core/browser/DefaultBrowser.kt b/core/src/commonMain/kotlin/dev/kdriver/core/browser/DefaultBrowser.kt index e211166e2..d5f744dc2 100644 --- a/core/src/commonMain/kotlin/dev/kdriver/core/browser/DefaultBrowser.kt +++ b/core/src/commonMain/kotlin/dev/kdriver/core/browser/DefaultBrowser.kt @@ -262,11 +262,10 @@ open class DefaultBrowser( http = HTTPApi(config.host ?: "127.0.0.1", config.port ?: error("Port not set")) delay(config.browserConnectionTimeout) - repeat(config.browserConnectionMaxTries) { - if (testConnection()) return@repeat - delay(config.browserConnectionTimeout) + val connected = awaitConnection(config.browserConnectionMaxTries, config.browserConnectionTimeout) { + testConnection() } - logger.info("Connection to browser established") + if (connected) logger.info("Connection to browser established") val info = info ?: run { // Say what actually failed. Without this the only visible symptom is a 30s wait and a diff --git a/core/src/jvmTest/kotlin/dev/kdriver/core/browser/AwaitConnectionTest.kt b/core/src/jvmTest/kotlin/dev/kdriver/core/browser/AwaitConnectionTest.kt new file mode 100644 index 000000000..cc24b607d --- /dev/null +++ b/core/src/jvmTest/kotlin/dev/kdriver/core/browser/AwaitConnectionTest.kt @@ -0,0 +1,67 @@ +package dev.kdriver.core.browser + +import kotlinx.coroutines.test.runTest +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFalse +import kotlin.test.assertTrue + +class AwaitConnectionTest { + + @Test + fun awaitConnection_whenTheBrowserAnswersImmediately_stopsPolling() = runTest { + var attempts = 0 + + val connected = awaitConnection(maxTries = 60, intervalMillis = 500) { + attempts++ + true + } + + assertTrue(connected) + // The whole point: one answer is enough. The previous `repeat { … return@repeat }` kept + // going for all 60 tries — and skipped the delay while doing so, so the browser got 60 + // requests back to back right after it started. + assertEquals(1, attempts) + } + + @Test + fun awaitConnection_whenTheBrowserAnswersLate_stopsAtThatAttempt() = runTest { + var attempts = 0 + + val connected = awaitConnection(maxTries = 60, intervalMillis = 500) { + attempts++ + attempts >= 3 + } + + assertTrue(connected) + assertEquals(3, attempts) + } + + @Test + fun awaitConnection_whenTheBrowserNeverAnswers_triesExactlyMaxTimes() = runTest { + var attempts = 0 + + val connected = awaitConnection(maxTries = 5, intervalMillis = 500) { + attempts++ + false + } + + assertFalse(connected) + assertEquals(5, attempts) + } + + @Test + fun awaitConnection_waitsBetweenAttempts() = runTest { + var attempts = 0 + val start = testScheduler.currentTime + + awaitConnection(maxTries = 4, intervalMillis = 500) { + attempts++ + false + } + + // One interval per failed attempt, the last one included: that trailing wait is what the + // caller's "waited for N ms" error message counts. + assertEquals(4 * 500L, testScheduler.currentTime - start) + } +}