Closer

[GoogleCodeExporter]

We recently deprecated Closeables.closeQuietly (see issue 1118 for discussion 
on the reasons for that). Closeables.close(Closeable, boolean) makes it a bit 
easier to close Closeables correctly, but it's still too hard, especially when 
dealing with multiple Closeables. While people on JDK7 should be using 
try-with-resources to handle opening and closing resources, Guava is a JDK6 
library and we'd like to provide something to make this easier for users who 
are stuck on JDK6.

We're experimenting with a class called Closer that's intended to help with 
this. Note that the most important thing this class does (that's hard to do 
without it) is ensuring that when an exception is thrown from the main block of 
code using the resources, any exceptions that are thrown after that when 
attempting to close those resources is _suppressed_: it's not thrown, but the 
exception that was thrown is reported in some way. JDK7 provides a mechanism to 
do this on Throwable itself, while pre-JDK7 we generally have to just log the 
exception.

Here are the alternatives:

===============================================================

1. Closer that can use Throwable.addSuppressed() to suppress an exception when 
running under JDK7 or log the exception when not, imperative version:

  Closer closer = Closer.create();
  try {
    InputStream in = closer.closeLater(openInputStream());
    OutputStream out = closer.closeLater(openOutputStream());
    return copy(in, out);
  } catch (Throwable e) {
    throw closer.rethrow(e, IOException.class);
  } finally {
    closer.close();
  }

Advantages:
- can use Throwable.addSuppressed() when running under JDK7... this is mainly 
useful for internal code in a library that may be used under either JDK6 or 
JDK7, such as Guava

Disadvantages:
- having to catch and rethrow any Throwable makes this approach bulky and 
easier to misuse
- exceptions not transparent... requires checked exception types that may be 
thrown from the run() method to be declared in the call to rethrow()
- the one advantage probably isn't actually useful to most users, who should 
only be using this if using JDK6

===============================================================

2. Same as above, callback version:

  Closer.runAndClose(new Closer<Long>() {
    @Override protected Long run() throws Exception {
      InputStream in = closeLater(openInputStream());
      OutputStream out = closeLater(openOutputStream());
      return copy(in, out);
    }
  });

Advantages:
- hardest to use wrong
- least boilerplate
- same as above with regard to Throwable.addSuppressed()

Disadvantages:
- may need final variables
- generics
- anonymous inner class ugliness
- returning a value vs. not returning (Closer<Void>? a different type? no 
version that returns a value, use an AtomicReference or single-element array?)
- exceptions not transparent... requires checked exception types that may be 
thrown from the run() method to be declared somehow, or has to just not allow 
checked exceptions other than IOException

===============================================================

3. Closer that always logs exceptions that are suppressed:

  Closer closer = Closer.create();
  try {
    InputStream in = closer.closeLater(openInputStream());
    OutputStream out = closer.closeLater(openOutputStream());
    return closer.done(copy(in, out));
  } finally {
    closer.close();
  }

Advantages:
- fairly short, straightforward and simple
- exception transparency--any type of checked exception in the try block will 
be thrown as normal

Disadvantages:
- can only suppress exceptions by logging
- may be easier to forget to call closer.done()

===============================================================

We could also, for example, provide both option 1 and option 2, with option 2 
being there for simple cases and option 1 for when you have something more 
complex, though we'd rather have just one option if possible.

Any opinions on this?

Original issue reported on code.google.com by cgdecker@google.com on 1 Nov 2012 at 8:43

• Blocking: #1185

[GoogleCodeExporter]

Original comment by cpov...@google.com on 1 Nov 2012 at 8:48

• Now blocking: #1185

[GoogleCodeExporter]

Original comment by cgdecker@google.com on 1 Nov 2012 at 10:20

[GoogleCodeExporter]

I really like the 1st variant. While it's longer than the others, it's a simple 
try-catch-finally pattern, easy to type and to remember.

I'd only change one thing: As `IOException` may get thrown in `Closer.close()` 
anyway, it can be included in `Closer.rethrow` by default.

The 2nd variant is a nice functional piece, which could drive me mad in Java. 
Nor Eclipse has an easy time with it, sometimes autocomplete stops working in 
the annonymous class.

It could be "improved" by parametrizing `Closer` with an exception type. Or 
maybe two... Or better not. This would solve the exception transparency, but 
make it even harder to write and read.

A fluent variant like `Long x = new Closer<Long>() {...}.runAndClose();` would 
reduce the closing parens, braces and semicolons horror slightly.

The 3rd variant is shorter than the 1st one, but `done` is just too easy to 
forget. I guess the consequence is that no exception gets thrown, as `close` 
assumes it has already happened. Too bad, IMHO.

Original comment by Maaarti...@gmail.com on 1 Nov 2012 at 11:20

[GoogleCodeExporter]

> I'd only change one thing: As `IOException` may get thrown in 
`Closer.close()` anyway, it can be included in `Closer.rethrow` by default.

True... I had it like that at first, but then changed it for some reason.

> It could be "improved" by parametrizing `Closer` with an exception type. Or 
maybe two... Or better not. This would solve the exception transparency, but 
make it even harder to write and read.

If anything, the runAndClose() method would have overloads parameterized with 
one or two exception types. Another thing we considered was making the run() 
method only throw IOException since that seems like the most common case, 
thinking that users who need another checked exception type could use the first 
form instead.

> A fluent variant like `Long x = new Closer<Long>() {...}.runAndClose();` 
would reduce the closing parens, braces and semicolons horror slightly.

One problem there is that it's easy to call run() instead of runAndClose() 
accidentally, like with Thread and start()/run(). I also tend to dislike the 
look of newing an object and calling a method on it without assigning it to a 
variable. Passing the new object directly to a method looks better to me (in 
general).

