Closing a Java FileInputStream

Question

Alright, I have been doing the following (variable names have been changed):

FileInputStream fis = null;
try
{
    fis = new FileInputStream(file);

    ... process ...

}
catch (IOException e)
{
    ... handle error ...
}
finally
{
    if (fis != null)
        fis.close();
}

Recently, I started using FindBugs, which suggests that I am not properly closing streams. I decide to see if there's anything that can be done with a finally{} block, and then I see, oh yeah, close() can throw IOException. What are people supposed to do here? The Java libraries throw too many checked exceptions.

Answer 1

For Java 7 and above try-with-resources should be used:

try (InputStream in = new FileInputStream(file)) {
  // TODO: work
} catch (IOException e) {
  // TODO: handle error
}

If you're stuck on Java 6 or below...

This pattern avoids mucking around with null:

    try {
        InputStream in = new FileInputStream(file);
        try {
            // TODO: work
        } finally {
            in.close();
        }
    } catch (IOException e) {
        // TODO: error handling
    }

---

For a more detail on how to effectively deal with close, read this blog post: Java: how not to make a mess of stream handling. It has more sample code, more depth and covers the pitfalls of wrapping close in a catch block.

Answer 2

Something like the following should do it, up to you whether you throw or swallow the IOException on attempting to close the stream.

FileInputStream fis = null;
try
{
    fis = new FileInputStream(file);

    ... process ...


}
catch (IOException e)
{
    ... blah blah blah ...
}
finally
{
    try
    {
        if (fis != null)
            fis.close();
    }
    catch (IOException e)
    {
    }
}

Answer 3

You could use the try-with-resources feature added JDK7. It was created precisely to deal with this kind of things

static String readFirstLineFromFile(String path) throws IOException {
  try (BufferedReader br = new BufferedReader(new FileReader(path))) {
    return br.readLine();
  }
}

The documenation says:

The try-with-resources statement ensures that each resource is closed at the end of the statement.

Answer 4

You could also use a simple static Helper Method:

public static void closeQuietly(InputStream s) {
   if (null == s) {
      return;
   }
   try {
      s.close();
   } catch (IOException ioe) {
      //ignore exception
   }
}

and use this from your finally block.

Answer 5

Nothing much to add, except for a very minor stylistic suggestion. The canonical example of self documenting code applies in this case - give a descriptive variable name to the ignored IOException that you must catch on close().

So squiddle's answer becomes:

public static void closeQuietly(InputStream s) {
   try {
      s.close();
   } catch (IOException ignored) {
   }
}

Answer 6

In most cases, I find it is just better not to catch the IO exceptions, and simply use try-finally:

final InputStream is = ... // (assuming some construction that can't return null)
try {
    // process is
    ...
} finally {
    is.close();
}

Except for FileNotFoundException, you generally can't "work around" an IOException. The only thing left to do is report an error, and you will typically handle that further up the call stack, so I find it better to propagate the exception.

Since IOException is a checked exception, you will have to declare that this code (and any of its clients) throws IOException. That might be too noisy, or you might not want to reveal the implementation detail of using IO. In that case, you can wrap the entire block with an exception handler that wraps the IOException in a RuntimeException or an abstract exception type.

Detail: I am aware that the above code swallows any exception from the try block when the close operation in the finally block produces an IOException. I don't think that is a big problem: generally, the exception from the try block will be the same IOException that causes the close to fail (i.e. it is quite rare for IO to work fine and then fail at the point of closing). If this is a concern, it might be worth the trouble to "silence" the close.

Answer 7

The following solution correctly throws an exception if close fails without hiding a possible exception before the close.

try {
    InputStream in = new FileInputStream(file);
    try {
        // work
        in.close();
    } finally {
        Closeables.closeQuietly(in);
    }
} catch(IOException exc) {
    // kernel panic
}

This works because calling close a second time has no effect.

This relies on guava Closeables, but one can write its own closeQuietly method if preferred, as shown by squiddle (see also serg10).

Reporting a close error, in the general case, is important because close might write some final bytes to the stream, e.g. because of buffering. Hence, your user wants to know if it failed, or you probably want to act somehow. Granted, this might not be true in the specific case of a FileInputStream, I don't know (but for reasons already mentioned I think it is better to report a close error if it occurs anyway).

The above code is a bit difficult to grasp because of the structure of the embedded try blocks. It might be considered clearer with two methods, one that throws an IOException and one that catches it. At least that is what I would opt for.

private void work() throws IOException {
    InputStream in = new FileInputStream(file);
    try {
        // work
        in.close();
    } finally {
        Closeables.closeQuietly(in);
    }
}

public void workAndDealWithException() {
    try {
        work();
    } catch(IOException exc) {
        // kernel panic
    }
}

Based on http://illegalargumentexception.blogspot.com/2008/10/java-how-not-to-make-mess-of-stream.html (referenced by McDowell).

Answer 8

Hopefully we will get closures in Java some day, and then we will lose lots of the verbosity.

So instead there will be a helper method somwhere in javaIO that you can import, it will probably takes a "Closable" interface and also a block. Inside that helper method the try {closable.close() } catch (IOException ex){ //blah} is defined once and for all, and then you will be able to write

 Inputstream s = ....;
 withClosable(s) {
    //your code here
 }

Answer 9

Are you concerned primarily with getting a clean report from FindBugs or with having code that works? These are not necessarily the same thing. Your original code is fine (although I would get rid of the redundant if (fis != null) check since an OutOfMemoryException would have been thrown otherwise). FileInputStream has a finalizer method which will close the stream for you in the unlikely event that you actually receive an IOException in your processing. It's simply not worth the bother of making your code more sophisticated to avoid the extremely unlikely scenario of

1. you get an IOException and
2. this happens so often that you start to run into finalizer backlog issues.

Edit: if you are getting so many IOExceptions that you are running into problems with the finalizer queue then you have far far bigger fish to fry! This is about getting a sense of perspective.