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

[Andrew Su]

----- 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 20110307_close_streams_v4.patch
Type: text/x-patch
Size: 1254 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20110307/af3854e6/20110307_close_streams_v4.patch