Friday, July 17, 2015

The Achilles Heel of Try-With-Resources

Here's some code that you might see since Java 7 appeared. Can you spot the bug?

public List<String> extractData(File file, String encoding)
throws IOException
{
    List<String> results = new ArrayList<String>();
    try (BufferedReader rdr = new BufferedReader(new InputStreamReader(new FileInputStream(file), encoding)))
    {
        // read each line and extract data
    }
    return results;
}

OK, here's a hint: what if encoding is invalid?

The answer is that the InputStreamReader constructor will throw UnsupportedEncodingException (which is a subclass of IOException).

But this exception happens after the FileInputStream has been created, but before the body of the try-with-resources statement. Which means that the implicit finally in that statement won't get executed, and the file will remain open until the stream's finalizer executes.

To solve this problem, you need to break apart the constructor chain, to ensure that the actual resource is assigned to its own variable in the resource clause, and will therefore be closed. There are two ways to approach this: either use multiple resource clauses, or push non-resource constructors into the body of the statement.

In this case, the InputStreamReader and BufferedReader are simple decorator objects: if you neglect to close them, it doesn't change the behavior of the program. So we can construct them inside the body of the statement:

try (InputStream in = new FileInputStream(file))
{
    BufferedReader rdr = new BufferedReader(new InputStreamReader(in, encoding));
    // ...

Bug solved: regardless of what happens when constructing rdr, the underlying stream will be closed; we don't have to worry about a “too many open files” exception.

However, if we try to do something similar with output streams, we introduce a new bug:

try (OutputStream out = new FileOutputStream(file))
{
    // DON'T DO THIS!
    BufferedWriter wtr = new BufferedWriter(new OutputStreamWriter(out, encoding));
    // ...

The problem with this code is that decorators are important when writing. Unless you close the BufferedOutputStream, the data in its buffer won't be written to the file. On the positive side, you'll see this bug the first time you try to write a file. To solve it and still use the try-with-resources construct, you need to use multiple resource clauses:

try (OutputStream out = new FileOutputStream(file) ;
        OutputStreamWriter osw = new OutputStreamWriter(out, encoding) ;
        BufferedWriter wtr = new BufferedWriter(osw))
{
    // ...

There you have it, the only safe way to use try-with-resources. If you currently have code with nested constructors, I strongly recommend that you change it. And please change any public documentation that has nested constructors; explaining the bug to someone that is “just following the docs” wastes time that could be used for better things.

No comments: