CODEWITHSUNDEEP
javastream
RandomUser
RandomUser

Reputation: 4220

Java, exception handling and closing streams with try, finally

I just wanted to see if there was a better way I should be handling this. My understanding of streams is that as long as you close a stream, any streams encapsulated within it will be closed which is why I only close TarArchiveOutputStream in finally. If I get FileNotFound on the rawDir or archiveDir I want to log it, otherwise anything else I want to throw.

public static void createTarGzOfDirectory(File rawDir, File archiveFile) throws IOException {
    FileOutputStream fOut = null;
    BufferedOutputStream bOut = null;
    GzipCompressorOutputStream gzOut = null;
    TarArchiveOutputStream tOut = null;
    try {
        fOut = new FileOutputStream(archiveFile);
        bOut = new BufferedOutputStream(fOut);
        gzOut = new GzipCompressorOutputStream(bOut);
        tOut = new TarArchiveOutputStream(gzOut);
        addFileToTarGz(tOut, rawDir, "");
    } catch (FileNotFoundException e) {
        log.error("File not found: " + e);
    } finally {
        if(tOut != null) {
            tOut.finish();
            tOut.close();
        }
    }

Any other considerations or thoughts on improving things?

Upvotes: 2

Views: 2611

Answers (3)

Stephen C
Stephen C

Reputation: 718788

My understanding of streams is that as long as you close a stream, any streams encapsulated within it will be closed ...

That is correct.

However, your code is (effectively) assuming that if tOut is null, then none of the other streams in the chain have been created. That's a somewhat dodgy assumption. Consider this sequence:

  1. The FileOutputStream is created and is assigned to fOut.
  2. The BufferedOutputStream is created and is assigned to bOut.
  3. The GzipCompressorOutputStream constructor throws an exception or error. (Maybe the heap is full ...).
  4. The catch is skipped ... wrong exception.
  5. The finally checks tOut, finds it is null, and does nothing.

Net result: we've leaked the file descriptor / channel held by the FileOUtputStream.

The key to getting this example (absolutely) right is to understand which of those stream objects holds the critical resources, and ensuring that THAT stream gets closed. The other streams that don't hold resources don't have to be closed.

} finally {
    if (fOut != null) {
        fOut.close();
    }
}

The other point is that you need to move the tOut.finish() call into the try block after the addFileToTarGz call.

  • If the addFileToTarGz call throws an exception, or if you don't get that far, the finish call is a waste of time.

  • The finish call will attempt to write the index to the archive, and THAT could throw an IOException. If this happens in the finally block, then any following code in the finally block to close the stream chain won't get executed ... and a file descriptor will be leaked.

Upvotes: 3

Guillaume Polet
Guillaume Polet

Reputation: 47608

Although it would look ugly and is,maybe, unlikely to be the case, you should close them all in cascade. Yes, if you close the TarArchiveOutputStream, it is supposed to close the underlyning streams. But, depending on the implementation, it may not always be the case. Moreover, and probably mainly, if one of the intermediate constructors throw an exception, tOut will be null, but the other ones may not be. Meaning that your streams are opened but your did not close any.

Upvotes: 1

Kane
Kane

Reputation: 4157

You could chain all your constructors together like so:

    tOut = new TarArchiveOutputStream(new GzipCompressorOutputStream(new BufferedOutputStream(new FileOutputStream(archiveFile))));

And save yourself 6 lines of initialization and 3 local variables for debugging. Not everyone likes chaining things that way - I personally find it more readable but the rest of your team may prefer it your way.

As far as closing the stream, it looks correct to me.

Upvotes: 0

Related Questions