[rfc][icedtea-web] Ensure JarFile handles do not leak, introduce closeSilently utility

[Jiri Vanek]

On 02/13/2013 05:43 PM, Adam Domurad wrote:
> On 02/12/2013 04:55 PM, Adam Domurad wrote:
>> This squelches some Eclipse warnings.
>>
>>> 2013-XX-XX  Adam Domurad <adomurad at redhat.com>
>>>
>>>     Ensure JarFile handles do not leak.
>>>     * netx/net/sourceforge/jnlp/util/StreamUtils.java:
>>>     New, introduces 'closeSilently'. Ignores nulls & exceptions when
>>>     closing.
>>>     * netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java:
>>>     Ensure StreamUtils.closeSilently or close is called for each
>>>     JarFile.
>
> OK that patch had a bug in it, and was probably too paranoid. This ones better.
>
> Turns out JNLPClassLoader already had a closeStreams utility function.
>
> 2013-XX-XX  Adam Domurad <adomurad at redhat.com>
>
>      Ensure JarFile handles do not leak.
>      * netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java:
>      Ensure close is called for each JarFile.
>
>

Idea is good and I'm ok with it after two things are fixed

1) can we have an reproducer/test for this?
> closeJarFiles2.patch
>
>
> diff --git a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
> --- a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
> +++ b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
> @@ -812,6 +812,8 @@ public class JNLPClassLoader extends URL
>                           break;
>                       }
>                   }
> +
> +                jarFile.close();
>               } catch (IOException e) {
>                   /*
>                    * After this exception is caught, it is escaped. This will skip
> @@ -834,12 +836,15 @@ public class JNLPClassLoader extends URL
>           File f = tracker.getCacheFile(location);
>
>           if( f != null) {
> +            JarFile mainJar = null;
>               try {
> -                JarFile mainJar = new JarFile(f);
> +                mainJar = new JarFile(f);
>                   mainClass = mainJar.getManifest().
>                           getMainAttributes().getValue("Main-Class");
>               } catch (IOException ioe) {
>                   mainClass = null;
> +            } finally {
> +                closeStream(mainJar);

2)This have not compiled in head. The method do not exists....
If this is doing something similar to StreamUtils.closeSilently then I'm ok with it. But have to compile...I not found it ;(

Also  StreamUtils.closeSilently(mainJar);
  have not compiled... :(
>               }
>           }
>
> @@ -1269,6 +1274,7 @@ public class JNLPClassLoader extends URL
>                                   jarEntries.add(je.getName());
>                               }
>
> +                            jarFile.close();
>                           }
>
>                           addURL(jar.getLocation());
> @@ -1292,6 +1298,8 @@ public class JNLPClassLoader extends URL
>                               JarIndex index = JarIndex.getJarIndex(jarFile, null);
>                               if (index != null)
>                                   jarIndexes.add(index);
> +
> +                            jarFile.close();
>                           } else {
>                               CachedJarFileCallback.getInstance().addMapping(jar.getLocation(), jar.getLocation());
>                           }
> @@ -1365,6 +1373,7 @@ public class JNLPClassLoader extends URL
>                                        new FileOutputStream(outFile));
>
>               }
> +            jarFile.close();

Just thinking about - why not to finally block too? To verbose? To rare?
/me do not insists, just thinking
>           } catch (IOException ex) {
>               if (JNLPRuntime.isDebug())
>                   ex.printStackTrace();

Thanx for check!
J.