Thursday, May 10, 2012

Common try-with-resources idiom won't clean up properly

So I was re-reading some blogs on the JDK 7 language changes and I noticed the following idiom seems popular:

//
try (BufferedReader br = new BufferedReader(
        new FileReader("/tmp/click.xml"))) {
   System.out.println(br.readLine());
}

That seems all well and good; but consider what happens should something goes wrong constructing the BufferedReader. The contract for try-with-resources will only try to close and tidy up the declared field, if something goes wrong then FileReader is never closed.

So consider this example where the constructor of BufferedReader throws an Error, it could well be an OutOfMemoryError or any number of other failure modes.

//
package client;

import java.io.*;

public class BufferedFileExample {

    public static void main(String[] args) throws FileNotFoundException, 
                                                  IOException {

        try (BufferedReader br = new MyBufferedReader(
            new MyFileReader("/tmp/click.xml"))) {
            System.out.println(br.readLine());
        }
    }
    
    public static class MyFileReader extends FileReader 
    {

        public MyFileReader(String in) throws FileNotFoundException {
            super(in);
        }
        
        public void close() throws IOException {
            super.close();
            System.out.println("Close called");
        }
    }
    
    
    
    public static class MyBufferedReader extends BufferedReader 
    {

        public MyBufferedReader(Reader in) {
            super(in);
            throw new Error();
        }
    }
}

This gets us the following output and as such the close is not performed on the FileReader which until the finaliser is called. You are stuck until the gc wakes up in the future to tidy it up.

Exception in thread "main" java.lang.Error
 at client.BufferedFileExample$MyBufferedReader.(BufferedFileExample.java:38)
 at client.BufferedFileExample.main(BufferedFileExample.java:12)
Process exited with exit code 1.

Luckily there is an easy fix for this, the language change supports multiple entries in try statement, so you can modify this code to be:

//
try (FileReader fr = new MyFileReader("/tmp/click.xml");
     BufferedReader br = new MyBufferedReader(
        fr)) {
   System.out.println(br.readLine());
}

So if you run the code with the modification above then you find that close is now called:

Close called
Exception in thread "main" java.lang.Error
 at client.BufferedFileExample$MyBufferedReader.(BufferedFileExample.java:38)
 at client.BufferedFileExample.main(BufferedFileExample.java:12)
Process exited with exit code 1.

So problem solved, well unfortunately there is another wrinkle here, if we remove the code that throws the Error then we will see the following output:

Close called
Close called
Process exited with exit code 0.

So this is fine for classes that implement the existing java.io.Closeble interface because calling close for a second time is fine; but the more generic interface that is used for other closeable resources unfortunately doesn't have this restriction, so quote the spec

Note that unlike the close method of Closeable, this close method is not required to be idempotent. In other words, calling this close method more than once may have some visible side effect, unlike Closeable.close which is required to have no effect if called more than once. However, implementers of this interface are strongly encouraged to make their close methods idempotent.

So in order to using a split-try-with-resources idiom you first need to make sure that the resources have a idempotent close method. I suspect that most API will do this; but you need to check first and perhaps need to further wrap your objects to make this the case.

One final point with try-with-resource is the handling of exceptions when the close and construction fails. JDK 7 introduced the concept of suppressed exceptions, so you it make the split version of the code throw an exception both in the constructor or the MyBufferedReader and in the close of MyFileReader you get to see the following novel stack trace.

Close called
Exception in thread "main" java.lang.Error
 at client.BufferedFileExample$MyBufferedReader.(BufferedFileExample.java:38)
 at client.BufferedFileExample.main(BufferedFileExample.java:12)
 Suppressed: java.lang.Error: close
  at client.BufferedFileExample$MyFileReader.close(BufferedFileExample.java:27)
  at client.BufferedFileExample.main(BufferedFileExample.java:14)
Process exited with exit code 1.

This might not be what you calling code is expecting - certainly it might differ slightly from what the try/finally code you are replacing would produce, worth know thing that exceptions are handled slightly differently. For example if there is just an Error thrown during the close method would would see this odd self-referential stack trace.

Close called
Close called
Exception in thread "main" java.lang.Error: close
 at client.BufferedFileExample$MyFileReader.close(BufferedFileExample.java:28)
 at java.io.BufferedReader.close(BufferedReader.java:517)
 at client.BufferedFileExample.main(BufferedFileExample.java:15)
 Suppressed: java.lang.Error: close
  at client.BufferedFileExample$MyFileReader.close(BufferedFileExample.java:28)
  ... 1 more
Process exited with exit code 1.

So the lesson here is that the impact of some of the new JDK 7 constructs are going to a little bit subtler than we might expect. Many thanks to my valued colleague Mark Warner for talking this one through with me.

4 comments:

danielm said...

While I agree that there is a potential resource leak, I don't think the try-with-resources idiom is to blame. Rather, it's the "new BufferedReader(new FileReader())" idiom, which is something that has been around since the beginning if time. If you rewrite the original code snippet using the old try/finally style, you'll find the exact same problem.

The reason no one bothers about this too much is that it's extremely unlikely that the BufferedReader constructor will ever throw an exception. If it does, it will probably be something fatal like OutOfMemoryError, so your process is toast anyway.

Gerard Davison said...

I agree with you that the Buffered/File pattern is common across java code; but this blog doesn't just affect this simple case. I perhaps should have used another example. It is possible that a stack of filters is used for a particular resource and the contraction or any of a number of them could fail and leave a resource open.

You can make the same mistake with try/finally as you say; but the try-with-resources might give some people a false sense of security. So I though it worth pointing out.

Finally I do have to disagree with your final statement that OutOfMemoryError means your process is toast. It is possible that this condition is temporary and in that case you would want the rest of the process to recover from it. There are ways and means of structuring the code so you can recover.

Wouter Oet said...

In your first example you're wrong.

try (BufferedReader br = new BufferedReader(
new FileReader("/tmp/click.xml"))) {
System.out.println(br.readLine());
}

Invoking br.close() as the try-with-resources will do, will close the FileReader! Just check out the source code of BufferedReader.

Gerard Davison said...

Wouter,

The FileReader will only be closed if the constructor for BufferedReader complete successfully. Try running the code provided in first example to see what happens.

I am not saying that FileReader.close() is never called, only that is cannot be guaranteed to be called. This is an important distinction.

Gerard