What happens to a BufferedReader that doesn't get closed within a callable.call?

Question

I have three questions.

To explain, I was reviewing someone's code, and noticed BufferedReaders sometimes aren't being closed. Usually, Eclipse gives a warning that this is a potential memory leak (and I fix it). However, within a Callable inner class, there is no warning.

class outerClass {
    ...
    public void someMethod() {
        Future<Integer> future = outputThreadPool.submit(new innerClass(this.myProcess.getInputStream(), threadName));
        ...
    }

    class innerClass implements Callable<Integer> {
        private final InputStream stream;
        private final String prepend;

        innerClass(InputStream stream, String prepend) {
            this.stream = stream;
            this.prepend = prepend;
        }

        @Override
        public Integer call() {
            BufferedReader stdOut = new BufferedReader(new InputStreamReader(stream));
            String output = null;
            try {
                while ((output = stdOut.readLine()) != null) {
                    log.info("[" + prepend + "] " + output);
                }

            } catch (IOException ignore) {
            // I have no idea why we're ignoring this... :-|        
            }
            return 0;   
        }
    }
}

The people who wrote the code are experienced Java developers, so my first thought is that it's intentional... but it could be they were in a hurry when they wrote it and just overlooked it.

My questions are:

1. Why does Eclipse not highlight this (which may be answered by the answer to the following questions)?

2. What is the worst that could happen if it's closed within the call() method? (I can't think of a good reason... and I've been searching for a while... but maybe it was intentional not to close the BufferedReader)

3. What is the worst that could happen if the BufferedReader is not closed within the inner class?

Answer 1

I would say that since they're creating a BufferedReader around a given InputStream, the code is safe not calling close(). The code that calls close() should always be the code that creates the stream and done using try/finally.

public static void read(String str) throws IOException {
    FileInputStream stream = null
    try {
        stream = new FileInputStream(str);
        readStreamToConsole(stream);
    } finally {
        if (stream != null)
            stream.close();
    }
}

private static void readStreamToConsole(InputStream stream) {
    BufferedReader stdOut = new BufferedReader(new InputStreamReader(stream));
    String output = null;
    while ((output = stdOut.readLine()) != null)
        System.out.println(output);
}

Another note: your code appears to be logging output from some other process. You probably wouldn't be able to close the stream anyway. Without testing it myself, I'm not sure what would happen if you closed a stream from another process.

Oh, and the IOException isn't likely to happen because the stream is coming from another process. That's not likely to happen unless some irrecoverable error occurs. It still wouldn't be a bad idea to log the exception somehow, though.

---

Edit to address your comment about mixed answers:

Let's use an output stream and BufferedWriter as an example this time:

private static final String NEWLINE = System.getProperty("line.separator");

public static void main(String[] args) throws IOException {
    String file = "foo/bar.txt";
    FileOutputStream stream = null;
    try {
        stream = new FileOutputStream(file);
        writeLine(stream, "Line 1");
        writeLine(stream, "Line 2");
    } finally {
        if (stream != null)
            stream.close();
    }
}

private static void writeLine(OutputStream stream, String line) throws IOException {
    BufferedWriter writer = new BufferedWriter(new InputStreamWriter(stream));
    writer.write(line + NEWLINE);
}

This works. The writeLine method is used as a delegate to creating writer and actually writing a single line to the file. Of course, this logic could be something more complex, such as turning an object into a String and writing it. This makes the main method a little easier to read too.

Now, what if instead, we closed the BufferedWriter?

private static void writeLine(OutputStream stream, String line) throws IOException {
    BufferedWriter writer = null;
    try {
        writer = new BufferedWriter(new InputStreamWriter(stream));
        writer.write(line + NEWLINE);
    } finally {
        if (writer != null)
            writer.close();
    }
}

Try running it with that, and it will fail every time on the second writeLine call. It's good practice to always close streams where they're created, instead of where they're passed. It may be fine initially, but then trying to change that code later could result in errors. If I started with only 1 writeLine call with the bad method and someone else wanted to add a second one, they'd have to refactor the code so that writeLine didn't close the stream anyway. Getting close-happy can cause headaches down the road.

Also note that technically, the BufferedWriter isn't the actual handle to your system resource, the FileOutputStream is, so you should be closing on the actual resource anyway.

So, rule of thumb: only close your streams where you create them, and always do the creation and closing in a try/finally block (or Java 7's awesome try/resource block, which does the closing for you).

Answer 2

You may not want to close the BufferedReader in this case. The InputStream which was passed to the constructor is the object that may be associated with a system resource. The BufferedReader and the InputStreamReader are just wrappers around that. Closing the BufferedReader would also close the InputStream, which may not be what the caller wanted.

Answer 3

BuffereddReader close()

Closes this stream and releases any system resources associated with it. If the stream is already closed then invoking this method has no effect.

So, if you don't close(), system resources may be still associated with the reader which may cause memory leak.

Why eclipse not highlighting: it is not compile time error if you ignore calling close(), so eclipse don't highlight.

Answer 4

No matter where you open the stream you should close it in the finally block and it is a good practice to test the stream for null as well, because if the file is not existing the stream will be null, exception will be thrown (FileNotFoundException) but the finally bock is done. I.e. the content of the call method should be:

   BufferedReader stdOut = null;
   String output = null;
        try {
            stdOut = new BufferedReader(new InputStreamReader(stream));
            while ((output = stdOut.readLine()) != null) {
                log.info("[" + prepend + "] " + output);
            }
        } catch (FileNotFoundException ex) {
            log.warn("Unable to open nonexisten file " + whichOne);  
        } catch (IOException ex) {
            log.warn("Unable to read from stream");  
        } finally {
            if (stdOut != null) {
                try {
                   stdOut.close();
                } catch (IOException e) {
                    log.warn("Unable to close the stream");
                }
            }
        }
        return 0;

Or if you are allowed to use Java 7 you can use the benefits of AutoCloseable interface and the new language structure for this purpose. see http://www.oracle.com/technetwork/articles/java/trywithresources-401775.html

Answer 5

1. Doesn't highlight and it's true, because a stream can be closed somewhere else out of the call method.

2. If it's closed within the call method that other threads may be using it at the moment.

3. With the BufferdRreader nothing but if you loose the reference to stream and cannot close it and this leads to a memory leak.