http://gwt-code-reviews.appspot.com/1880803/diff/7001/dev/core/src/com/google/gwt/util/tools/Utility.java File dev/core/src/com/google/gwt/util/tools/Utility.java (right):
http://gwt-code-reviews.appspot.com/1880803/diff/7001/dev/core/src/com/google/gwt/util/tools/Utility.java#newcode54 dev/core/src/com/google/gwt/util/tools/Utility.java:54: } catch (IOException e) { On 2013/01/04 10:05:28, tbroyer wrote:
On 2013/01/04 10:00:17, jens wrote: > On 2013/01/04 02:26:08, tbroyer wrote: > > On 2013/01/04 01:31:40, jens wrote: > > > How about logging this exception as it is an I/O error and could
indicate
> > > failing hardware? > > > Personally I think completely ignoring this exception just
because you can
> not > > > recover from it is bad practice. > > > > While I agree on principle, I think this is best left for another
CL.
> > Guava has a nice Closeables utility [1] that we might want to use,
but that
> > means replacing all uses of Utility.close(Closeable) with > > Closeables.close(Closeable,boolean) and adding a boolean in these try/finally > to > > track whether to swallow the exception. > > > > [1] > > >
http://docs.guava-libraries.googlecode.com/git-history/release/javadoc/com/google/common/io/Closeables.html#close%252528java.io.Closeable,
> > boolean) > > I agree that introducing Closeables.close(.., ..) is better done in
another
CL. > > > But this method is touched now and replaces all other
Utility.close() methods
> thus making it a good place to actually react to this IOException in
the mean
> time. Imagine GWT uses Utility.close(BufferedOutputStream)
somewhere: calling
> BufferedOutputStream.close() will result in flushing remaining
buffers and if
> that fails you probably have written out something broken/incomplete
without
> detecting it. > > In fact that is the case for FilterOutputStream and all its sub
classes:
>
http://docs.oracle.com/javase/6/docs/api/java/io/FilterOutputStream.html#close%2528)
So you're proposing to rather add a close(ImageInputStream) method
than factor
all those close() methods into a single close(Closeable) method?
No, using a single method makes totally sense. I would log the IOException as warning. For swallowed exceptions logging will happen anyways if we switch to Guava's Closeables.close() method (as stated in Guava's JavaDoc). We can have logging now and will continue to have it when using Guava. P.S.: The FilterOutputStream was a bad example as it seems like that the implementation of FilterOutputStream.close() ignores any exception from the internal call to flush(). But the key point is that an implementation of Closeable.close() can do more than just releasing native resources. http://gwt-code-reviews.appspot.com/1880803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
