Java: BufferedReader hangs forever on close() and StreamDecoder doesn't respect thread interrupt

Question

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();
    }
}

Answer 1

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.

Answer 2

If you kill the process, the stdout/stderr should dry out not long after (EOF and null from readLine eventually when OS pipes dry out).

At least, closing the streams should cause the concurrent readers/writers to fail. That works on sockets, so I expect it to work on files and processes...

So I see no point in calling the bufferedreader.close(), you have nothing to loose. BufferedReader is just memory that will be freed on GC. The underlying streams were already closed, and even then, the process is killed. One of those kill or close must eject the slave thread's readLine with null or some exception.

UPDATE:

The code below shows that killing the process will end the streams as expected:

package tests;

import java.io.BufferedReader;
import java.io.InputStream;
import java.io.InputStreamReader;
import java.io.PrintWriter;
import java.io.StringWriter;
import java.util.concurrent.TimeUnit;
import java.util.stream.Stream;

public class TestProcessStreamsCloseEOF {

    static void p(Object msg, Throwable... ta) {
        StringWriter sw = new StringWriter();
        PrintWriter pw = new PrintWriter(sw);
        pw.println(Thread.currentThread().getName()+"] "+msg);

        if(ta!=null && ta.length>0)
            Stream.of(ta).forEach(t -> t.printStackTrace(pw));
        pw.flush();
        System.out.print(sw.toString());
    }

    public static void main(String[] args) throws Exception {
        /*
        slowecho.bat:
        -----------
        @echo off
        echo line 1
        pause
        echo line 2
        */
        Process p = new ProcessBuilder("slowecho.bat").start();
        new Thread(() -> {dump(p.getInputStream());}, "dumpstdout").start();
        new Thread(() -> {dump(p.getErrorStream());}, "dumpstderr").start();

        p("sleep 5s");
        Thread.sleep(5000);

        p("destroy...");
        //p.destroy();
        p.destroyForcibly();

        p("waitfor 5s");
        p.waitFor(5, TimeUnit.SECONDS);

        p("sleep 5s");
        Thread.sleep(5000);

        p("end.");
    }

    static void dump(InputStream is) {
        try {
            BufferedReader br = new BufferedReader(new InputStreamReader(is, "ISO-8859-1"));
            String line;
            while((line=br.readLine()) !=null) {
                p(line);
            }
        } catch(Throwable t) {
            p(""+t, t);
        }
        p("end");
    }

}

Answer 3

I would rather prefer to handle resources gracefully like below

import java.io.BufferedReader;
import java.io.InputStream;
import java.io.InputStreamReader;
import java.io.PrintWriter;
import java.io.StringWriter;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.stream.Stream;

public class Test {

    static AtomicBoolean interruptLoggingThread = new AtomicBoolean(false);

    static void print(Object msg, Throwable... expArr) {
        StringWriter sw = new StringWriter();
        PrintWriter pw = new PrintWriter(sw);
        pw.println(Thread.currentThread().getName() + "] " + msg);

        if (expArr != null && expArr.length > 0) {
            Stream.of(expArr).forEach(exp -> exp.printStackTrace(pw));
        }
        pw.flush();
        System.out.print(sw.toString());
    }

    public static void main(String[] args) throws Exception {
        Process p = new ProcessBuilder("slowecho.bat").start();
        new Thread(() -> {
            dump(p.getInputStream());
        }, "dumpstdout").start();
        new Thread(() -> {
            dump(p.getErrorStream());
        }, "dumpstderr").start();

        try {
            p.waitFor(10000, TimeUnit.SECONDS); // Wait process to terminate within 10 Seconds
            interruptLoggingThread.set(true);
            p.destroyForcibly(); // To ensure process got terminated forcibly (if not gracefully terminated within 10 seconds)
        } catch (Exception e) {
        }
        try {
            p.getInputStream().close(); // Ensure to close standard output stream
        } catch (Exception e) {
        }
        try {
            p.getErrorStream().close(); // Ensure to close standard error stream
        } catch (Exception e) {
        }
    }

    static void dump(InputStream is) {
        try {
            BufferedReader br = new BufferedReader(new InputStreamReader(is, "ISO-8859-1"));
            String line;
            while (!interruptLoggingThread.get()) {
                if (br.ready()) { // ready() is non-blocking
                    line = br.readLine();
                    // ready() check ensures, readLine() will not be blocked for input
                    if (line != null) {
                        print(line);
                    }
                } else {
                    Thread.sleep(1000l);
                }
            }
        } catch (Throwable t) {
            print("" + t, t);
        }
        print("end");
    }
}