> The 3rd variant is shorter than the 1st one, but `done` is just too easy to 
forget. I guess the consequence is that no exception gets thrown, as `close` 
assumes it has already happened. Too bad, IMHO.

Yeah, that is a concern. With the first form, there's similarly a concern that 
you'll forget to catch(Throwable) and catch(IOException) or some such instead, 
or that you'll forget to declare all the types of checked exceptions your code 
may throw in the call to rethrow() and end up with some checked exceptions 
silently wrapped as RuntimeExceptions, etc.

Original comment by cgdecker@google.com on 2 Nov 2012 at 1:30

[GoogleCodeExporter]

> Another thing we considered was making the run() method only throw 
IOException since that seems like the most common case, thinking that users who 
need another checked exception type could use the first form instead.

But then they'd have to learn both ways, which would lead to more errors. And 
the fact that the one way is needed only rarely does *not* make it better.

> > A fluent variant like `Long x = new Closer<Long>() {...}.runAndClose();` 
would reduce the closing parens, braces and semicolons horror slightly.

> One problem there is that it's easy to call run() instead of runAndClose() 
accidentally, like with Thread and start()/run().

Yes, that's bad. Unfortunately, it's allowed even with `protected`. And the 
result of a wrong usage is the worst possible: nothing gets closed. Using other 
names could help, especially renaming `run` to something long and terrible 
(executeWithResources? :D).

> I also tend to dislike the look of newing an object and calling a method on 
it without assigning it to a variable. Passing the new object directly to a 
method looks better to me (in general).

Isn't it just a prejudice? That's how StringBuilder, Joiner, Splitter, and 
FluentIterable get used, maybe most of the time.

> > The 3rd variant is shorter than the 1st one, but `done` is just too easy to 
forget. I guess the consequence is that no exception gets thrown, as `close` 
assumes it has already happened. Too bad, IMHO.

> Yeah, that is a concern. With the first form, there's similarly a concern 
that you'll forget to catch(Throwable) and catch(IOException) or some such 
instead, or that you'll forget to declare all the types of checked exceptions 
your code may throw in the call to rethrow() and end up with some checked 
exceptions silently wrapped as RuntimeExceptions, etc.

Yes, but that's relatively harmless when compared to not closing resources 
(confused `run` and `runAndClose`) or throwing no exception at all (forgotten 
`done`, then an exception in `close` gets swallowed in the assumption there's 
already one pending).

Maybe you could offer an eclipse template for the proper usage? And/or FindBugs 
rules?

Original comment by Maaarti...@gmail.com on 2 Nov 2012 at 4:16

[GoogleCodeExporter]

> But then they'd have to learn both ways, which would lead to more errors. And 
the fact that the one way is needed only rarely does *not* make it better.

Agreed that we'd rather only provide one way if possible.

> Yes, that's bad. Unfortunately, it's allowed even with `protected`. And the 
result of a wrong usage is the worst possible: nothing gets closed. Using other 
names could help, especially renaming `run` to something long and terrible 
(executeWithResources? :D).

I think that passing the callback to a static method is probably safer and 
easier. If you don't call an instance method on it at all, you don't risk 
calling the wrong one.

> Isn't it just a prejudice? That's how StringBuilder, Joiner, Splitter, and 
FluentIterable get used, maybe most of the time.

Possibly. Actually, I have no problem at all with Joiner, etc. (a static 
factory method reads much differently to me for such things than a constructor 
call) and I generally don't mind it with any kind of fluent builder. It's 
mostly just newing something to call a single method on it that I don't like, 
because I feel like anything doing that should be a static method instead. 
Anyway, just my personal preferences.

> Yes, but that's relatively harmless when compared to not closing resources 
(confused `run` and `runAndClose`) or throwing no exception at all (forgotten 
`done`, then an exception in `close` gets swallowed in the assumption there's 
already one pending).

Good points!

> Maybe you could offer an eclipse template for the proper usage? And/or 
FindBugs rules?

Possibly.

Original comment by cgdecker@google.com on 2 Nov 2012 at 3:03

[GoogleCodeExporter]

I think I prefer option 2. The problem with 1 and 3 is that it's so easy to 
make mistakes - and the mistakes are hard to spot when reviewing the code, 
because code that catches IOException in option 1 looks perfectly normal, as 
does code that doesn't call closer.done().

Option 2 is not pretty, but the robustness outweighs that for me.

Original comment by petterma...@gmail.com on 5 Nov 2012 at 9:13

[GoogleCodeExporter]

I just had a look at Guava 14.0-rc1 and I noticed that it contains a class 
com.google.common.io.Closer. Unfortunately it's package private. Any change we 
can have this for Christmas? ;)

(I guess I can wait until Guava 15.0, but of course it'd be nice to have a 
smooth migration away from Closeables#closeQuietly now that said method has 
been deprecated.)

Original comment by stephan...@gmail.com on 15 Dec 2012 at 1:58

[GoogleCodeExporter]

s/change/chance/g

Original comment by stephan...@gmail.com on 15 Dec 2012 at 1:59

[GoogleCodeExporter]

@stephan202: I'd been somewhat hesitant about opening Closer up because of my 
concerns about the potential for accidental misuse and not being sure whether 
the callback version might be preferable. But I've decided it's probably fine 
and that we should get it out, especially considering that 
Closeables.closeQuietly is being deprecated. So, yes... probably. =)

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

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

[GoogleCodeExporter]

Original comment by cgdecker@google.com on 14 Jan 2013 at 11:20

• Changed state: Fixed

[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