Skip to content

Commit 833a64b

Browse files
gnodetclaude
andauthored
Fix #12912: assign FastTerminal before starting the build thread (#12962)
The FastTerminal constructor started the build thread before returning, so MessageUtils.terminal was still null when the build thread ran the builder callback — a race between the constructor returning and the thread scheduling. Split construction from start: MessageUtils now assigns the field before calling FastTerminal.start(), and Thread.start() provides the happens-before edge that makes the assignment visible to the build thread. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 07a9913 commit 833a64b

3 files changed

Lines changed: 41 additions & 1 deletion

File tree

impl/maven-jline/src/main/java/org/apache/maven/jline/FastTerminal.java

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,20 @@ public FastTerminal(Callable<Terminal> builder, Consumer<Terminal> consumer) {
104104
"fast-terminal-thread");
105105
// a wedged builder must not keep the JVM alive; everything waits on the future, not the thread
106106
this.buildThread.setDaemon(true);
107+
}
108+
109+
/**
110+
* Starts the build thread. Must be called <em>after</em> the caller has published this
111+
* {@code FastTerminal} (e.g. assigned it to {@link MessageUtils#terminal}) so that code running
112+
* on the build thread can obtain a non-null reference through {@link MessageUtils#getTerminal()}.
113+
* <p>
114+
* {@link Thread#start()} establishes a <em>happens-before</em> edge, so the assignment made by
115+
* the caller before this method is visible to the build thread without additional
116+
* synchronization.
117+
*
118+
* @see <a href="https://github.com/apache/maven/issues/12912">#12912</a>
119+
*/
120+
public void start() {
107121
this.buildThread.start();
108122
}
109123

impl/maven-jline/src/main/java/org/apache/maven/jline/MessageUtils.java

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,11 @@ public static void systemInstall() {
4747
}
4848

4949
public static void systemInstall(Consumer<TerminalBuilder> builderConsumer, Consumer<Terminal> terminalConsumer) {
50-
MessageUtils.terminal = new FastTerminal(
50+
// Assign the FastTerminal to the field BEFORE starting the build thread so that code
51+
// running on that thread (e.g. JLine's FFM provider init, logger calls) sees a non-null
52+
// reference when it calls MessageUtils.getTerminal(). Thread.start() provides the
53+
// happens-before edge that makes the assignment visible to the new thread.
54+
FastTerminal ft = new FastTerminal(
5155
() -> {
5256
TerminalBuilder builder =
5357
TerminalBuilder.builder().name("Maven").dumb(true);
@@ -64,6 +68,8 @@ public static void systemInstall(Consumer<TerminalBuilder> builderConsumer, Cons
6468
terminalConsumer.accept(terminal);
6569
}
6670
});
71+
MessageUtils.terminal = ft;
72+
ft.start();
6773
}
6874

6975
private static LineReader createReader(Terminal terminal) {

impl/maven-jline/src/test/java/org/apache/maven/jline/FastTerminalReentrancyTest.java

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,26 @@ void arbitraryTerminalMethodsFromTheConsumerDoNotDeadlock() {
122122
});
123123
}
124124

125+
/**
126+
* Verifies that {@link MessageUtils#getTerminal()} is non-null when called from the builder
127+
* callback. Before the fix, the {@code FastTerminal} constructor started its build thread
128+
* before returning, so {@code MessageUtils.terminal} was still {@code null} when the build
129+
* thread ran the builder callback &mdash; a race between the constructor returning and the
130+
* thread scheduling. After the fix, {@code MessageUtils} assigns the field before calling
131+
* {@link FastTerminal#start()}, and {@link Thread#start()} provides the happens-before edge.
132+
*
133+
* @see <a href="https://github.com/apache/maven/issues/12912">#12912</a>
134+
*/
135+
@Test
136+
void terminalAssignmentIsVisibleFromBuilderCallback() {
137+
assertTimeoutPreemptively(Duration.ofSeconds(30), () -> {
138+
CompletableFuture<Terminal> observed = new CompletableFuture<>();
139+
installAndAwait(builder -> observed.complete(MessageUtils.getTerminal()), terminal -> {});
140+
assertNotNull(observed.get(), "MessageUtils.getTerminal() must not return null from the builder callback");
141+
assertTrue(observed.get() instanceof FastTerminal, "terminal should be the FastTerminal wrapper");
142+
});
143+
}
144+
125145
/**
126146
* Both terminal calls a single log statement makes, run on the terminal building thread.
127147
*/

0 commit comments

Comments
 (0)