views:

537

answers:

8

Is there a not so ugly way of treat the close() exception to close both streams then:

    InputStream in = new FileInputStream(inputFileName);
    OutputStream out = new FileOutputStream(outputFileName);

    try {
    copy(in, out);
    } finally {
    try {
        in.close();
    } catch (Exception e) {
        try {
        // event if in.close fails, need to close the out
        out.close();
        } catch (Exception e2) {}
        throw e; // and throw the 'in' exception
    }
    out.close();
    }

update: All the above code is within one more try-catch, thanks for the warnings.

--FINALLY (after the answers)

And a good utility method can be done using Execute Around idiom (thanks Tom Hawtin).

+2  A: 

One trick I sometimes use is to define a method called closeQuietly(Closeable) that tests to see if its argument is null then closes it, ignoring any exceptions. But you need to be a careful closing OutputStreams and Writers that way because they may actually throw an exception that matters; e.g. if the final flush fails.

Things are likely to improve with Java 7. Reports are that it will have a new construct that provides a more concise way of handling managed resources; e.g. streams that need to be closed when they are finished with.

Finally, you should be aware that your example has a bug. If the method call to open the second stream, the first stream will not be closed. The second open needs to be done inside the try block.

Stephen C
+18  A: 

You could implement a utility method:

public final class IOUtil {
  private IOUtil() {}

  public static void closeQuietly(Closeable... closeables) {
    for (Closeable c : closeables) {
        if (c != null) try {
          c.close();
        } catch(Exception ex) {}
    }
  }
}

Then your code would be reduced to:

try {
  copy(in, out);
} finally {
  IOUtil.closeQuietly(in, out);
}

Additional

I imagine there'll be a method like this in a 3rd party open-source library. However, my preference is to avoid unnecessary library dependencies unless I'm using a large portion of its functionality. Hence I tend to implement simple utility methods like this myself.

Adamski
this is slightly different, as the in exception is not rethrown
Sam Holder
@Sam: It's a good point; I'll amend my answer.
Adamski
Nice idea, a var-arg of closeables.
Yishai
Actually I changed my mind as I don't agree with propagating the first Exception whilst ignoring subsequent ones. Better to suppress all exceptions given that this is simply relinquishing resources rather than an exceptional circumstance.
Adamski
I'd add a nullcheck. The closeable resource may not have been assigned. To the OP: the Commons IO library has similar methods: http://commons.apache.org/io/api-1.4/org/apache/commons/io/IOUtils.html
BalusC
You want to close the output stream "quietly"?!
Tom Hawtin - tackline
Why have they left the IOUtils constructor public in Commons I/O? Also, this looks out of date as they haven't referenced Closeable but have overloaded the closeQuietly method with multiple types (Writer, Reader, etc).
Adamski
+1 for nice idea and a gentle Warning, Closable is 1.5 up
ring bearer
@Adamski: "I don't agree with propagating the first Exception while ignoring subsequent ones"Agree, should subclass IOException with some ChainedIOException containing all the failures if really an exception has to be raised.
Matthieu BROUILLARD
+1 I modify the code a little bit to use the if/try idiom I saw in a James Gosling code years ago.
OscarRyz
@Oscar, cool idiom, thanks.
Yishai
Why would the Java developers raise an exception if it is just to be ignored?
Tom Brito
@Tom Brito, although closing a stream is generally not an operation you want to get worked up about, logging the failure can be valuable. In some cases the correct closing is in fact what flushes the stream from a buffer and in fact things can go wrong there that are not "nothings." That is Tom Hawtin's point above.
Yishai
@Yishai so you agree with my choose of the right answer (not this)
Tom Brito
@Tom Brito, since you chose my answer, I'm not going to argue ;-)
Yishai
+1  A: 

In most cases the 'in' close() exception is irrelevant, so:

    try {
      copy(in, out);
    } finally {
    try {  in.close()  }  catch (Exception e) { /* perhaps log it */ }
    try {  out.close() }  catch (Exception e) {/* perhaps log it */ }
    } 

It's usually bad practice to swallow exceptions, but in this case I think it's ok.

leonbloy
+8  A: 

This is the correct idom (and it works fine):

   InputStream in = null;
   OutputStream out = null;
   try {
       in = new FileInputStream(inputFileName);
       out = new FileOutputStream(outputFileName);
       copy(in, out);
   finally {
       close(in);
       close(out);
   }

  public static void close(Closeable c) {
     if (c == null) return; 
     try {
         c.close();
     } catch (IOException e) {
         //log the exception
     }
  }

The reason this works fine is that the exception thrown before you got to finally will be thrown after your finally code finishes, provided that your finally code doesn't itself throw an exception or otherwise terminate abnormally.

Yishai
Note: this is not the correct idiom for all stream types. If the `OutputStream` buffers data (`BufferedOutputStream`) or it writes closing blocks (`ZipOutputStream`), you could lose data and the application would not handle it because it swallows exceptions. Logging is not a replacement for correct error handling.
McDowell
@McDowell, it is a good point regarding the output stream (see my comment to Adamski's answer that said something similar), however I would still say it is the standard idiom.
Yishai
+8  A: 
try {
    final InputStream in = new FileInputStream(inputFileName);
    try {
        final OutputStream out = new FileOutputStream(outputFileName);    
        try {
            copy(in, out);
            out.flush(); // Doesn't actually do anything in this specific case.
        } finally {
            out.close();
        }
    } finally {
        in.close();
    }
} catch (IOException exc) {
    throw new SomeRelevantException(exc);
}

Remember that opening a stream may throw an exception, so you do need a try between the stream openings (please don't do some hack involving nulls. Anything can throw an Error (which are not an instances of Exception).

It turns out that catch and finally should rarely share the same try.

You may want to use the Execute Around idiom.

I believe the standard good way to copy is using NIO's transferTo/transferFrom.

Tom Hawtin - tackline
+1, that's identical (character-for-character) to the answer I was typing! :)
Adam Paynter
+1  A: 

You have, in commons io, in IOUtils, some closeQuietly methods.

Istao
+5  A: 

Guava has very nice IO APIs that eliminate the need for this. For instance, your example would be:

Files.copy(new File(inputFileName), new File(outputFileName));

More generally, it uses the concept of InputSuppliers and OutputSuppliers to allow the InputStreams and OutputStreams to be created within its utility methods, allowing it full control over them so it can handle closing properly.

Additionally, it has Closeables.closeQuietly(Closeable) which is basically the type of method most of the answers have suggested.

The IO stuff in it is still in beta and subject to change, but it's worth checking out and even using, depending on what it is you're working on.

ColinD
+1  A: 

I strongly believe that in Java 7.0, you do not need to explicitly close the stream yourself anymore. Language Features in Java 7

try (BufferedReader br = new BufferedReader(new FileReader(path)) {
   return br.readLine();
}
vodkhang
When it's finally released, this will be terrific. You can specify multiple resources in the try(...) parameters, which are closed cleany for you by the JVM.
Sam Barnum