[RFC][Icedtea-web]: Close streams after opening them.

[Denis Lila]

Hi Andrew.

What about doing something like this:

        OutputStream s = null;
        try {
            try {
                s = new FileOutputStream(file);
                store(s, header);
            } finally {
                if (s != null) {
                    s.close();
                }
            }
        } catch (Exception ex) {
            ex.printStackTrace();
        }

(and similarly for the InpuStream case).

This way we don't have to have another try catch block
inside the finally (which is unpleasant and weird looking),
we don't have to have more than one printStackTrace call
to handle the 2 IOExceptions, and we take care of any
exceptions thrown in "new FileOutputStream(file);" (a point
brought up by Andrew Haley in another message).

Also, this is closer to what we're trying to do, which is
returning unused resources, and handling exceptional conditions,
so it makes sense to have one try...finally block for the former
and one try...catch for the latter.

Regards,
Denis.

----- Original Message -----
> ----- Original Message -----
> > From: "Omair Majid" <omajid at redhat.com>
> > To: "Andrew Su" <asu at redhat.com>
> > Cc: "Denis Lila" <dlila at redhat.com>, "IcedTea"
> > <distro-pkg-dev at openjdk.java.net>
> > Sent: Monday, March 7, 2011 6:56:40 PM
> > Subject: Re: [RFC][Icedtea-web]: Close streams after opening them.
> > On 03/07/2011 04:40 PM, Andrew Su wrote:
> > >
> > > ----- Original Message -----
> > >> >  From: "Omair Majid"<omajid at redhat.com>
> > >> >  To: "Andrew Su"<asu at redhat.com>
> > >> >  Cc: "Denis Lila"<dlila at redhat.com>,
> > >> >  "IcedTea"<distro-pkg-dev at openjdk.java.net>
> > >> >  Sent: Monday, March 7, 2011 4:30:20 PM
> > >> >  Subject: Re: [RFC][Icedtea-web]: Close streams after opening
> > >> >  them.
> > >> >  On 03/07/2011 04:01 PM, Andrew Su wrote:
> > >>>> >  >>  Subject: Re: [RFC][Icedtea-web]: Close streams after
> > >>>> >  >>  opening them.
> > >>>>> >  >>>  Questions, comments, concerns?
> > >>>> >  >>
> > >>>> >  >>  Maybe it would be better to put the close() in a
> > >>>> >  >>  "finally"
> > >>>> >  >>  block
> > >>>> >  >>  after the catch?
> > >>> >  >
> > >>> >  >  I don't think that'd be necessary, since if an exception
> > >>> >  >  was
> > >>> >  >  thrown.
> > >>> >  >  The stream won't be open.
> > >>> >  >
> > >> >
> > >> >  store(s, header) can throw an IOException too. I agree with
> > >> >  Dennis;
> > >> >  s.close() should be in a finally block. I also see similar
> > >> >  code
> > >> >  in
> > >> >  PropertiesFile.load(): an InputStream is never closed. Can you
> > >> >  please
> > >> >  fix that too?
> > > Haha yeah, I overlooked that. :P
> > > Updated to fix this for load() as well.
> > >
> > > Cheers,
> > >   Andrew
> > >
> > >> >
> > >> >  Cheers,
> > >> >  Omair
> > >
> > >
> > > 20110307_close_streams_v3.patch
> > >
> > >
> > > diff -r 5bc80c9e11c7
> > > netx/net/sourceforge/jnlp/util/PropertiesFile.java
> > > --- a/netx/net/sourceforge/jnlp/util/PropertiesFile.java Mon Mar
> > > 07
> > > 09:50:14 2011 -0500
> > > +++ b/netx/net/sourceforge/jnlp/util/PropertiesFile.java Mon Mar
> > > 07
> > > 16:40:28 2011 -0500
> > > @@ -109,15 +109,21 @@
> > >        */
> > >       public void load() {
> > >           loaded = true;
> > > -
> > > +
> >
> > Please dont include unnecessary whitespace changes.
> 
> *blames IDE*
> 
> >
> > > + InputStream s = null;
> > >           try {
> > >               if (!file.exists())
> > >                   return;
> > >
> > > - InputStream s = new FileInputStream(file);
> > > + s = new FileInputStream(file);
> > >               load(s);
> > >           } catch (IOException ex) {
> > >               // eat
> > > + } finally {
> > > + try {
> > > + s.close();
> > > + } catch (Exception e) {
> > > + }
> >
> > Please only catch IOException. I dont think we should completely
> > swallow
> > exceptions. A printStackTrace would be better. I am quite surprised
> > the
> > original code is fine with eating exceptions too..
> Fixed.
> Here's the updated patch.
> 
> Okay to commit after adding ChangeLog?
> 
> Cheers,
> Andrew
> 
> >
> > >           }
> > >       }
> > >
> > > @@ -127,13 +133,18 @@
> > >       public void store() {
> > >           if (!loaded)
> > >               return; // nothing could have changed so save
> > >               unnecessary load/save
> > > -
> > > +
> > > + OutputStream s = null;
> > >           try {
> > > - OutputStream s = new FileOutputStream(file);
> > > + s = new FileOutputStream(file);
> > >               store(s, header);
> > >           } catch (IOException ex) {
> > >               // eat
> > > + } finally {
> > > + try {
> > > + s.close();
> > > + } catch (Exception e) {
> > > + }
> > >           }
> > >       }
> > > -
> > >   }
> >
> > Same concerns here.
> >
> > Cheers,
> > Omair