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

[Jiri Vanek]

On 04/22/2013 11:05 PM, Adam Domurad wrote:
> On 03/26/2013 05: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?
>
> Reproducer is more or less impossible. Unit tests attached (first unit-tests for JNLPClassLoader!!).
> Note that they are heavily unix-specific.
>
> One note about reproducers: They may seem to mostly succeed, but as long as there is one failure this is difficult to say. Once the file leaks in JNLPClassLoader the rest use the same file descriptor, so there is no further leak. I have verified that each test is able to catch the resulting leak, if not for the caching.
>
> ChangeLog:
> 2013-XX-XX Adam Domurad <adomurad at redhat.com>
>
> * tests/netx/unit/net/sourceforge/jnlp/runtime/JNLPClassLoaderTest.java:
> New, JNLPClassLoader unit tests for (checkForMain), (getMainClassName),
> (activateNativeJar), and (isInvalidJar). Checks for file descriptor
> leaks.
> * netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
> (isInvalidJar): Change to default visibility for testing purposes.
> (checkForMain): Same.
> (getMainClassName): Same.
>
>>> 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);
>
> This indeed is what needs to be done and really should compile fine.
> I will fix it (ie change this one line to this) before pushing.
>
>> 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?
>
> Too rare; not worth the resulting inelegance in my opinion.
>
>> /me do not insists, just thinking
>>> } catch (IOException ex) {
>>> if (JNLPRuntime.isDebug())
>>> ex.printStackTrace();
>>
>> Thanx for check!
>> J.
>
> Happy hacking,
> -Adam

I must say I have a lot to learn from this testcase!
Thank you for both patch and test.
Acked.