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 @@ -362,7 +362,13 @@ open class DefaultBrowser(
logger.debug("Closing browser gracefully...")
withTimeoutOrNull(5.seconds) { connection?.browser?.close() }
logger.debug("Killing browser process...")
process?.destroy()
// Wait for it to actually be gone, don't just ask. destroy() returns immediately and does not
// reach the browser's child processes, which keep open handles on the profile directory —
// a browser started on the same --user-data-dir right after would hang before opening its
// debug port, and the start would time out for no visible reason.
process?.let {
if (!it.destroyAndAwaitExit()) logger.warn("Browser process ${it.pid()} still alive after kill")
}
process = null
logger.debug("Closing connection...")
connection?.close()
Expand Down
50 changes: 50 additions & 0 deletions core/src/commonMain/kotlin/dev/kdriver/core/browser/ProcessExit.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
package dev.kdriver.core.browser

import kotlinx.coroutines.delay

/**
* Forcibly kills the process and, where the platform supports it, its whole tree.
*
* [Process.destroy] only asks politely (SIGTERM / TerminateProcess on the top-level process). On
* Windows in particular, a browser's renderer and GPU children survive it.
*/
expect fun Process.killTree()

/**
* Terminates the process, then waits until it has actually gone.
*
* [Process.destroy] only *requests* termination: it returns immediately, and it does not reach the
* browser's child processes. Those children keep open handles on the profile directory, so a browser
* started on the same `--user-data-dir` shortly afterwards cannot take ownership of it: it hangs
* before opening its debug port, and the start times out with no visible cause. Callers therefore
* need to know the browser is really gone, not merely that it was asked to leave.
*
* Grace-waits for [gracePeriodMillis], then escalates to [killTree] and waits up to
* [killTimeoutMillis] more. The same shape as zendriver's `Browser.stop`.
*
* @return true if the process exited, false if it was still alive when the timeouts elapsed.
*/
suspend fun Process.destroyAndAwaitExit(
gracePeriodMillis: Long = 3_000,
killTimeoutMillis: Long = 5_000,
): Boolean {
if (!isAlive()) return true

destroy()
return awaitExit(gracePeriodMillis) || run {
killTree()
awaitExit(killTimeoutMillis)
}
}

private suspend fun Process.awaitExit(timeoutMillis: Long): Boolean {
var waited = 0L
while (waited < timeoutMillis) {
if (!isAlive()) return true
delay(EXIT_POLL_INTERVAL_MILLIS)
waited += EXIT_POLL_INTERVAL_MILLIS
}
return !isAlive()
}

private const val EXIT_POLL_INTERVAL_MILLIS = 100L
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
@@ -1,4 +1,4 @@
package dev.kdriver.core.browser

Check warning on line 1 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#L1

File 'core\src\jsMain\kotlin\dev\kdriver\core\browser\Process.js.kt' with '11' functions detected. Defined threshold inside files is set to '11'. (detekt.TooManyFunctions)

import kotlinx.io.files.Path

Expand All @@ -18,6 +18,10 @@
throw UnsupportedOperationException()
}

actual fun Process.killTree() {

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 killTree is missing documentation. (detekt.UndocumentedPublicFunction)
throw UnsupportedOperationException()
}

actual suspend fun startProcess(
exe: Path,
params: List<String>,
Expand Down
13 changes: 13 additions & 0 deletions core/src/jvmMain/kotlin/dev/kdriver/core/browser/Process.jvm.kt
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package dev.kdriver.core.browser

Check warning on line 1 in core/src/jvmMain/kotlin/dev/kdriver/core/browser/Process.jvm.kt

View check run for this annotation

codefactor.io / CodeFactor

core/src/jvmMain/kotlin/dev/kdriver/core/browser/Process.jvm.kt#L1

File 'core\src\jvmMain\kotlin\dev\kdriver\core\browser\Process.jvm.kt' with '11' functions detected. Defined threshold inside files is set to '11'. (detekt.TooManyFunctions)

import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.runBlocking
Expand Down Expand Up @@ -92,6 +92,19 @@
}
}

/**
* Kills this process and every descendant.
*
* `Process.destroyForcibly()` on its own only reaches the top-level process; on Windows the browser's
* renderer and GPU children outlive it and keep holding the profile's files. Descendants are snapshot
* first, because killing the parent detaches them and they can no longer be enumerated.
*/
actual fun Process.killTree() {
val descendants = runCatching { toHandle().descendants().toList() }.getOrDefault(emptyList())
destroyForcibly()
descendants.forEach { runCatching { it.destroyForcibly() } }
}

actual fun freePort(): Int? {
ServerSocket(0, 5, InetAddress.getByName("127.0.0.1")).use { socket ->
return socket.localPort
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package dev.kdriver.core.browser

Check warning on line 1 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#L1

File 'core\src\mingwMain\kotlin\dev\kdriver\core\browser\Process.mingw.kt' with '11' functions detected. Defined threshold inside files is set to '11'. (detekt.TooManyFunctions)

import kotlinx.cinterop.*
import kotlinx.coroutines.Dispatchers
Expand Down Expand Up @@ -53,6 +53,19 @@
*/
actual suspend fun Process.readStderrSnapshot(maxBytes: Int, timeoutMillis: Long): String? = null

/**
* Kills this process.
*
* `TerminateProcess` already ends it outright, so there is nothing gentler to escalate from here.
* Note that it does **not** reach the process's children: enumerating them on Windows means walking
* 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.
*/
@OptIn(ExperimentalForeignApi::class)
actual fun Process.killTree() {
processHandle?.let { if (isAlive()) TerminateProcess(it, 1u) }
}

@OptIn(ExperimentalForeignApi::class)
actual suspend fun startProcess(
exe: Path,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,17 @@ actual abstract class Process {

actual suspend fun Process.readStderrSnapshot(maxBytes: Int, timeoutMillis: Long): String? = null

/**
* Kills this process with SIGKILL.
*
* POSIX has no cheap, portable way to enumerate descendants, so only the process itself is signalled.
* That is enough here: on Linux and macOS the browser's children terminate with their parent, unlike
* on Windows.
*/
actual fun Process.killTree() {
if (isAlive()) kill(processIdentifier, SIGKILL)
}

actual fun addShutdownHook(hook: suspend () -> Unit) {
// POSIX doesn't have a direct equivalent to Java shutdown hooks
// Could use atexit() but it doesn't support suspend functions
Expand Down
Loading