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

[Jiri Vanek]

ping?

On 03/26/2013 10:26 AM, Jiri Vanek wrote:
> 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.