Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
gissuebot opened this issue Oct 31, 2014 · 22 comments
Assignees
Labels
Milestone

Comments

@gissuebot
Copy link

Original issue created by reinierz on 2012-08-23 at 06:00 PM


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).

@gissuebot
Copy link
Author

Original comment posted by cpovirk@google.com on 2012-08-23 at 09:26 PM


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.


Status: Accepted
Labels: Package-IO, Type-ApiDocs

@gissuebot
Copy link
Author

Original comment posted by cpovirk@google.com on 2012-08-24 at 12:00 AM


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.

@gissuebot
Copy link
Author

Original comment posted by reinierz on 2012-08-24 at 02:36 AM


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.

@gissuebot
Copy link
Author

Original comment posted by Maaartinus on 2012-08-24 at 04:08 PM


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).

@gissuebot
Copy link
Author

Original comment posted by kak@google.com on 2012-10-23 at 04:46 PM


(No comment entered for this change.)


Owner: cgdecker@google.com

@gissuebot
Copy link
Author

Original comment posted by finnw1 on 2012-10-23 at 06:26 PM


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.

@gissuebot
Copy link
Author

Original comment posted by cpovirk@google.com on 2012-10-23 at 06:29 PM


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

@gissuebot
Copy link
Author

Original comment posted by cgdecker@google.com on 2012-10-23 at 08:33 PM


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.

@gissuebot
Copy link
Author

Original comment posted by Maaartinus on 2012-10-23 at 09:44 PM


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
    }
}

@gissuebot
Copy link
Author

Original comment posted by cgdecker@google.com on 2012-10-23 at 10:07 PM


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.

@gissuebot
Copy link
Author

Original comment posted by Maaartinus on 2012-10-23 at 11:35 PM


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.

@gissuebot
Copy link
Author

Original comment posted by cgdecker@google.com on 2012-10-25 at 11:01 PM


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.)


Status: Fixed
Labels: Milestone-Release14

@gissuebot
Copy link
Author

Original comment posted by cgdecker@google.com on 2012-11-01 at 08:47 PM


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.

@gissuebot
Copy link
Author

gissuebot commented Nov 1, 2014

Original comment posted by g...@maginatics.com on 2012-12-17 at 07:19 PM


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

@gissuebot
Copy link
Author

Original comment posted by reinierz on 2012-12-18 at 10:24 AM


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.

@gissuebot
Copy link
Author

Original comment posted by cgdecker@google.com on 2012-12-18 at 09:43 PM


@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.

@gissuebot
Copy link
Author

gissuebot commented Nov 1, 2014

Original comment posted by g...@maginatics.com on 2012-12-18 at 09:48 PM


@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

@gissuebot
Copy link
Author

Original comment posted by cgdecker@google.com on 2012-12-19 at 10:34 PM


@gaul: This important oversight as been addressed: dba37ce

@gissuebot
Copy link
Author

Original comment posted by tnorbye@google.com on 2012-12-21 at 05:27 PM


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.

@gissuebot
Copy link
Author

gissuebot commented Nov 1, 2014

Original comment posted by jplaisa...@indeed.com on 2013-08-30 at 09:32 PM


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.

@gissuebot
Copy link
Author

gissuebot commented Nov 1, 2014

Original comment posted by ko...@tresata.com on 2013-12-03 at 06:03 PM


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.

@gissuebot
Copy link
Author

Original comment posted by tnorbye@google.com on 2014-05-08 at 06:14 PM


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

https://google.github.io/guava/apidocs/com/google/common/io/Closeables.html#closeQuietly(java.io.InputStream)

@cgdecker cgdecker modified the milestone: 14.0 Nov 1, 2014
TWiStErRob referenced this issue in bumptech/glide Apr 10, 2015
TWiStErRob added a commit to TWiStErRob/net.twisterrob.libraries that referenced this issue Feb 24, 2023
TWiStErRob added a commit to TWiStErRob/net.twisterrob.inventory that referenced this issue Feb 24, 2023
TWiStErRob added a commit to TWiStErRob/net.twisterrob.travel that referenced this issue Nov 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants