kotlin: cancelling a compile now returns at once, instead of waiting for kotlinc - #642
Open
oyvindberg wants to merge 2 commits into
Open
kotlin: cancelling a compile now returns at once, instead of waiting for kotlinc#642oyvindberg wants to merge 2 commits into
oyvindberg wants to merge 2 commits into
Conversation
…for kotlinc The 120s I put on these tests in the previous commit was wrong, and hid a real bug. Cancelling a compile should cost about nothing; a bound that tolerates two minutes asserts the opposite. Setting the bound to what it should be — the test's own sleep plus a 200ms cancellation budget — isolated it immediately. Scala returns in 162ms and its mid-compile ProgressCallback variant in 4ms; Java likewise. Only Kotlin failed, and it failed by taking as long as the whole compile. The cause: kotlinc observes cancellation only when it calls back into our message collector or polls the `Services` status, and between those points it is unreachable — `Thread.interrupt` sets a flag nothing checks. `compileIncremental` then invokes the compiler ON THE CALLING THREAD, so there was nothing to interrupt into: `IO.interruptible` cancellation could not return until the compile had finished by itself. `compileWithReflection` accidentally did better, because it parks the caller in an interruptible `join`. That also explains why this surfaced now rather than earlier. Before #625 the incremental runner never resolved, so every Kotlin compile took the reflection path. Making incremental compilation actually engage moved these compiles onto the path with no way out. `runCancellably` gives the compile its own thread and has the caller wait on a latch either side can trip — `cancellation.onCancel` for one side, completion for the other. A cancelled compile ABANDONS that thread rather than joining it, the same trade `Outcome.runInFreshThread` documents for native compilers that ignore interrupts: the work is wasted either way, and the alternative is making the user wait for output nobody wants. Daemon thread, so an abandoned one cannot hold the JVM open. Both paths go through it, since the fallback deserves the same guarantee. Note it cannot reproduce on a fast machine: the compile finishes inside any generous bound, which is exactly how a 30s timeout passed for so long while being wrong. The tight bound is what makes the behaviour observable at all, so it is the point of the change rather than incidental to it. Per-site budgets rather than one flat number, because the tests do not all wait the same time before cancelling — one deliberately sleeps 500ms to get well into a compile, and a flat total would assert something weaker for it than for the others. Verified: CancellationTest 7/7, three consecutive runs, and the suite got FASTER (2314ms -> 1578ms) because the abandoned compile no longer runs to completion. Full bleep-bsp-tests minus the linker suites: 618 passed, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Abandoning a thread is not the same as it stopping, and the previous commit did not say which happens. It is cooperative: kotlinc calls `checkCanceled` at phase boundaries and our `CompilationCanceledStatus` proxy throws there, so most abandoned compiles stop shortly after the cancel. But that is cooperation, not a guarantee — one inside a stretch that polls neither the status nor the message collector runs to the end, still writing class files into an output directory after we have reported CompilationCancelled. That hazard is real and this repo has already been bitten by it: #626 fixed a teardown that raced "a kotlinc still emitting into it", which is the same thing one level up. So the threads are tracked, exactly as ZincBridge.abandonedEcjThreads tracks the ECJ equivalent — identity set, self-removing on completion, snapshot for diagnostics. A non-empty snapshot during a build means something is writing into a directory nobody is waiting for, which is worth being able to see rather than inferring from corrupted output. Not solved here, and worth stating: nothing stops a NEW compile of the same project from starting while an abandoned one still writes. ProjectLock serializes compiles, but the abandoned thread does not hold it. The alternatives are to keep holding the lock until the runaway finishes (correct, but reintroduces the wait for the next compile rather than for the cancel) or to compile into a scratch directory and publish atomically. Both are larger than this fix, and the existing ECJ and native-compiler paths make the same trade today. CancellationTest 7/7. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
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
1 participant
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.
A regression that shipped in M11, found because the flaky-test fix in #641 was wrong.
That PR bumped these tests to a 120s timeout. Papering over — cancelling a compile should cost about nothing, and a bound tolerating two minutes asserts the opposite.
Setting the bound to what it should be isolated it immediately
Each test's own sleep plus a 200ms cancellation budget:
Root cause
kotlinc observes cancellation only when it calls back into our message collector or polls the
Servicesstatus. Between those points it is unreachable —Thread.interruptsets a flag nothing checks.compileIncrementalthen invokes the compiler on the calling thread, so there was nothing to interrupt into:IO.interruptiblecancellation could not return until the compile finished by itself.compileWithReflectionaccidentally did better, because it parks the caller in an interruptiblejoin.That also explains the timing. Before #625 the incremental runner never resolved, so every Kotlin compile took the reflection path. Making incremental compilation actually engage moved these compiles onto the path with no way out. So this is a real regression in M11 that surfaced only as a flaky test — a user pressing Ctrl-C on a Kotlin build waits for the whole compile.
Fix
runCancellablygives the compile its own thread and has the caller wait on a latch either side can trip —cancellation.onCancelfor one, completion for the other. A cancelled compile abandons that thread rather than joining it, the same tradeOutcome.runInFreshThreaddocuments for native compilers that ignore interrupts: the work is wasted either way, and the alternative is making the user wait for output nobody wants. Daemon thread, so an abandoned one cannot hold the JVM open. Both paths go through it, since the fallback deserves the same guarantee.Test bounds
Per-site budgets rather than one flat number: the tests do not all wait the same time before cancelling — one deliberately sleeps 500ms to get well into a compile — and a flat total would assert something weaker for it than for the others. (I got this wrong first time round and it produced a spurious failure, which is what surfaced the distinction.)
This cannot reproduce on a fast machine: the compile finishes inside any generous bound, which is exactly how a 30s timeout passed for so long while being wrong. The tight bound is what makes the behaviour observable, so it is the point of the change rather than incidental.
Verification
CancellationTest7/7, three consecutive runsbleep-bsp-testsminus linker suites: 618 passed, 0 failed🤖 Generated with Claude Code