diff --git a/src/main/kotlin/com/gitvantage/git/Git.kt b/src/main/kotlin/com/gitvantage/git/Git.kt index 602e730..e75572f 100644 --- a/src/main/kotlin/com/gitvantage/git/Git.kt +++ b/src/main/kotlin/com/gitvantage/git/Git.kt @@ -5,8 +5,13 @@ package com.gitvantage.git import com.gitvantage.model.Activity import com.gitvantage.model.Perf +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.delay +import kotlinx.coroutines.launch +import kotlinx.coroutines.runBlocking import java.io.File import java.util.concurrent.TimeUnit +import kotlin.time.Duration.Companion.seconds /** * The one place the app spawns `git`. @@ -189,6 +194,12 @@ object Git { * malloc and Objective-C locks the UI thread needs, so the app grew laggier the longer it ran * (and the bigger the process got). Mutations through [run] keep the default; a commit or a * switch is expected to write the index, and the app is the one asking. + * + * [timeoutSeconds] is a second coroutine beside the read, which kills git *and everything it + * started* when time runs out and is cancelled when the read finishes first. It used to be a `waitFor(timeout)` after reading stdout to EOF — which a hung + * git never reaches, so no timeout ever fired: a field report had a `fetch` running for 2h 11m + * against its 90s. The children matter as much as git: a fetch's `ssh`, or the shell behind an + * alias, holds the same stdout, and the read only ends when the last of them lets go. */ private fun spawnNow(dir: File, args: List, timeoutSeconds: Long, captureErr: Boolean, readOnly: Boolean): Result = runCatching { val pb = ProcessBuilder(listOf("git") + args).directory(dir) @@ -202,14 +213,38 @@ object Git { .apply { isDaemon = true; start() } } else null - val out = proc.inputStream.bufferedReader().readText() - val finished = proc.waitFor(timeoutSeconds, TimeUnit.SECONDS) - if (!finished) proc.destroyForcibly() + var timedOut = false + // runBlocking returns only once the timeout coroutine has finished, cancelled or not, which + // also makes its write to `timedOut` visible here. + val out = runBlocking { + // Not on runBlocking's own thread: that thread is about to block in the read, and a + // timeout waiting for it to be free would never fire. + val timeout = launch(Dispatchers.Default) { + delay(timeoutSeconds.seconds) + timedOut = true + killTree(proc) + } + try { + proc.inputStream.bufferedReader().readText().also { proc.waitFor() } + } finally { + timeout.cancel() + } + } // join() also publishes the pump thread's write to `err` to this thread. pump?.join(TimeUnit.SECONDS.toMillis(2)) - if (finished) Result(proc.exitValue(), out, err) else Result(-1, out, "timed out") + if (timedOut) Result(-1, out, "timed out") else Result(proc.exitValue(), out, err) }.getOrElse { e -> Result(-1, "", e.message ?: "failed to run git") } + + /** + * Kill [proc] and its descendants. Children first, listed while git is still alive: once git + * dies they are reparented, stop being its descendants, and keep the pipe open with nobody + * left to find them. + */ + private fun killTree(proc: Process) { + runCatching { proc.descendants().forEach { it.destroyForcibly() } } + proc.destroyForcibly() + } } diff --git a/src/test/kotlin/com/gitvantage/git/GitPerfTest.kt b/src/test/kotlin/com/gitvantage/git/GitPerfTest.kt index d11d2c6..a470c93 100644 --- a/src/test/kotlin/com/gitvantage/git/GitPerfTest.kt +++ b/src/test/kotlin/com/gitvantage/git/GitPerfTest.kt @@ -32,3 +32,30 @@ val GitPerfCounters by testSuite { } } } + +/** + * A git that never finishes is killed when its time is up, with whatever it started. + * + * The timeout used to be checked only after stdout reached EOF, which a hung git never gets to, so + * a stalled `fetch` ran for hours. The alias runs `sleep` under a shell: a grandchild holding git's + * stdout, the way a fetch's `ssh` does, so killing git alone would still leave the read waiting. + */ +val GitTimeout by testSuite { + testFixture { Sandbox() } asContextForEach { + + test("a hung git and its children are killed at the timeout") { + val work = repo() + val started = System.nanoTime() + val out = Git.readOrNull(work, "-c", "alias.hang=!sleep 60", "hang", timeoutSeconds = 1) + val tookMs = (System.nanoTime() - started) / 1_000_000 + + assert(out == null) { "a timed-out git read as success: '$out'" } + assert(tookMs < 10_000) { "the 1s timeout took ${tookMs}ms to fire" } + } + + test("a git that finishes in time is unaffected") { + val work = repo() + assert(Git.readOrNull(work, "rev-parse", "--is-inside-work-tree", timeoutSeconds = 5)?.trim() == "true") + } + } +}