commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Damjan Jovanovic <damjan....@gmail.com>
Subject Re: [imaging] Closing stream
Date Thu, 24 Oct 2013 18:46:31 GMT
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 <garydgregory@gmail.com> wrote:
> On Wed, Oct 23, 2013 at 9:38 PM, sebb <sebbaz@gmail.com> wrote:
>
>> On 24 October 2013 01:16, Gary Gregory <garydgregory@gmail.com> 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: garydgregory@gmail.com | ggregory@apache.org
>> > 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: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>>
>
>
> --
> E-Mail: garydgregory@gmail.com | ggregory@apache.org
> 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: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Mime
View raw message