As one of the perpetrators of the problem, I have now fixed it. The
reasons I swallowed exceptions were simple:
* when multiple resources need to be closed, earlier exceptions in
finally cause later close() methods to get skipped and those resources
to leak
* when an exception is thrown and close() then throws another
exception, the close() exception is propagated and the original
exception - which could reveal the cause of the problem - swallowed
Anyway we now propagate close() exceptions instead of logging and/or
catching, which has shrunk the jar by about 0.5%. The non-leaking with
multiple resources has been preserved but is much uglier:
} finally {
IOException closeException = null;
if (srcChannel != null) {
try {
srcChannel.close();
} catch (final IOException ioException) {
closeException = ioException;
}
}
if (dstChannel != null) {
try {
dstChannel.close();
} catch (final IOException ioException) {
closeException = ioException;
}
}
if (closeException != null) {
throw closeException;
}
}
and original exception propagation benefit will now be lost, but only
Java 7's try-with-resources takes care of all those problems cleanly
anyway (eg. building up a list of all suppressed exceptions inside the
original exception), so you can't have it all.
Damjan
On Thu, Oct 24, 2013 at 3:49 AM, Gary Gregory <[email protected]> wrote:
> On Wed, Oct 23, 2013 at 9:38 PM, sebb <[email protected]> wrote:
>
>> On 24 October 2013 01:16, Gary Gregory <[email protected]> wrote:
>> > Hi All:
>> >
>> > I see a log of this pattern:
>> >
>> > try {
>> > if (outputStream != null) {
>> > outputStream.close();
>> > }
>> > } catch (final Exception e) {
>> > Debug.debug(e);
>> > }
>> >
>> > for example in org.apache.commons.imaging.Imaging:
>> >
>> > public static void writeImage(final BufferedImage src, final File
>> file,
>> > final ImageFormat format, final Map<String,Object> params)
>> > throws ImageWriteException,
>> > IOException {
>> > OutputStream os = null;
>> >
>> > try {
>> > os = new FileOutputStream(file);
>> > os = new BufferedOutputStream(os);
>> >
>> > writeImage(src, os, format, params);
>> > } finally {
>> > try {
>> > if (os != null) {
>> > os.close();
>> > }
>> > } catch (final Exception e) {
>> > Debug.debug(e);
>> > }
>> > }
>> > }
>> >
>> > Which seems wrong to me. If I get an IOE while writing, we throw, but if
>> we
>> > get one when flushing and closing the file (which may also write), we
>> > swallow it. Why? It seems the IOE from close should percolate up and not
>> be
>> > swallowed.
>>
>> I agree that's bad practice - not only because of swallowing the Exception.
>> The catch block should catch IOException not Exception.
>>
>
> I do not think we should even catch the IOE, it should percolate up, just
> like the writeImage call. Why not simply:
>
> {
> final OutputStream os = new BufferedOutputStream(new
> FileOutputStream(file));
> try {
> writeImage(src, os, format, params);
> } finally {
> os.close();
> }
> }
>
> Or if your are really paranoid:
>
> {
> OutputStream os = new FileOutputStream(file);
> try {
> os = new BufferedOutputStream(os);
> writeImage(src, os, format, params);
> } finally {
> os.close();
> }
> }
>
> I'm looking for a pattern we can apply throughout [imaging] before 1.0.
>
> Gary
>
>
>> > All of this is moot in Java 7 with try-with-resources blocks but we are
>> not
>> > ready for Java 7 here I imagine.
>> >
>> > Gary
>> > --
>> > E-Mail: [email protected] | [email protected]
>> > Java Persistence with Hibernate, Second Edition<
>> http://www.manning.com/bauer3/>
>> > JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
>> > Spring Batch in Action <http://www.manning.com/templier/>
>> > Blog: http://garygregory.wordpress.com
>> > Home: http://garygregory.com/
>> > Tweet! http://twitter.com/GaryGregory
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [email protected]
>> For additional commands, e-mail: [email protected]
>>
>>
>
>
> --
> E-Mail: [email protected] | [email protected]
> Java Persistence with Hibernate, Second
> Edition<http://www.manning.com/bauer3/>
> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
> Spring Batch in Action <http://www.manning.com/templier/>
> Blog: http://garygregory.wordpress.com
> Home: http://garygregory.com/
> Tweet! http://twitter.com/GaryGregory
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
