commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gary Gregory <garydgreg...@gmail.com>
Subject Re: [imaging] Closing stream
Date Thu, 24 Oct 2013 01:49:10 GMT
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

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message