I have a Java program which launches a separate subprocess represented by the Process class, and then attaches listeners which look at the stdout/stderr of the Process. In some cases, the Process will hang and stop being able to make progress, at which time the TimeLimiter will throw a TimeoutException, attempt to interrupt the underlying thread which is actually doing the readLine() call, and then kill the Process using kill -9 and close the stdout and stderr streams from the Process object. The last thing it tries to do is close the BufferedReader, but this call hangs forever. Sample code below:
private static final TimeLimiter timeLimiter = new SimpleTimeLimiter(); // has its own thread pool
public void readStdout(Process process) {
BufferedReader reader = new BufferedReader(new InputStreamReader(process.getInputStream()));
try {
String line = null;
while ((line = timeLimiter.callWithTimeout(reader::readLine, 5, TimeUnit.SECONDS, true)) != null) { // this will throw a TimeoutException when the process hangs
System.out.println(line);
}
} finally {
killProcess(process); // this does a "kill -9" on the process
process.getInputStream().close(); // this works fine
process.getErrorStream().close(); // this works fine
reader.close(); // THIS HANGS FOREVER
}
}
Why does the close() call hang forever, and what can I do about it?
Related question: Program freezes on bufferedreader close
UPDATE:
In case it wasn't clear, TimeLimiter is from the Guava libraries: https://github.com/google/guava/blob/master/guava/src/com/google/common/util/concurrent/SimpleTimeLimiter.java
Also, I've been asked for the code from the killProcess() method, so here it is (note this only works on Linux/Unix machines):
public void killProcess(Process process) {
// get the process ID (pid)
Field field = process.getClass().getDeclaredField("pid"); // assumes this is a java.lang.UNIXProcess
field.setAccessible(true);
int pid = (Integer)field.get(process);
// populate the list of child processes
List<Integer> processes = new ArrayList<>(Arrays.asList(pid));
for (int i = 0; i < processes.size(); ++i) {
Process findChildren = Runtime.getRuntime().exec(new String[] { "ps", "-o", "pid", "--no-headers", "--ppid", Integer.toString(processes.get(i)) });
findChildren.waitFor(); // this will return a non-zero exit code when no child processes are found
Scanner in = new Scanner(findChildren.getInputStream());
while (in.hasNext()) {
processes.add(in.nextInt());
}
in.close();
}
// kill all the processes, starting with the children, up to the main process
for (int i = processes.size() - 1; i >= 0; --i) {
Process killProcess = Runtime.getRuntime().exec(new String[] { "kill", "-9", Integer.toString(processes.get(i)) });
killProcess.waitFor();
}
}
The underlying issue here is with multi-threading and synchronization locks. When you call timeLimiter.callWithTimeout it's creating another thread in a thread pool to actually do the readLine(). When the call times out, the main thread tries to call close(), but unfortunately the readLine() and close() methods in the BufferedReader use the same synchronization lock object, so since another thread already has the lock, this call will block until the other thread relinquishes it. But if the readLine() call never returns, then the close() call will hang forever. Here's a snip-it of the BufferedReader source code:
String readLine(boolean ignoreLF) throws IOException {
StringBuffer s = null;
int startChar;
synchronized (lock) {
ensureOpen();
boolean omitLF = ignoreLF || skipLF;
...
...
...
}
}
public void close() throws IOException {
synchronized (lock) {
if (in == null)
return;
try {
in.close();
} finally {
in = null;
cb = null;
}
}
}
Of course, the TimeLimiter will try to interrupt the thread which is doing the readLine(), so that thread should really exit and let the thread trying to call close() go through. The real bug here is that the thread interruption isn't being respected by BufferedReader. In fact, there is already a bug reported in the JDK tracker about this very thing, but it's marked as "won't fix" for some reason: https://bugs.openjdk.java.net/browse/JDK-4859836
Although, to be fair, it's not really BufferedReader's responsibility to honor the thread interrupt. BufferedReader is just a buffer class, all calls to its read() or readLine() methods just read data from the underlying input stream. In this case, the underlying class is an InputStreamReader, which if you look at its source code, creates a StreamDecoder under the hood to do all of its read operations. Really, the bug is with StreamDecoder -- it should honor thread interrupts, but it doesn't.
What to do about it? Well for better or for worse, there's no way to force an object to give up its thread lock. Since StreamDecoder is obviously not code that we own or can edit, there's not much we can do. My current solution was just to remove the part where we call close() on the BufferedReader, so now at least the program doesn't hang forever. But it's still a memory leak... that thread which is running readLine() in the TimeLimiter's thread pool will essentially run forever. Since this is all part of a long-running program which processes a lot of data over time, eventually that thread pool will fill up with junk threads and the JVM will crash...
If anyone has any other advice for how to work around this, please let me know.