Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -275,11 +275,13 @@ open class DefaultBrowser(
val waitedMs = config.browserConnectionTimeout * (config.browserConnectionMaxTries + 1)
val stderr = process?.readStderrSnapshot()
logger.error(
"Browser never opened its debug port on ${config.host}:${config.port} after ${waitedMs}ms " +
"(pid=${process?.pid()}, alive=${process?.isAlive()}). " +
"Last connection error: " +
(lastConnectionError?.let { "${it::class.simpleName}: ${it.message}" } ?: "none") +
". Browser stderr: " + (stderr?.trim()?.takeIf { it.isNotEmpty() } ?: "<none>")
browserStartFailureMessage(
endpoint = "${config.host}:${config.port}",
waitedMs = waitedMs,
fate = processFate(process?.pid(), process?.isAlive() ?: false, process?.exitCodeOrNull()),
lastConnectionError = lastConnectionError,
stderr = stderr,
)
)
stop()
throw FailedToConnectToBrowserException()
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
package dev.kdriver.core.browser

/**
* Reports the exit status of a process that has already terminated, or null if it is still running
* (or the platform cannot tell).
*
* Paired with [Process.isAlive]: `isAlive() == false` says the browser is gone, this says *how* it
* went. The distinction is the whole point — on Windows a Chrome that finds a live instance on the
* same `--user-data-dir` hands its command line over and exits **0**, silently, without ever opening
* a debug port; a Chrome that crashed exits non-zero. Both look identical without this.
*/
expect fun Process.exitCodeOrNull(): Int?

/**
* Describes what became of the browser process.
*
* Reports the exit status as-is rather than interpreting it: the whole reason for reading it is that
* we do not yet know which statuses a browser that never opened its debug port exits with. Guessing
* here would put an unverified claim into every log line.
*
* @param pid the browser's process id, if we had one.
* @param alive whether the process was still running when we gave up.
* @param exitCode its exit status, or null if it is alive or the platform cannot tell.
*
* @return a fragment such as `pid=1916, exited with 21`, meant to be embedded in a larger message.
*/
internal fun processFate(pid: Long?, alive: Boolean, exitCode: Int?): String = "pid=$pid, " + when {
alive -> "still running"
exitCode == null -> "already gone (exit status unavailable)"
else -> "exited with $exitCode"
}

/**
* Builds the message logged when a browser never opened its debug port.
*
* Pure and separated from [DefaultBrowser.start] so the wording is covered by tests on any OS — the
* facts it reports come from platform calls that cannot be exercised in CI.
*
* @param endpoint the address the debug port was expected on.
* @param waitedMs how long we waited for it.
* @param fate the process's id and what became of it — see [processFate].
* @param lastConnectionError the last failure seen while polling, if any.
* @param stderr whatever the browser wrote to stderr, if anything.
*
* @return a single line naming the endpoint, the process's fate, and the last connection error.
*/
internal fun browserStartFailureMessage(
endpoint: String,
waitedMs: Long,
fate: String,
lastConnectionError: Throwable?,
stderr: String?,
): String = "Browser never opened its debug port on $endpoint after ${waitedMs}ms ($fate). " +
"Last connection error: " +
(lastConnectionError?.let { "${it::class.simpleName}: ${it.message}" } ?: "none") +
". Browser stderr: " + (stderr?.trim()?.takeIf { it.isNotEmpty() } ?: "<none>")
4 changes: 4 additions & 0 deletions core/src/jsMain/kotlin/dev/kdriver/core/browser/Process.js.kt
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@
throw UnsupportedOperationException()
}

actual fun Process.exitCodeOrNull(): Int? {

Check warning on line 21 in core/src/jsMain/kotlin/dev/kdriver/core/browser/Process.js.kt

View check run for this annotation

codefactor.io / CodeFactor

core/src/jsMain/kotlin/dev/kdriver/core/browser/Process.js.kt#L21

The function exitCodeOrNull is missing documentation. (detekt.UndocumentedPublicFunction)
throw UnsupportedOperationException()
}

actual fun Process.killTree() {
throw UnsupportedOperationException()
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,3 +120,11 @@ actual fun defaultBrowserSearchConfig(): BrowserSearchConfig {
else -> BrowserSearchConfig(File.pathSeparator, searchWindowsProgramFiles = true)
}
}

