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

[Omair Majid]

On 03/07/2011 07:04 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 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
>
>
> 20110307_close_streams_v4.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 19:05:06 2011 -0500
> @@ -110,14 +110,20 @@
>       public void load() {
>           loaded = true;
>
> +        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 (IOException e) {
> +            }
>           }
>       }
>

I was hoping to see some exception.printStackTrace()s used here instead 
of disappearing exceptions.

> @@ -128,11 +134,17 @@
>           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 (IOException e) {
> +            }
>           }
>       }
>

And likewise.

Omair