Skip to content

Commit 9d2df7f

Browse files
committed
Shut down the command line's thread pool when formatting is done
Main.formatFiles created a fixed thread pool on every call and never shut it down. The command line does not notice, because the process exits, but a tool that runs Main in-process kept up to MAX_THREADS idle threads per call (#40, from google/google-java-format#384). The pool is now closed when formatFiles returns. On Java 21 ExecutorService is AutoCloseable, and close() waits for the submitted tasks, which formatFiles has already waited for by then. The new MainTest runs format from a thread of its own thread group, which the pool's threads join, and fails without the change because a pool thread is still running.
1 parent f5e6b12 commit 9d2df7f

2 files changed

Lines changed: 34 additions & 2 deletions

File tree

  • open-java-format/src

‎open-java-format/src/main/java/com/palantir/javaformat/java/Main.java‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -120,11 +120,18 @@ public int format(String... args) throws UsageException {
120120
}
121121
}
122122

123-
@SuppressWarnings("for-rollout:RedundantControlFlow")
124123
private int formatFiles(CommandLineOptions parameters, JavaFormatterOptions options) {
125124
int numThreads = Math.min(MAX_THREADS, parameters.files().size());
126-
ExecutorService executorService = Executors.newFixedThreadPool(numThreads);
125+
// Closing the pool ends its threads, so that a tool that runs Main in-process does not keep them. The close
126+
// waits for the submitted tasks, which formatFiles has already waited for.
127+
try (ExecutorService executorService = Executors.newFixedThreadPool(numThreads)) {
128+
return formatFiles(parameters, options, executorService);
129+
}
130+
}
127131

132+
@SuppressWarnings("for-rollout:RedundantControlFlow")
133+
private int formatFiles(
134+
CommandLineOptions parameters, JavaFormatterOptions options, ExecutorService executorService) {
128135
Map<Path, String> inputs = new LinkedHashMap<>();
129136
Map<Path, Future<String>> results = new LinkedHashMap<>();
130137
boolean allOk = true;

‎open-java-format/src/test/java/com/palantir/javaformat/java/MainTest.java‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,8 @@
3535
import java.nio.file.attribute.PosixFilePermission;
3636
import java.util.EnumSet;
3737
import java.util.Locale;
38+
import java.util.concurrent.FutureTask;
39+
import java.util.concurrent.TimeUnit;
3840
import org.junit.jupiter.api.Test;
3941
import org.junit.jupiter.api.io.TempDir;
4042
import org.junit.jupiter.api.parallel.Execution;
@@ -91,6 +93,29 @@ public void version() throws UsageException {
9193
assertThat(err.toString()).contains("open-java-format: Version ");
9294
}
9395

96+
// Main used to leave its thread pool running after format returned. The command line does not notice, because it
97+
// exits, but anything that runs Main in-process kept the idle threads (#40, from google/google-java-format#384).
98+
@Test
99+
public void formatLeavesNoPoolThreadRunning() throws Exception {
100+
Path path = Files.writeString(testFolder.resolve("A.java"), "class A {}\n");
101+
Main main = new Main(
102+
new PrintWriter(new StringWriter(), true), new PrintWriter(new StringWriter(), true), System.in);
103+
// The pool's threads join the thread group of the thread that creates the pool.
104+
ThreadGroup group = new ThreadGroup("formatLeavesNoPoolThreadRunning");
105+
FutureTask<Integer> format = new FutureTask<>(() -> main.format(path.toString()));
106+
new Thread(group, format).start();
107+
assertThat(format.get()).isEqualTo(0);
108+
109+
Thread[] threads = new Thread[group.activeCount() + 16];
110+
int count = group.enumerate(threads);
111+
for (int i = 0; i < count; i++) {
112+
threads[i].join(TimeUnit.SECONDS.toMillis(10));
113+
assertWithMessage(threads[i].getName() + " is still running")
114+
.that(threads[i].isAlive())
115+
.isFalse();
116+
}
117+
}
118+
94119
@Test
95120
public void preserveOriginalFile() throws Exception {
96121
Path path = Files.createFile(testFolder.resolve("Test.java"));

0 commit comments

Comments
 (0)