Closables.closeQuietly should be deprecated or at least needs lots of warnings in documentation.

[GoogleCodeExporter]

About 95% of all the usage of closeQuietly I see is broken; closeQuietly's 
primary purpose right now appears to entice people to write buggy programs. 
That's not a good thing.

Some explanation is probably warranted. The vast majority of closables fall 
into two categories:

A) A readable; InputStream or Reader. The vast majority of implementations of 
any such streams never actually throw IOException, so the close(readable, true) 
method does nothing of any use whatsoever. closeQuietly lets you dodge the need 
to formally handle the wont-actually-ever-happen IOException, but this is 
rarely needed as the close method is usually near the read methods, which can 
and do throw IOException. Just pop the close() call into the try block and 
voila.

B) A writable; OutputStream or Writer. swallowing IOExceptions on close is very 
very very VERY bad, and something that the mere existence of closeQuietly is 
strongly enforcing. Many implementations use buffers, and if not the 
implementation itself, upstream systems might be buffering. No byte or 
character that was supplied to a 'write' method, even if that call to write did 
not cause an exception, is guaranteed to have gone anywhere until you flush the 
stream, which close implicitly does. Therefore, any exception that falls out of 
a close() method should be treated as if any number of previous write calls 
would have failed with that exception if only there were less buffers in 
between. In other words, there is no practical difference between write() 
throwing an exception and close() throwing an exception, for writables. 
closeQuietly() is an outright bug if used in this way, as it leads to programs 
silently ignoring a disk failure or network failure event. There is zero 
difference between silently ignoring IOExceptions thrown by your write() 
methods, and silently ignoring IOExceptions thrown by closing that stream or 
writer.

In conclusion, Closables.closeQuietly has vanishingly small true use cases, but 
in practice it is abused. Therefore, it should probably be removed (well, 
@Deprecated), or at least a lot of scary warnings need to be added to the 
javadoc to explain what it is for (to dodge the need to formally handle 
IOExceptions for readables, ONLY - it is inappropriate for anything else).

Closables.close does have a realistic use case (to facilitate the throwing of 
the first exception, and not the 'followup' usually less useful exception 
thrown by the close method once a write/read has already failed).

Original issue reported on code.google.com by reini...@gmail.com on 23 Aug 2012 at 6:00

[GoogleCodeExporter]

Hmm, years ago I claimed this: "closeQuietly's doc describes its main purpose 
as closing a stream that may have already thrown an exception."  But I see no 
evidence that this is true.  (And it's not even strong enough: It should be 
used only when closing a stream that *has* thrown an exception.)  We should at 
least improve the doc here.  Ideally we would also look at some users and see 
how often (quite possibly 95% of the time) callers are broken.

Original comment by cpov...@google.com on 23 Aug 2012 at 9:26

• Changed state: Accepted
• Added labels: Package-IO, Type-ApiDocs

[GoogleCodeExporter]

