Flushables.flushQuietly(Flushable) doesn't check for null (and doesn't use @Nullable)

[GoogleCodeExporter]

The Problem:

Closeables.closeQuietly(Closeable) is null-safe, whereas 
Flushables.flushQuietly(Flushable) isn't.

The Solution:

In Flushables, this (which is invoked by flushQuietly):

    public static void flush(Flushable flushable, boolean swallowIOException)
      throws IOException {
    try {
      flushable.flush();
    } catch (IOException e) {
      if (swallowIOException) {
        logger.log(Level.WARNING,
            "IOException thrown while flushing Flushable.", e);
      } else {
        throw e;
      }
    }
  }

Should become this (to replicate Closeables.close(Closeable, boolean)):

  public static void flush(@Nullable Flushable flushable, boolean swallowIOException)
      throws IOException {
    if (flushable == null) {
      return;
    }
    try {
      flushable.flush();
    } catch (IOException e) {
      if (swallowIOException) {
        logger.log(Level.WARNING,
            "IOException thrown while flushing Flushable.", e);
      } else {
        throw e;
      }
    }
  }

(changes are a simple null-check and @Nullable on the first parameter).

Would probably need a Javadoc update as well.

Possibly invalid:

I was surprised by this inconsistency and thought it was probably by design as 
I assume something like this would have been noticed already, considering these 
are rather often used (I think), but as I couldn't find a reference of this 
already mentioned somewhere (or anyone else already bitten by it), I figured 
I'd report it anyways.

Apologies if this is invalid or was already debated and rejected.

Original issue reported on code.google.com by laurent....@gmail.com on 26 Jul 2012 at 4:09

[GoogleCodeExporter]

I just verified that this still exists in trunk and in the release 13 RC2.

So whoever has edit rights on tickets can update this to also have the laevl 
Milestone-Release13.

Current trunk is: 
http://code.google.com/p/guava-libraries/source/browse/guava/src/com/google/comm
on/io/Flushables.java

Release 13 RC2 is: 
http://code.google.com/p/guava-libraries/source/browse/guava/src/com/google/comm
on/io/Flushables.java?name=v13.0-rc2#53

Original comment by laurent....@gmail.com on 26 Jul 2012 at 4:12

[GoogleCodeExporter]

Sounds reasonable...won't make it in for r13 though, but it'll be in r14.

Original comment by kak@google.com on 26 Jul 2012 at 4:18

• Changed state: Accepted
• Removed labels: Milestone-Release12

[GoogleCodeExporter]

Interesting.

Closeables.close accepts null in order to support a specific usage pattern, 
albeit not the one that the Javadoc covers.  The Javadoc covers something like 
this:

  FileOutputStream out = new FileOutputStream("foo");
  try {
    // do work with out
  } finally {
    closeQuietly(out);
  }

But we can imagine that you might want to catch any IOException now, rather 
than later, in which case you'd write (h/t 2009-era Jesse Wilson):

  FileOutputStream out = null;
  try {
    out = new FileOutputStream("foo");
    // do work with out
  } catch (IOException e) {
    // handle this failure. There was a problem opening or writing to the file
  } finally {
    closeQuietly(out);
  }

Would a situation like this arise for flush()?

Original comment by cpov...@google.com on 26 Jul 2012 at 4:24

[GoogleCodeExporter]

I don't know, and I don't know this Jesse Wilson gentleman (the - now - Android 
guy, I assume?), but just noticed this when writing some code and showing it to 
a co-worker who had noticed this behavior himself just recently, and it was 
indeed code following the second pattern.

As it was in old code that I was cleaning up, I chose to closeQuietly and as I 
wasn't sure if the stream implementation used would flush on close, I chose to 
do it explicitly just in case for the time being. And I tend to write "2009-era 
Jesse Wilson" code, apparently.

So, I guess this can arise for Flushables. :)
Doesn't mean it's warranted though.

Original comment by laurent....@gmail.com on 26 Jul 2012 at 4:44

[GoogleCodeExporter]

(Yes, the Android guy.)

I admit that I'm arguing from a position of ignorance here in that I'm not sure 
I've ever seen a legitimate flush() call.  I just searched over the internal 
Google codebase for Flushables users, and all seem to fall into one of two 
categories:

- People who are implementing an interface that includes a flush() method.  
Good for them, but this doesn't tell us who ultimately calls flush() :)

- People who call flush() immediately before close(). I'm pretty sure that this 
is supposed to be unnecessary, though naturally I can't find explicit 
documentation to that effect.

We might imagine that someone would write "one record" and then flush.  
However, the documentation for flush() emphasizes that the flushed data might 
not have made it to disk, so the whole thing seems to be best effort, anyway :\

http://docs.oracle.com/javase/6/docs/api/java/io/OutputStream.html#flush%28%29

Anyway, if I'm right that the @Nullable argument serves only to support the 
specific usage pattern above, then I don't think it's likely to come up with 
flush().  It seems to matter only when you open a file in a try block and close 
it in a finally block.  Unless the flush() appears in the finally block, 
there's no way for the stream to be null.  But if it appears in the finally 
block, it immediately precedes close(), so it should be unnecessary.  The 
analogy to Closeables ends up not quite holding up, thanks to the weird 
situation that Closeables was designed for.

Original comment by cpov...@google.com on 26 Jul 2012 at 9:47

[GoogleCodeExporter]

Possibly we should update the Closeables doc to show off this 
jessewilson/laurent.malvert technique instead of or in addition to the simpler 
pattern.

Original comment by cpov...@google.com on 26 Jul 2012 at 9:49

[GoogleCodeExporter]

Regarding "it immediately precedes close(), so it should be unnecessary." I was 
having a discussion with a couple of people and the conclusion was:

1. Nobody knows if there's a stream "out there" not flushing when closed.
2. There's no guarantee, javadoc says nothing about it so we *must* always call 
flush.
3. `Writer.close()` and `FilterOutputStream.close()` document the flushing,, 
`OutputStream.close()` says nothing about it, why?

So I can imagine that many people chose to always call flush before close.

Original comment by Maaarti...@gmail.com on 28 Jul 2012 at 8:31

[GoogleCodeExporter]

If you call flush at the end of the try block instead of in the finally block, 
can't you avoid this?

Original comment by kurt.kluever on 7 Aug 2012 at 5:51

• Changed state: Research

[GoogleCodeExporter]

Except you may never reach the end of the try block, and therefore never flush 
before the close.  The exception may not be thrown by the output stream you 
wish to flush quietly but you may still want to flush what output you have and 
close the stream.

Original comment by pdav...@karn.co.uk on 7 Aug 2012 at 5:55

[GoogleCodeExporter]

I still can't believe that an OutputStream implementation would require a 
flush() before close().  I understand that it's not documented that way, but it 
feels like the sort of thing that's undocumented simply because it's taken for 
granted.  I'm not thrilled that that's the best argument I have, but it would 
seem at *least* as crazy for flush() to be required but for that not to be 
documented.

Original comment by cpov...@google.com on 7 Aug 2012 at 6:57

[GoogleCodeExporter]

I'm with Chris on this one.

Can anybody point to real-world examples of such "non-well-behaved" 
OutputStreams?

Original comment by stephan...@gmail.com on 7 Aug 2012 at 8:32

[GoogleCodeExporter]

> Except you may never reach the end of the try block, and therefore never 
flush before the close.  The exception may not be thrown by the output stream 
you wish to flush quietly but you may still want to flush what output you have 
and close the stream.

I really think you shouldn't be attempting to flush() if you got an exception 
during the try block, regardless of whether it was the output stream that threw 
the exception or not. What's the point? You got an exception in the block, so, 
depending on what you're doing and the stream implementation, one of a few 
things are possible:

- You hadn't written anything at all yet. Doesn't matter if you call flush() or 
not.
- You started writing but didn't finish due to the exception. The amount you 
succeeded in writing is essentially arbitrary; a flush() call serves at most to 
write out a little more of that arbitrary amount than would otherwise have been 
written.
- The exception did occur in a write call to the output stream; if flush() does 
anything at all, it'll probably just throw too. Otherwise, the above applies.
- The exception occurred after you'd finished writing to the output stream but 
before the end of the try block because you're doing something else afterwards. 
In this case, you could just put a flush() call after you finish writing and 
before the rest of the block.

All of this is moot, though, given that as far as we know, all 
OutputStream/Writer implementations that need to flush data ensure they do so 
during close(). I'm in favor of closing this issue.

Original comment by cgdecker@google.com on 11 Mar 2013 at 9:14

[GoogleCodeExporter]

Marking as obsolete now that closeQuietly itself is going away (issue 1118).

Original comment by cpov...@google.com on 10 Dec 2013 at 9:00

• Changed state: Obsolete

[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