Eclipse warns about a potential resource leak although I have a finally block which closes the outermost stream, what am I missing?

Question

is there a reason Eclipse gives me the following resource leak warning: Resource leak: 'br' is never closed" ? The code I am talking about is at the bottom of this post.

I thought my finally block had it all covered, my reasoning:

• res will only be null if the FileInputStream constructor threw and therefore nothing has to be closed
• res will be the inputstream if the InputStreamReader constructor throws (malformed encoding string for example) and then only the InputStream must be closed so ok
• etc...

So what am I missing? Or could this be an eclipse bug?

Kind regards!

S.

public static String fileToString(String fileName, String encoding) throws IOException {
    InputStream is;
    InputStreamReader isr;
    BufferedReader br;
    Closeable res = null;
    try {
        is = new FileInputStream(fileName);
        res = is;
        isr = new InputStreamReader(is, encoding);
        res = isr;
        br = new BufferedReader(isr);
        res = br;
        StringBuilder builder = new StringBuilder();
        String line = null;
        while ((line = br.readLine()) != null) {
            builder.append(line);
            builder.append(LS);
        }
        return builder.toString();
    } finally {
        if (res != null) {
            res.close();
        }
    }
}

Answer 1

Eclipse probably just isn't understanding the shuffling you're doing with the res variable.

I recommend using the try-with-resources statement (available in Java 7 and up, so three and a half years now), it dramatically simplifies these sorts of chains:

public static String fileToString(String fileName, String encoding) throws IOException {

    try (
        InputStream is = new FileInputStream(fileName);
        InputStreamReader isr = new InputStreamReader(is, encoding);
        BufferedReader br = new BufferedReader(isr)
    ) {
        StringBuilder builder = new StringBuilder();
        String line = null;
        while ((line = br.readLine()) != null) {
            builder.append(line);
            builder.append(LS);
        }
        return builder.toString();
    }
}

If you can't use try-with-resources, you probably want something like the Apache Commons IOUtils class's closeQuietly methods (either literally that one, or your own) rather than shuffling res around, which is awkward to read and I daresay prone to maintenance issues.

Using IOUtils might look like this:

public static String fileToString(String fileName, String encoding) throws IOException {
    InputStream is = null;
    InputStreamReader isr = null;
    BufferedReader br = null;

    try {
        is = new FileInputStream(fileName);
        isr = new InputStreamReader(is, encoding);
        br = new BufferedReader(isr)
        StringBuilder builder = new StringBuilder();
        String line = null;
        while ((line = br.readLine()) != null) {
            builder.append(line);
            builder.append(LS);
        }
        br.close();
        return builder.toString();
    }
    finally {
        IOUtils.closeQuietly(br, isr, is);
    }
}

Note how I use a normal close in the try, but then ensure cleanup in the finally.

But try-with-resources is the better answer, as it's more concise and hooks into the new(ish) "suppressed exceptions" stuff.

---

Side note: There's no reason for the = null initialization of line, you assign it on the next line.

Side note 2: If the file is likely to be of any size, consider finding out how big it is in advance and setting the capacity of the StringBuilder in the constructor. StringBuilder's default capacity is 16, so a file of even a few hundred bytes involves several reallocations of StringBuilder's internal buffer.