Fix #12761: do not wait for the terminal on the thread building it - #12814
Merged
Conversation
MessageUtils.systemInstall publishes the FastTerminal before the background thread has built the real one, and every FastTerminal method delegates through getTerminal(). Anything that thread logs is therefore rendered through a terminal that same thread is still producing, and parks on its own future. There is no lock cycle, so jstack reports no deadlock. Both the builder callable and the consumer callback are inside that window: the consumer runs before terminal.complete(term). Two distinct call paths reach it from a single log statement, the level rendering via toAnsi and the log sink via terminal.writer(), so guarding one call site is not enough. getTerminal() hands the building thread a dumb stand-in instead, which unblocks every delegating method at once. Do not narrow this to a plain-text fallback in the message builder: the writer path would still park. The stand-in writes to the System.err captured at construction, not the live one, which the consumer replaces with a logging-backed stream that would feed the output back into the logger it came from. Rendering degrades to unstyled text for the duration of the build, which is what a dumb terminal would produce anyway. The build thread also becomes a daemon, so a wedged build cannot keep the JVM alive. JLine 4.x initialises its native loader during provider probing and logs a warning when a library candidate fails, which is what made this reachable after the 3.30.6 to 4.3.1 bump in 4.0.0-rc-6. The failure itself is harmless, JLine recovers by extracting the bundled library.
slachiewicz
requested review from
cstamas and
gnodet
and removed request for
gnodet
August 25, 2026 00:36
slachiewicz
marked this pull request as ready for review
August 25, 2026 00:39
This was referenced Aug 25, 2026
Handing the building thread a DumbTerminal meant constructing a terminal inside terminal construction, on that same thread, in a failure path. It wedged on ubuntu/jdk-17 when the stand-in was built after the consumer had already run AnsiConsole.systemInstall, while passing everywhere else. A log statement asks the terminal for exactly two things, its type while rendering and its writer while emitting. AttributedCharSequence.toAnsi asks for the type first and renders plain for a dumb one, so it never reaches any other method. Answering those two with a constant and a PrintWriter over the captured stream covers the same paths and cannot block. The tests now exercise both calls from both halves of the window rather than one call each.
gnodet
approved these changes
Aug 25, 2026
gnodet
left a comment
gnodet
left a comment
Contributor
There was a problem hiding this comment.
Well-analyzed fix for a real deadlock (#12761) where the FastTerminal build thread parks on its own CompletableFuture when JLine 4.x logs during terminal construction. The re-entrancy detection is correct, the two guarded methods cover the complete call surface, thread safety is sound, and the tests are thorough.
Highlights:
- The fix correctly identifies the two terminal methods reachable from a log statement during construction:
getType()(viaAttributedCharSequence.toAnsi) andwriter()(via the log sink installed byAnsiConsole.systemInstall). The analysis of why no other methods need guarding is confirmed by readingJLineMessageBuilderFactory.build()and the JLinetoAnsicontract. - Thread-safety on
fallbackWriteris sound:isBuildThreadWaitingOnItself()returns true only whenThread.currentThread() == buildThread && !terminal.isDone(), sofallbackWriter()is strictly single-thread-accessed. ThefallbackOutputfield is final and set beforeThread.start(), establishing a happens-before guarantee. - Making the build thread a daemon is a good safety improvement.
- The tests are well-designed:
assertTimeoutPreemptivelyuses thread interruption so a regression produces a red test rather than a hung fork.
📋 PR Metadata
| Aspect | Current | Suggested |
|---|---|---|
| Milestone | (none) | 4.0.0-rc-7 |
🔀 Backport Status
The backport-to-4.0.x label is present — no backport PR found yet, which is expected since this PR was just created. The PR description confirms maven-4.0.x carries the same code and JLine version.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
gnodet
added a commit
that referenced
this pull request
Aug 27, 2026
…12814) (#12854) * Fix #12761: do not wait for the terminal on the thread building it MessageUtils.systemInstall publishes the FastTerminal before the background thread has built the real one, and every FastTerminal method delegates through getTerminal(). Anything that thread logs is therefore rendered through a terminal that same thread is still producing, and parks on its own future. There is no lock cycle, so jstack reports no deadlock. Both the builder callable and the consumer callback are inside that window: the consumer runs before terminal.complete(term). Two distinct call paths reach it from a single log statement, the level rendering via toAnsi and the log sink via terminal.writer(), so guarding one call site is not enough. getTerminal() hands the building thread a dumb stand-in instead, which unblocks every delegating method at once. Do not narrow this to a plain-text fallback in the message builder: the writer path would still park. The stand-in writes to the System.err captured at construction, not the live one, which the consumer replaces with a logging-backed stream that would feed the output back into the logger it came from. Rendering degrades to unstyled text for the duration of the build, which is what a dumb terminal would produce anyway. The build thread also becomes a daemon, so a wedged build cannot keep the JVM alive. JLine 4.x initialises its native loader during provider probing and logs a warning when a library candidate fails, which is what made this reachable after the 3.30.6 to 4.3.1 bump in 4.0.0-rc-6. The failure itself is harmless, JLine recovers by extracting the bundled library. * Answer the two terminal calls directly instead of standing in a terminal Handing the building thread a DumbTerminal meant constructing a terminal inside terminal construction, on that same thread, in a failure path. It wedged on ubuntu/jdk-17 when the stand-in was built after the consumer had already run AnsiConsole.systemInstall, while passing everywhere else. A log statement asks the terminal for exactly two things, its type while rendering and its writer while emitting. AttributedCharSequence.toAnsi asks for the type first and renders plain for a dumb one, so it never reaches any other method. Answering those two with a constant and a PrintWriter over the captured stream covers the same paths and cannot block. The tests now exercise both calls from both halves of the window rather than one call each. Co-authored-by: Sylwester Lachiewicz <slachiewicz@apache.org>
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
2 participants
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.
Fixes #12761. A
mvnstart can hang before printing anything, with no Java-level deadlock reported.MessageUtils.systemInstallpublishes theFastTerminalimmediately and builds the real terminal onfast-terminal-thread. EveryFastTerminalmethod delegates throughgetTerminal(), which waits on the future that thread completes, so a single log statement from that thread parks it on its own result.mainthen parks behind it inactivateLogging.The trigger arrived with the rc-6 JLine 3.30.6 → 4.3.1 bump: JLine 4.x initialises its native loader during provider probing and logs a JUL warning when a library candidate fails
System.load, by which timeactivateLogginghas bridged JUL to SLF4J. The load failure itself is harmless — JLine recovers by extracting the bundled library.Two details shaped the fix. The hazard window covers the consumer as well, since
consumer.accept(term)runs beforeterminal.complete(term). And one log statement reaches the terminal twice, throughMavenSimpleLogger.renderLevel→toAnsi→getType()and again throughwrite→ the log sink installed increateTerminal→terminal.writer(), so guarding the message builder alone moves the hang rather than removing it.Those two calls are the whole surface.
AttributedCharSequence.toAnsiasks for the type first and renders plain for a dumb terminal, so it never reaches another method.getType()andwriter()therefore answer the building thread directly, withTYPE_DUMBand a writer over the stream captured before the consumer swaps the system streams. Rendering degrades to unstyled text while the build is in flight, which is what a dumb terminal would produce anyway.Two alternatives were tried and rejected. Handing back a stand-in
DumbTerminalcovers every delegating method at once, but it constructs a terminal inside terminal construction on that same thread, and wedged on ubuntu/jdk-17 (see the second commit). Throwing on re-entry escapes through the JUL handler and aborts JLine's fallback chain, turning a recoverable warning into a failed startup.Verified: two tests in
maven-jline, each exercising both calls from one half of the window. With the guards removed both fail at 30 s; with them they pass in 0.4 s.maven-4.0.xcarries the same code and the same JLine version. The CI symptom is apache/maven-executor#38.This change was created with AI assistance.