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?

Solution 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).

Solution 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.

Solution 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.

Solution 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

Solution 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.

View more solutions

Comments

[matt b]

not flagging the warning sounds more like an Eclipse bug than a conscious decision to say "it's ok to do it in this case"

[Brian]

See this question. If a resource like an InputStream is being passed into a method, that means that someone else opened it, so someone else should close it as well. If they pass a stream to your method and then try to use it somewhere else afterwards, it would break because it would be closed (closing a BufferReader closes its stream), even though your method might have only needed to do one thing with the stream.

[Michael Krussel]

stdOut can never be equal to null, so there is no point in the if block before closing. If the construction failed, it will never reach the finally block.

[Jiri Kremser]

Oh, you are right. I've edited it slightly. Now, it is correct.

[GLaDOS]

Right you are... I went to look at the place where the stream was actually created ... much further upstream in the code; and it is being closed there. Thanks!

[GLaDOS]

Pretty sure we won't be going to 7 any time soon :(

[GLaDOS]

Also right about logging output from another process. That process was doing some work on the file being streamed by the stream in question, and it was important to track any potential errors or problems in the logs.

[GLaDOS]

Also awesome and thanks for the more detailed answer. You were more help than my dev manager who was overseeing the code in question at time of writing :)