I looked at 25 random users -- ~15 readers and ~10 writers.  (Some both read 
and write, so it's a little fuzzy.)

- A couple were closing ByteArrayOutputStreams.  It would be nice if 
BAOS.close() were spec'ed to throw no exception, but it's at least documented 
as unnecessary.

- One was a legitimate use in a catch() block.  Another was close, but 
closeQuietly() was misused in the same file, so it's hard to call that 
"support."

- One was closing the servlet output stream, which I doubt buys you much, since 
doGet is already declared to throw IOException.

- A few were closing multiple streams.  The try-catch nesting necessary to do 
this right is horrific (or less horrific if we concede that closeQuietly(input) 
is OK), but if that's what we want to support, we should provide 
close(Closable...) instead.  Better yet, we should promote InputSupplier and 
OutputSupplier more heavily, since they make it simpler for libraries to handle 
close() for you.

- Among the readers, nearly all already declared "throws IOException" or, in a 
few cases, could easily do so (e.g., main(), setUp()).  A few were had already 
translated exceptions to different types, so they'd need to do the same for 
IOException.  (And probably they should.)

- In a couple cases, it wasn't clear to me exactly what was going on.

In short, we're looking at maybe a 90% misuse rate.  I say we kill it.

Thanks for the detailed writeup.

Original comment by cpov...@google.com on 24 Aug 2012 at 12:00

[GoogleCodeExporter]

Awesome - fast response time and great job scanning more code (and being a bit 
more verbose about it!).

Sidenote for those who stumble on this page looking for a way to handle BAOS' 
badly specced close method: Just don't close it. Calling close on a BAOS does 
nothing except flip a boolean which locks out further writes. Resources claimed 
by a BAOS get reclaimed by a garbage collector, whether it is closed or not.

Original comment by reini...@gmail.com on 24 Aug 2012 at 2:36

[GoogleCodeExporter]

I'd suggest something like `close(IOException e, Closeable... closeables)` 
which closes everything and throws e, if not null; otherwise rethrows the first 
exception encountered, if any. This is how I'd use it and could also replace 
the only use of `closeQuietly` in public Guava (in `ByteStreams.slice`).

Original comment by Maaarti...@gmail.com on 24 Aug 2012 at 4:08

[GoogleCodeExporter]

Original comment by kak@google.com on 23 Oct 2012 at 4:46

[GoogleCodeExporter]

Can someone explain what (if anything) is wrong with the example in the 
JavaDoc, assuming the stream is writable?

Let's assume the Closeable is a FileOutputStream, and specialize the example 
accordingly:

   public void useStreamNicely() throws IOException {
     FileOutputStream stream = new FileOutputStream("foo");
     boolean threw = true;
     try {
       // ... code which writes something to the file ...
       threw = false;
     } finally {
       // If an exception occurs, rethrow it only if threw==false:
       Closeables.close(stream, threw);
     }
   }

Now, what problem does this create for the caller?

I would guess none.  If one or more IOExceptions is thrown, an IOException will 
 be propagated.  I don't care whether write() or close() threw the exception.  
Is the exception from a write(), a close(), or a read() (from another source) 
somewhere in the loop?  It doesn't matter, because

1. I still want the output stream to be closed;
2. The file "foo" is presumably invalid (and I will delete it) no matter where 
the exception occurred, and
3. No useful information will be lost from the logs.

Original comment by fin...@gmail.com on 23 Oct 2012 at 6:26

[GoogleCodeExporter]

Yep, that's fine (as far as I can tell). This bug refers only to closeQuietly.

Original comment by cpov...@google.com on 23 Oct 2012 at 6:29

[GoogleCodeExporter]

What I'm finding looking through usages lines up with what Chris saw. Using 
Closeables.closeQuietly only to handle null because of initializing the 
Closeable to null outside of the try block was a common misuse.

For the case with multiple streams to close, I think a version of close like 
"close(boolean swallowIOException, Closeable... closeables)" might be 
reasonable, but it makes it seem correct to open two streams and then use that 
method to close them both, when really you need two try blocks with two 
separate calls to close if there's any chance that opening the second stream 
could throw. Like Chris said, the code to do that is truly horrible, but it's 
the only thing that's close to being correct. So really, I guess I'm pretty 
dubious about it after all.

Anyway, I think I'm in agreement that closeQuietly should die.

Original comment by cgdecker@google.com on 23 Oct 2012 at 8:33

[GoogleCodeExporter]

AFAIK you can do with a single block, or is anything wrong with this?

void useStreamNicely() throws IOException {
    FileOutputStream out1 = null;
    FileOutputStream out2 = null;
    try {
        out1 = new FileOutputStream("foo1");
        out2 = new FileOutputStream("foo2");
        // ... code which writes something to the files ...
    } catch (final IOException e) {
        Closeables.close(e, out1, out2);
    }
    // as the above call is guaranteed to re-throw
    // close get invoked exactly once
    Closeables.close(null, out1, out2);
}

It looks strange as there's no finally, yet I can't see any problem there. 
Moreover, it throws the very first exception, which might be an advantage when 
analyzing the problem.

public static void close(IOException exception, Closeable... closeables)
        throws IOException {
    Throwable result = exception;
    for (final Closeable c : closeables) {
        try {
            if (c!=null) c.close();
        } catch (final Throwable e) {
            // the very first exception wins
            if (result==null) {
                result = e;
            } else {
                addSuppressed(result, e);
            }
        }
    }
    if (result instanceof IOException) throw (IOException) result;
    if (result instanceof RuntimeException) throw (RuntimeException) result;
    if (result instanceof Error) throw (Error) result;
    // now result must be null and we're done
}

The following is optional:

private static void addSuppressed(Throwable mainException, Throwable 
suppressedException) {
    // in JDK7 addSuppressed should be used
    // which doesn't exist in JDK6
    // so we need reflection
    try {
        // do addSuppressed when in JDK7+
        Throwable.class
        .getDeclaredMethod("addSuppressed", Throwable.class)
        .invoke(mainException, suppressedException);
    } catch (final Exception ignored) {
        // we're in JDK6 and do nothing here
    }
}

Original comment by Maaarti...@gmail.com on 23 Oct 2012 at 9:44

[GoogleCodeExporter]

Hmm, I guess that would work, though I don't especially like the idea of 
needing to call Closeables.close multiple times to implement that 
correctly--the method _looks_ like something you can use just once and be fine, 
and it seems likely that too many users would misuse it once again.

The idea of using reflection to call addSuppressed is interesting, but mainly 
from the perspective of something we could do to allow suppression when closing 
streams within Guava's other utility methods. People using Java 7 really 
shouldn't be using Closeables at all.

Original comment by cgdecker@google.com on 23 Oct 2012 at 10:07

[GoogleCodeExporter]

Calling `close` just once works with a single resource, but it's not perfect as 
you need the boolean. With more resources it needs nesting and maybe even 
multiple booleans - it's something I could imagine myself doing wrongly.

I don't think that calling close twice could be misused or easily forgotten. 
People are used to consider the two cases (normal exit and throwing) and if 
what they need to is (about) the same in both cases, it should be fine. Surely 
everybody would wonder why the duplication is necessary, but it's something 
easy to remember and really hard to get wrong.

A non-duplicating solution would require something like `try {...} finally 
(Exception e) {...}` where `e` would be null in case of a normal exit. Sure, 
`@Cleanup` and JDK7 ARM are nicer.

Original comment by Maaarti...@gmail.com on 23 Oct 2012 at 11:35

[GoogleCodeExporter]

Closeables.closeQuietly is now deprecated and will be removed in Guava 16.0 
(two releases after next release rather than the usual one... it's been around 
quite a while and is the most popular method in common.io internally.)

Original comment by cgdecker@google.com on 25 Oct 2012 at 11:01

• Changed state: Fixed
• Added labels: Milestone-Release14

[GoogleCodeExporter]

I've created issue 1184 to carry on some of the discussion from here about a 
better way of dealing with Closeables pre-JDK7. Please let me know if you have 
any thoughts on that.

Original comment by cgdecker@google.com on 1 Nov 2012 at 8:47

[GoogleCodeExporter]

Could you introduce variants for safe uses of closeQuietly, e.g., for 
InputStream and Reader?

Original comment by g...@maginatics.com on 17 Dec 2012 at 7:19

[GoogleCodeExporter]

re: comment 14: 

any particular use case? normally, just pop the close call into the try block 
that contains the read calls.

it never actually throws anything, it just declares that it does.

Original comment by reini...@gmail.com on 18 Dec 2012 at 10:24

[GoogleCodeExporter]

@gaul: I don't want to encourage any kind of use of closeQuietly at this point. 
I think that Closer (which we should be getting out in 14.0) is a better 
approach to closing than anything in Closeables for pre-JDK7 code.

Original comment by cgdecker@google.com on 18 Dec 2012 at 9:43

[GoogleCodeExporter]

@cgdecker: fair enough and I look forward to using Closer in 14.0.  My coworker 
suggested adding a reference to Glengarry Glen Ross to the Javadocs: 
http://en.wikipedia.org/wiki/Coffee's_for_closers

Original comment by g...@maginatics.com on 18 Dec 2012 at 9:48

[GoogleCodeExporter]

@gaul: This important oversight as been addressed: 
http://code.google.com/p/guava-libraries/source/detail?r=dba37ce4fc0444ba2640eb6
6f4552cb3bc4bea23

Original comment by cgdecker@google.com on 19 Dec 2012 at 10:34

[GoogleCodeExporter]

All the sample code above is dealing with output streams, and the "90% misuse" 
comment also seems to focus on output usages. However, in our code I would say 
at least half of the uses are with inputs (e.g. reading in .xml and property 
files etc).

One unfortunate aspect of going to a new API for this (Closer) is that we've 
finally got IDE support to be aware of the old way: 
https://bugs.eclipse.org/bugs/show_bug.cgi?id=381445. Time to file a new 
request I guess.

Original comment by tnor...@google.com on 21 Dec 2012 at 5:27

[GoogleCodeExporter]

kind of late on this one but putting close at the end of the try block for 
input resources is just asking for resource leaks.

this is wrong because if an exception is thrown in readFully in is not closed:

final File file = new File("input.txt");
InputStream in = null;
try {
    in = new FileInputStream(file);
    final byte[] bytes = new byte[(int)file.length()];
    ByteStreams.readFully(in, bytes);
    in.close();
} catch (IOException e) {
    //handle e
}

you have to do this:

final File file = new File("input.txt");
InputStream in = null;
try {
    in = new FileInputStream(file);
    final byte[] bytes = new byte[(int)file.length()];
    ByteStreams.readFully(in, bytes);
    in.close();
} catch (IOException e) {
    try {
        if (in != null) {
            in.close();
        }
    } catch (IOException e2) {
        //ignore or log
    }
    //handle e
}

or this:

final File file = new File("input.txt");
InputStream in = null;
try {
    in = new FileInputStream(file);
    final byte[] bytes = new byte[(int)file.length()];
    ByteStreams.readFully(in, bytes);
} catch (IOException e) {
    //handle e
} finally {
    try {
        if (in != null) {
            in.close();
        }
    } catch (IOException e) {
        //ignore or log
    }
}

which is really quite verbose, when compared to:

final File file = new File("input.txt");
InputStream in = null;
try {
    in = new FileInputStream(file);
    final byte[] bytes = new byte[(int)file.length()];
    ByteStreams.readFully(in, bytes);
} catch (IOException e) {
    //handle e
} finally {
    Closeables.closeQuietly(in);
}

I agree that Closer is much better when dealing with multiple resources because 
it provides exception safety but it's overly verbose if you only have one 
resource.

I think the problem here isn't Closeables.closeQuietly, it's that java 
programmers generally don't know how to write exception safe code that doesn't 
leak resources.  If you look at close() usage in the wild I think you'll find 
that people didn't write better error handling code before the existence of 
Closeables.closeQuietly.  The fact that it is abused and misused doesn't mean 
that removing it will magically make people write better code.  The best way to 
get people to write correct error handling code is to make it easy and terse to 
do so.  Closeables.closeQuietly helps with that in a lot of situations.  Closer 
also helps, and try-with-resources helps even more, but Closeables.closeQuietly 
is still a valuable tool in the toolbox even with those other things.

Original comment by jplaisa...@indeed.com on 30 Aug 2013 at 9:32

[GoogleCodeExporter]

we use closeQuietly extensively. relying on the fact that close can be called 
multiple times safely we do something like:

OutputStream out = null;
try {
    out = new ...
    // do stuff here with out
    out.close();
} finally {
    Closeables.closeQuietly(out);
}

it servers 2 purposes: 1) handle possible null and 2) suppress exception in 
close if another one is already thrown.

i always thought this pattern was sound. is it not?

now we have to go replace this all over codebase because suddenly its 
deprecated, and will be gone in 2 releases, which is very quick. in my opinion 
that sucks for what is supposed to be a stable extension to java libraries.

Original comment by ko...@tresata.com on 3 Dec 2013 at 6:03

[GoogleCodeExporter]

FYI I just noticed there seems to be closeQuietly methods in Guava 17 which 
talk specifically about being useful when closing readers/inputstreams:

http://docs.guava-libraries.googlecode.com/git/javadoc/com/google/common/io/Clos
eables.html#closeQuietly(java.io.InputStream)

Original comment by tnor...@google.com on 8 May 2014 at 6:14

[GoogleCodeExporter]

This issue has been migrated to GitHub.

It can be found at https://github.com/google/guava/issues/<id>

Original comment by cgdecker@google.com on 1 Nov 2014 at 4:13

• Added labels: MigratedToGitHub

[GoogleCodeExporter]

Original comment by cgdecker@google.com on 3 Nov 2014 at 9:08

• Added labels: Restrict-AddIssueComment-Commit