Make Files.newReader() etc. more defensive against possible exceptions

[GoogleCodeExporter]

This StackOverflow question - http://stackoverflow.com/q/12552863 - 
demonstrates a potential danger when working with Closeable objects; namely 
that if Closeables are chained, and one of the constructors in the chain raises 
an exception, any already-constructed objects will not be explicitly closed, 
even in a try-with-resources block.  The methods in Guava's Files class, and 
possibly elsewhere, are susceptible to this edge case.

For example, the following code:

try(BufferedReader br = Files.newReader(f, null)) {
  // will never enter, as InputStreamReader throws an NPE
}

Will raise an exception, therefore not setting br to a BufferedReader.  As 
such, the inner FileInputReader, which did successfully get constructed, is 
lost.

A full test case is attached, demonstrating the FIR is not closed, and a 
snippet of the test output is below:

PASSED: closeInnerObject(UTF-8)
FAILED: closeInnerObject(null)
Wanted but not invoked:
fileInputStream.close();
-> at ChainedCloseableTest.closeInnerObject(ChainedCloseableTest.java:45)
Actually, there were zero interactions with this mock.

Original issue reported on code.google.com by dimo414 on 27 Nov 2013 at 12:13

Attachments:

• ChainedCloseableTest.java

[GoogleCodeExporter]

Files.newReader contains a null check, so I think the problem is restricted to 
more exotic cases. (I see that your test goes to some lengths to reproduce the 
problem on a Files.newReader-like method, so I gather that you've come to a 
similar conclusion.) I still see some sense in fixing this problem in case of 
weird problems like OutOfMemoryError (though maybe any recovery then is 
ill-fated), but does it come up with more mundane causes? If not, we'll have to 
consider how easy it would be to fix the problem in comparison to the benefits 
it brings.

Original comment by cpov...@google.com on 27 Nov 2013 at 12:21

[GoogleCodeExporter]

Ah, I see it does now contain a null check, the version on my machine (13.0.1) 
does not, and that's what I copied.  The null check therefore does address that 
specific example, however chaining Closeables fundamentally exposes this 
problem - Errors are a possibility, but it seems like a generally poor plan to 
depend upon Closeables not raising Exceptions in their constructors.

Suggested fix would be along the lines of the pre-try-with-resources behavior 
(simplified example):

InputStreamReader isr = null;
FileInputStream fis = null;
try {
  fis = new FileInputStream(file);
  isr = new InputStreamReader(fis);
  return isr;
} catch (Throwable t) {
  if(isr != null) isr.close();
  if(fis != null) fis.close();
  throw t;
}

Happy to put together a patch along these lines if interested.

Original comment by dimo414 on 27 Nov 2013 at 12:47

[GoogleCodeExporter]

Other than NPE if the Charset is null, the constructors for InputStreamReader 
and BufferedReader don't throw exceptions. Well, other than if the Charset 
implementation throws an exception from newDecoder(), but, really? And trying 
to defend against OOME everywhere seems like a losing battle to me.

The only thing I'd consider doing to be more defensive for Files.newReader is 
calling charset.newDecoder() before opening the InputStream. Note that JDK7's 
java.nio.file.Files.newBufferedReader isn't any more defensive than this. (We 
would probably want to set the malformed input/unmappable character actions to 
REPLACE, which is how the method currently behaves but differs from the JDK7 
method.)

Original comment by cgdecker@google.com on 27 Nov 2013 at 1:55

• Changed title: Make Files.newReader() etc. more defensive against possible exceptions
• Changed state: Research
• Added labels: Type-Enhancement

[GoogleCodeExporter]

This issue has been migrated to GitHub.

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

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

• Added labels: MigratedToGitHub

[GoogleCodeExporter]

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

• Changed state: Migrated

[GoogleCodeExporter]

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

• Added labels: Restrict-AddIssueComment-Commit