Search | Blog | Contact

Always close() in a finally block 8

Posted by Dean Wampler Thu, 31 Jul 2008 00:12:00 GMT

Here’s one for my fellow Java programmers, but it’s really generally applicable.

When you call close() on I/O streams, readers, writers, network sockets, database connections, etc., it’s easy to forgot the most appropriate idiom. I just spent a few hours fixing some examples of misuse in otherwise very good Java code.

What’s wrong the following code?

    
public void writeContentToFile(String content, String fileName) throws Exception {
    File output = new File(fileName);
    OutputStreamWriter writer = new OutputStreamWriter(new FileOutputStream(output), "UTF-8");
    writer.write(content);
    writer.close();
}

It doesn’t look all that bad. It tells it’s story. It’s easy to understand.

However, it’s quite likely that you won’t get to the last line, which closes the writer, from time to time. File and network I/O errors are common. For example, what if you can’t actually write to the location specified by fileName? So, we have to be more defensive. We want to be sure we always clean up.

The correct idiom is to use a try … finally … block.

    
public void writeContentToFile(String content, String fileName) throws Exception {
    File output = new File(getFileSystemPath() + contentFilename);
    OutputStreamWriter writer = null;
    try {
        writer = new OutputStreamWriter(new FileOutputStream(output), "UTF-8");
        writer.write(content);
    } finally {
        if (writer != null)
            writer.close();
    }
}

Now, no matter what happens, the writer will be closed, if it’s not null, even if writing the output was unsuccessful.

Note that we don’t necessarily need a catch block, because in this case we’re willing to let any Exceptions propagate up the stack (notice the throws clause). A lot of developers don’t realize that there are times when you need a try block, but not necessarily a catch block. This is one of those times.

So, anytime you need to clean up or otherwise release resources, use a finally block to ensure that the clean up happens, no matter what.

Comments

Leave a response

  1. Avatar
    Jaap Beetstra 26 minutes later:

    You should be careful with the constructor chaining here. If for some reason the OutputStreamWriter constructor throws an exception, the stream will nog be closed, since writer is not assigned yet. In this example it should not happen, but it would be possible if a different encoding is specified which is not necessarily available on the platform (iirc UTF-8 must be available according to the JLS).

    I usuallly use the idiom below, which has the same caveat, but is slightly more compact. 
        
    OutputStreamWriter writer = new OutputStreamWriter(new FileOutputStream(output), "UTF-8");
    try {
        writer.write(content);
    } finally {
        writer.close();
    }
  2. Avatar
    Andrew Rea about 9 hours later:

    In C# we also have a using statment block:

    using(FileStream fs1 = File.Open(@”C:\MyTricks.txt”)) { //...Bla Bla Bla }

    This automatically deals with the try, finally block in MSIL code. Nice shortcut and code/time saver in my opinion.

  3. Avatar
    Reader about 18 hours later:

    Jaap, how does your example code avoid the stream not being closed if OutputStreamWriter throws an exception? If it throws an exception the try block will never get executed and therefore the finally block will never get executed—you’re in the same situation as the original code.

  4. Avatar
    Reader about 18 hours later:

    You’ve still got a potentially unclosed stream if OutputStreamWriter throws. I think the following would cover that: FileOutputStream stream = null; try { stream = new FileOutputStream(output); writer = new OutputStreamWriter(stream, “UTF-8”); writer.write(content); } finally { if (writer != null) writer.close(); else if(stream != null) stream.close(); }

  5. Avatar
    JuanM about 19 hours later:

    This is one of the reasons why Closures would be useful for us Java Programmers, we could delegate the responsibility of closing the stream to the Stream, instead of closing it ourselves.

  6. Avatar
    Jaap Beetstra 1 day later:
    Jaap, how does your example code avoid the stream not being closed if OutputStreamWriter throws an exception?

    It doesn’t, it’s just somewhat more compact than the example in the posting.

    In the example given it is not really an issue anyway, since the constructor cannot fail (famous last words). If this a concern, such as with a different encoding, you could do it like this: 
    OutputStreamWriter writer = null;
OutputStream os= new FileOutputStream(output);
try {
  // UTF-7 might not be available
  writer = new OutputStreamWriter(os, "UTF-7");
  writer.write(content);
} finally {
  if (writer == null)
    os.close();
  else
    writer.close();
}
  7. Avatar
    Dean Wampler 3 days later:

    I guess I blew the example a bit with the nested writer/stream creation. It shows how tricky this can be to implement robustly. Good comments, all.

    JuanM is right; a much better design would be for libraries with non-trivial recovery requirements to guarantee proper clean up, with a user specified closure or worker object to do the work on the open resources. Database Connections are another example where this design would be useful.

    When I’m designing an API, I try to ensure that there is no way the user can to use it incorrectly!

  8. Avatar
    Steve Freeman 5 days later:

    Jaap is right. The pattern should be

    Resource resource = makeResourceOrFail();
try {
  doSomethingWith(resource);
} finally {
  resource.close();
}

    The only way to be really sure of all the corner cases is to insist on keeping things this clean. It’s not hard.

Comments