-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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
Comments
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: |
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.)
In short, we're looking at maybe a 90% misuse rate. I say we kill it. Thanks for the detailed writeup. |
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. |
Original comment posted by Maaartinus on 2012-08-24 at 04:08 PM I'd suggest something like |
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 |
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 { 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
|
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. |
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. |
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 { 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) The following is optional: private static void addSuppressed(Throwable mainException, Throwable suppressedException) { |
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. |
Original comment posted by Maaartinus on 2012-10-23 at 11:35 PM Calling 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 |
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: |
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. |
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? |
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. |
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. |
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 |
Original comment posted by cgdecker@google.com on 2012-12-19 at 10:34 PM @gaul: This important oversight as been addressed: dba37ce |
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. |
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"); you have to do this: final File file = new File("input.txt"); or this: final File file = new File("input.txt"); which is really quite verbose, when compared to: final File file = new File("input.txt"); 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 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; 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 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: |
[FIX] IOTools.ignorantClose usages, see google/guava#1118
[FIX] IOTools.ignorantClose usages, see google/guava#1118
[FIX] IOTools.ignorantClose usages, see google/guava#1118
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).
The text was updated successfully, but these errors were encountered: