Clean Code: refactoring a double try block

I recently cleaned up some old code and during this adventure, I came across a couple of lines that looked kind of ugly and potentially broken to me. As we refactored this code, we found out that it was not only bad code, but depending on the situation it could also swallow an exception. In this article, I’ll tell you what the problem was and how we resolved it.

Take the following code as an abstract example of what we found:

public class Test {
 
    public static void main(String[] args) {
        try {
            try {
                throw new NullPointerException();
            } finally {
                throw new IllegalArgumentException();
            }
        } catch(Exception e) {
            e.printStackTrace();
        }
    }
}

You may ask yourself which of the two exceptions will reach the catch block:

  • Both exceptions?
  • Only the NullPointerException?
  • Only the IllegalArgumentException?
  • No exception at all?

If you run this code, you will see this in the console:

java.lang.IllegalArgumentException
    at Test.main(Test.java:9)

It’s obvious that the original NullPointerException is “swallowed”, it will never reach the catch block. Additionally, nested try blocks are quite undesirable, at least in this case we’re looking at. At least in the example above, Eclipse will provide you with a warning because the finally block doesn’t complete normally.

Now let’s look at the original context and the original code:

FileOutputStream out = null;
try {
    try {
        out = new FileOutputStream(filename);
        out.write(fileContent);
    } finally {
        if (out != null) {
            out.close();
        }
    }
} catch (IOException e) {
    logger.error("Error writing to " + filename + ": " + e.getMessage(), e);
}

What’s bad about this code? First, we have the same try-try-sequence as in the abstract example given earlier. This can potentially lead to a situation where the original exception gets swallowed. Also for this code, you will never get a warning from Eclipse: FileOutputStream#close() might throw an IOException, but it would be caught by the catch block, thus Eclipse doesn’t warn you about the finally block. This code has another flaw: you cannot precisely determine whether it’s ever going to behave the way you want it to.

In order to improve this code, we moved the finally block behing the catch block. Additionally, we put IOUtils#closeQuietly(OutputStream) in place in order to knowingly ignore potential exceptions while closing the stream. The result:

FileOutputStream out = null;
try {
    out = new FileOutputStream(filename);
    out.write(fileContent);
} catch (IOException e) {
    logger.error("Error writing to " + filename + ": " + e.getMessage(), e);
} finally {
    IOUtils.closeQuietly(out);
}

This code looks and works much better than before and it definitely does what it’s supposed to do.

Keep in mind that closeQuietly is not the ever-perfect tool in every potential context of closing streams. You should only ever use closeQuietly if you are absolutely sure that you want to ignore an exception and why you want to ignore it. Should you intend to log a message or do anything else in order to handle that exception, you should definitely put a try-catch into the finally block. You may imagine the resulting code for yourself :)

  • Facebook
  • Delicious
  • Digg
  • StumbleUpon
  • Reddit
  • Blogger
  • LinkedIn
Robert Spielmann

9 Responses to Clean Code: refactoring a double try block

  1. Peter says:

    You definitely don’t want to ignore close() exceptions when closing output stream … and yes, they happen.

    I usually do:

    FileOutputStream out = null;
    try {
        out = new FileOutputStream(filename);
        out.write(fileContent);
     
        out.close();
        out = null;
    } catch (IOException e) {
        logger.error("Error writing to " + filename + ": " + e.getMessage(), e);
    } finally {
        IOUtils.closeQuietly(out);
    }

    This way, if close() inside try throws exception, it is thrown out, but I still quietly close stream if exception occur earlier, because there is already another exception.

    What do you think about it?

  2. amrr says:

    Robert, I found your article really interesting. I was surprised by the loss of that nullpointerexception, or at least unsure of what to expect.

    It makes me think of the possibiltiy of losing rethrown exceptions, and the importance of error-free finally blocks.

    i.e.

    try {
       xyz();
    } catch(Exception e) {
       log(e);
       throw new XxxException("Problem doing xyx();",e);
    } finally {
      bugWhichThrowsRuntimeException();
    }

    … the calling code would never see the XxxException. Maybe this is an unusual use-case, but it seems noteworthy to me.


    Peter is right: you need to call close() in the main try block, otherwise the fileContent may never be written to disk, and nothing would even be logged. I think you should add this to your code sample, then it will stand up as a really good article.

  3. J.C. says:

    Peter definitely has the right idea. Peter’s example is the only real way to deal with close properly. Here’s the full code block. You can feel free to refactor the finally into a method if you wish.

    FileOutputStream out = null;
    try {
      out = new FileOutputStream(filename);
      out.write(fileContent);
      out.close();
      out = null;
    } finally {
      if (out != null) {
        try {
          out.close();
        } catch (IOException) {
          ExceptionHandler.handleSecondaryException("Secondary exception while writing to file: " + filename, e);
        }
        out = null;
      }
    }
  4. Raja Kannappan says:

    Java 7 would handle this problem through automatic resource management.

    http://mail.openjdk.java.net/pipermail/coin-dev/2009-February/000011.html

  5. I like your example. Thanks!

  6. slava says:

    So, you’ve basically changed nothing. If you’d open the IOUtils source, you’d see following:

    public static void closeQuietly(Closeable closeable) {
      try {
        if (closeable != null) {
          closeable.close();
        }
      } catch (IOException ioe) {
        // ignore
      }
    }

    Therefore your code is effectively the following:

    FileOutputStream out = null;
    try {
        out = new FileOutputStream(filename);
        out.write(fileContent);
    } catch (IOException e) {
        logger.error("Error writing to " + filename + ": " + e.getMessage(), e);
    } finally {
        try {
            if (out != null) {
                out.close();
            }
        } catch (IOException ioe) {}
    }

    My opinion, the original code is better as it at least logs the exception thrown while closing the stream, and your code don’t.

    The interesting for us part of the java FileOutputStream implementation is:

    private native void close0() throws IOException;

    You really cannot deal effectively with the IOException while closing your stream. It doesn’t matter, when you’ve got the exception. Your file
    is corrupted or don’t exists anyway. Why don’t you just do that:

    try {
        FileOutputStream out = new FileOutputStream(filename);
        try {
            out.write(fileContent);
        } finally {
            out.close();
        }
    } catch (IOException e) {
        logger.error("Error writing to " + filename + ": " + e.getMessage(), e);
    }

    Or alternatively (which i would not recommend, yes i no this is not really safe), you can let the finalize method of the FileOutputStream do the job for you.
    Then your code could be:

     try {
         FileOutputStream out = new FileOutputStream(filename);
         out.write(fileContent);
         out.close();
     } catch (IOException e) {
         logger.error("Error writing to " + filename + ": " + e.getMessage(), e);
     }
  7. Chris says:

    Because in your code…

    try {
        FileOutputStream out = new FileOutputStream(filename);
        try {
            out.write(fileContent);
        } finally {
            out.close();
        }
    } catch (IOException e) {
        logger.error("Error writing to " + filename + ": " + e.getMessage(), e);
    }

    if the write() throws an IOException and then in the finally block, the close() also throws an IOException your catch block will only get the IOException from the close(). The first exception is lost.
    J.C’s code is the best here as it deals with all situations.

  8. Fabian Lange says:

    Hi Folks,
    thanks for the great discussion.
    Let’s attack this backwards.
    I looked up the actual implementation of close for File Input Stream on Windows and Solaris from the OpenJDK:
    its in io_util_md.c
    Windows:

    if (CloseHandle(h) == 0) { /* Returns zero on failure */
        SET_FD(this, fd, fid); // restore fd
        JNU_ThrowIOExceptionWithLastError(env, "close failed");
    }

    And people actually seem never to have managed to get this:
    http://www.techreplies.com/development-resources-58/can-closehandle-virtualfree-fail-381270/#

    Solaris:

    if (JVM_Close(fd) == -1) {
        SET_FD(this, fd, fid); // restore fd
        printf("JVM_Close returned -1\n");
        JNU_ThrowIOExceptionWithLastError(env, "close failed");
    }

    Here the other side. Somebody reported that this actually should be handled:
    http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4952229
    (note the forgotten debug output)

    So when writing to local disk this is unlikely to ever happen. OutputStream declares this, because there might be buffering implementations that need to flush() on close().

    But the Statement, that you cannot do anything is not correct. Just logging is poor mans exception handling. Logging gives no extra value. That is why Robert used IOUtils and this is perfect for that.
    You could retry or resort to a different storage in case of error.

    I actually do not see the point of coding out.close() twice, especially in regard of code complexity. It will only happen once. And it should happen always. So the correct location is the finally block. If you want to do something you might want to add a try catch finally inside the finally block.

    But this gets messy if you copy from one stream to another.

    To sum up: If you are not doing real error recovery, I consider Roberts solution to be the best.
    If you are planning for sensible error recovery you should be aware of something: the IOException can occur somewhere else. Eg from a Serialisation call. You certainly do not want to react to that one by retrying writing.

    FileOutputStream out = null;
    handleWriteError = false;
    try {
        out = new FileOutputStream(filename);
        out.write(fileContent);
    } catch (IOException e) {
        // error while writing to filename
        handleWriteError = true;
    } finally {
        try {
            out.close();
        } catch (IOException e) {
            // rare error when closing
            handleWriteError = true;
        }
    }
    if (handleWriteError) {
        File failed = new File(filename);
        if (failed.exists()) {
            if (!file.delete()) {
                sendAdminAlert("Cleanup of failed write failed. Could not remove corrupted file at "+filename);
            }
        }
        writeToSecondaryStore(filename, fileContent);
    }

    PS: One should actually really consider using BufferedXyz for performance reasons.

  9. Fabian Lange says:

    PPS:
    My favorite Solution is: rethrow the Exception
    For many people this is not that acceptable, but it is often easier than to write good Exception Handling in the method causing it.

Leave a Reply

Your email address will not be published. Required fields are marked *

You may use these HTML tags and attributes: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <strike> <strong>