Is there really a resource leak if I close the underlying stream?

Question

I ran a dynamic code analysis tool on one of our programs and this pattern was identified as a resource leak:

...
FileInputStream fileInputStream = new FileInputStream(file);
try {
    data = someMethod(new BufferedInputStream(fileInputStream));
    // Assume that someMethod(InputStream) internally reads the stream
    // until BufferedInputStream.read() returns -1.
    ...
}
finally {
    ...
    try {
        fileInputStream.close();
    } catch (IOException e) {
        ...
    }
}

Specifically, the analysis tool marked the new BufferedInputStream(...) call as a resource leak because it is never closed. In this pattern, however, the underlying stream fileInputStream is closed and the BufferedInputStream goes out of scope.

Note: I neglected to make it clear when I originally posted the question, but I realize this is not the "best" implementation. However, if there is not a de-facto resource leak here then it is unlikely that we will scour our legacy codebase for all instances of this pattern and close the outer streams or replace them with newer constructs such as try-with-resources--i.e., "If it ain't broke, don't fix it."

Given this situation, is this actually a resource leak?

Answer 1

There is no resource leak in this case. However, the cost of closing the BufferedInputStream instead (or as well) is minimal, so it is simplest to add a (strictly) unnecessary close to make the analysis tool happy.

Typical static analysis tools are looking for structural patterns in the code that indicate bugs. In this case, the pattern matching approach is giving a false positive for a resource leak.

Answer 2

Because Java has no deconstructors, the object going out of scope will not necessarily close the stream. Java does have finalizers, but there is no guarantee they will be called. The BufferedInputStream will not necessarily be closed. However because the underlying stream is closed I don't really think it matters.

It's better practice to ensure that the stream is closed, but it probably doesn't matter.

Answer 3

Its unlikely this is an actual resource leak, but writing code like this is definitely not a best practice.

In general, you should always call close on the outermost stream, which will ripple to the inner streams.

If you are using Java 7, you can use the new try-with-resources syntax and avoid using a finally block at all.

Related: The try-with-resources Statement