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

[Omair Majid]

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.

> +        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..

>           }
>       }
>
> @@ -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