/**
* Reads the exit status, or null while the process is still running.
*
* `exitValue()` throws `IllegalThreadStateException` rather than returning a sentinel when the
* process is alive, which is exactly the "still running" case we report as null.
*/
actual fun Process.exitCodeOrNull(): Int? = runCatching { exitValue() }.getOrNull()
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
package dev.kdriver.core.browser

import kotlin.test.Test
import kotlin.test.assertTrue

class StartFailureMessageTest {

private fun message(
alive: Boolean = false,
exitCode: Int? = null,
stderr: String? = null,
error: Throwable? = null,
) = browserStartFailureMessage(
endpoint = "127.0.0.1:53997", waitedMs = 30500,
fate = processFate(1916, alive, exitCode), lastConnectionError = error, stderr = stderr,
)

@Test
fun message_alwaysNamesTheEndpointTheWaitAndThePid() {
val m = message()
assertTrue("127.0.0.1:53997" in m, m)
assertTrue("30500ms" in m, m)
assertTrue("pid=1916" in m, m)
}

/**
* Reported as-is, with no interpretation: we are reading the exit status precisely because we do
* not know yet which statuses this failure produces. A zero must not be dressed up as anything.
*/
@Test
fun message_reportsTheExitStatusVerbatim() {
assertTrue("exited with 0" in message(exitCode = 0), message(exitCode = 0))
assertTrue("exited with 21" in message(exitCode = 21), message(exitCode = 21))
}

@Test
fun message_whenTheBrowserIsStillRunning_saysSoRatherThanGuessing() {
val m = message(alive = true, exitCode = null)
assertTrue("still running" in m, m)
}

@Test
fun message_whenTheExitStatusIsUnavailable_doesNotClaimACleanExit() {
val m = message(alive = false, exitCode = null)
assertTrue("exit status unavailable" in m, m)
assertTrue("exited with" !in m, "an unknown status must not be reported as an exit code: $m")
}

@Test
fun message_reportsStderrWhenThereIsSomeAndSaysSoWhenThereIsNot() {
assertTrue("<none>" in message(stderr = " "), "blank stderr reads as none")
assertTrue("Fontconfig error" in message(stderr = "Fontconfig error\n"), "stderr is quoted")
}

@Test
fun message_reportsTheLastConnectionError() {
val m = message(error = IllegalStateException("Connection refused"))
assertTrue("IllegalStateException: Connection refused" in m, m)
assertTrue("none" in message(error = null), "says none when there was no error")
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,23 @@
* a `CreateToolhelp32Snapshot` by parent id, which this target does not do — the JVM target, which is
* the one running browsers in production, uses `ProcessHandle.descendants()` for that.
*/
/**
* Reads the exit status via `GetExitCodeProcess`, the same call [Process.isAlive] uses — null while
* it still reports `STILL_ACTIVE`.
*/
@OptIn(ExperimentalForeignApi::class)
actual fun Process.exitCodeOrNull(): Int? {
val handle = processHandle ?: return null
val code = memScoped {
val c = alloc<DWORDVar>()
GetExitCodeProcess(handle, c.ptr)
c.value
}
return if (code == STILL_ACTIVE) null else code.toInt()
}

@OptIn(ExperimentalForeignApi::class)
actual fun Process.killTree() {

Check warning on line 80 in core/src/mingwMain/kotlin/dev/kdriver/core/browser/Process.mingw.kt

View check run for this annotation

codefactor.io / CodeFactor

core/src/mingwMain/kotlin/dev/kdriver/core/browser/Process.mingw.kt#L80

The function killTree is missing documentation. (detekt.UndocumentedPublicFunction)
processHandle?.let { if (isAlive()) TerminateProcess(it, 1u) }
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,3 +101,9 @@ actual fun tempProfileDir(): Path {

return Path(profilePath)
}

/**
* Not available on this target: liveness here is `kill(pid, 0)`, which says whether the process
* exists but never carries its exit status, and nothing reaps the child to collect one.
*/
actual fun Process.exitCodeOrNull(): Int? = null
Loading