Skip to content

Commit 18ceb3e

Browse files
authored
Fix multi-repo RPC deadlock via stderr redirect and dedicated GetObject pool (#7036)
Two independent deadlocks prevented `mod run` from completing across multiple C# repositories: 1. **Stderr pipe buffer deadlock**: On macOS the kernel pipe buffer for stderr is 64KB. When the dotnet subprocess writes enough to stderr to fill this buffer, it blocks on the next write(), which deadlocks any in-progress RPC response on stdout. Fix: redirect stderr at the OS level via ProcessBuilder.redirectError() — to the log file when configured, or to /dev/null otherwise. This eliminates the pipe entirely so the subprocess never blocks on stderr. 2. **GetObject thread pool starvation**: GetObject.Handler submitted background tree traversal tasks to ForkJoinPool.commonPool(), which is the same pool used by the CLI for repo-level fork-join work. When repo tasks saturated the pool (blocked on semaphores or waiting for RPC responses), GetObject producers couldn't start, deadlocking the batch protocol. Fix: use a dedicated cached thread pool for tree traversal so GetObject producers are never starved by unrelated work.
1 parent e5d7c93 commit 18ceb3e

5 files changed

Lines changed: 24 additions & 29 deletions

File tree

rewrite-core/src/main/java/org/openrewrite/rpc/RewriteRpcProcess.java

Lines changed: 13 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
import lombok.Setter;
3232
import org.jspecify.annotations.Nullable;
3333

34+
import java.io.File;
3435
import java.io.IOException;
3536
import java.io.InputStream;
3637
import java.io.UncheckedIOException;
@@ -46,6 +47,9 @@
4647
* A client for spawning and communicating with a subprocess that implements Rewrite RPC.
4748
*/
4849
public class RewriteRpcProcess extends Thread {
50+
private static final File DEV_NULL = new File(
51+
System.getProperty("os.name").startsWith("Windows") ? "NUL" : "/dev/null");
52+
4953
private final String[] command;
5054

5155
@Setter
@@ -63,7 +67,8 @@ public class RewriteRpcProcess extends Thread {
6367

6468
private final Map<String, String> environment = new LinkedHashMap<>();
6569

66-
private final StringBuffer accumulatedStderr = new StringBuffer();
70+
@Setter
71+
private @Nullable Path stderrRedirect;
6772

6873
public RewriteRpcProcess(String... command) {
6974
this.command = command;
@@ -88,6 +93,11 @@ public void run() {
8893
if (workingDirectory != null) {
8994
pb.directory(workingDirectory.toFile());
9095
}
96+
if (stderrRedirect != null) {
97+
pb.redirectError(ProcessBuilder.Redirect.appendTo(stderrRedirect.toFile()));
98+
} else {
99+
pb.redirectError(ProcessBuilder.Redirect.to(DEV_NULL));
100+
}
91101
process = pb.start();
92102
} catch (IOException e) {
93103
throw new UncheckedIOException(e);
@@ -99,32 +109,9 @@ public void run() {
99109
return null;
100110
}
101111

102-
// Accumulate any available stderr
103-
try {
104-
InputStream errorStream = process.getErrorStream();
105-
int available = errorStream.available();
106-
if (available > 0) {
107-
byte[] buffer = new byte[available];
108-
int read = errorStream.read(buffer);
109-
if (read > 0) {
110-
accumulatedStderr.append(new String(buffer, 0, read));
111-
}
112-
}
113-
} catch (IOException | UnsupportedOperationException e) {
114-
// Ignore errors reading stderr
115-
}
116-
117112
if (!process.isAlive()) {
118113
int exitCode = process.exitValue();
119114

120-
// Read any remaining stderr
121-
try {
122-
InputStream errorStream = process.getErrorStream();
123-
accumulatedStderr.append(readFully(errorStream));
124-
} catch (UnsupportedOperationException e) {
125-
// Ignore errors reading final stderr
126-
}
127-
128115
// Read any remaining stdout
129116
String stdOutput = "";
130117
try {
@@ -135,12 +122,11 @@ public void run() {
135122
}
136123

137124
String message = "RPC process shut down early with exit code " + exitCode;
138-
String errorOutput = accumulatedStderr.toString();
139125
if (!stdOutput.isEmpty()) {
140126
message += "\nStandard output:\n " + stdOutput.replace("\n", "\n ");
141127
}
142-
if (!errorOutput.isEmpty()) {
143-
message += "\nError output:\n " + errorOutput.replace("\n", "\n ");
128+
if (stderrRedirect != null) {
129+
message += "\nSee stderr log: " + stderrRedirect;
144130
}
145131
return new IllegalStateException(message.trim());
146132
}

rewrite-core/src/main/java/org/openrewrite/rpc/request/GetObject.java

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,13 @@ public class GetObject implements RpcRequest {
4444

4545
@RequiredArgsConstructor
4646
public static class Handler extends JsonRpcMethod<GetObject> {
47-
private static final ExecutorService forkJoin = ForkJoinPool.commonPool();
47+
// Dedicated pool for tree traversal so GetObject producers can't be starved
48+
// by unrelated tasks (e.g. repo-level fork-join work) occupying the commonPool.
49+
private static final ExecutorService TREE_TRAVERSAL_POOL = Executors.newCachedThreadPool(r -> {
50+
Thread t = new Thread(r, "rpc-get-object-traversal");
51+
t.setDaemon(true);
52+
return t;
53+
});
4854

4955
private final AtomicInteger batchSize;
5056
private final Map<String, Object> remoteObjects;
@@ -77,7 +83,7 @@ protected List<RpcObjectData> handle(GetObject request) throws Exception {
7783
Object before = remoteObjects.get(id);
7884

7985
RpcSendQueue sendQueue = new RpcSendQueue(batchSize.get(), batch::put, localRefs, request.getSourceFileType(), traceGetObject.get());
80-
forkJoin.submit(() -> {
86+
TREE_TRAVERSAL_POOL.submit(() -> {
8187
try {
8288
sendQueue.send(after, before, null);
8389

rewrite-csharp/src/main/java/org/openrewrite/csharp/rpc/CSharpRewriteRpc.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -323,6 +323,7 @@ private CSharpRewriteRpc startProcess(Stream<@Nullable String> cmd) {
323323
if (workingDirectory != null) {
324324
process.setWorkingDirectory(workingDirectory);
325325
}
326+
process.setStderrRedirect(log);
326327

327328
process.environment().putAll(environment);
328329

rewrite-javascript/src/main/java/org/openrewrite/javascript/rpc/JavaScriptRewriteRpc.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -382,6 +382,7 @@ public JavaScriptRewriteRpc get() {
382382
if (workingDirectory != null) {
383383
process.setWorkingDirectory(workingDirectory);
384384
}
385+
process.setStderrRedirect(log);
385386

386387
process.environment().putAll(environment);
387388
// caller-provided options, if any, are taking precedence over the options baked above

rewrite-python/src/main/java/org/openrewrite/python/rpc/PythonRewriteRpc.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -581,6 +581,7 @@ public PythonRewriteRpc get() {
581581
if (workingDirectory != null) {
582582
process.setWorkingDirectory(workingDirectory);
583583
}
584+
process.setStderrRedirect(log);
584585

585586
process.environment().putAll(environment);
586587

0 commit comments

Comments
 (0)