[icedtea-web] RFC: Fix for PR852: Classloader not being flushed after close

[Omair Majid]

On 01/27/2012 03:29 PM, Deepak Bhole wrote:
> * Omair Majid<omajid at redhat.com>  [2012-01-26 21:55]:
>> Hi Deepak,
>>
>> On 01/26/2012 01:18 PM, Deepak Bhole wrote:
>>> This patch fixes PR852. I would like to put it in 1.2 in addition to
>>> HEAD.
>>
>> I have a few on the patch comments below.
>>
>>> diff -r 41f03d932cdf netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
>>> --- a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java	Mon Jan 09 18:45:31 2012 -0500
>>> +++ b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java	Thu Jan 26 13:15:59 2012 -0500
>>
>>> +    public static void incrementLoaderCount(JNLPClassLoader loader) {
>>
>>> +    public static void decrementLoaderCount(JNLPClassLoader loader) {
>>
>> Just a thought, but it might be cleaner to make these instance
>> methods, since they always operate on a loader object. With that
>> changes, this becomes:
>>
>> public void incrementLoaderCount()
>> public void decrementLoaderCount()
>>
>> In fact, incrementLoaderCount does not even have to be public. If we
>> do go down that route, perhaps we can also go ahead and rename
>> decrementLoaderCount to dispose() or release()? (I am also thinking
>> close() but that would clash with the URLClassLoader's close()
>> method in OpenJDK7.) I think it makes the purpose clearer in that
>> this method releases resources associated with the classloader. It
>> also hides the fact that we are caching something from the users of
>> the JNLPClassLaoder api.
>>
>
> Sure. I think making it non-static should be perfectly fine.
>

What about private? ;)

> As for renaming decrement... to dispose, the loader is no always
> disposed after the call. If multiple applets are opened, we only want
> the count to be reduced, not the loader disposed, so I think the
> existing name is more appropriate.
>

I was thinking in terms of how reference counting works - when you are 
done with an object, you unref()/release() it and the object may or may 
not be actually disposed at that point. But looking at how shared 
objects like FileDescriptor in java are used, your naming makes more sense.

>>> +        synchronized(loader.useCount) {
>>> +            loader.useCount--;
>>> +
>>> +            if (loader.useCount<= 0) {
>>> +                synchronized(urlToLoader) {
>>> +                    urlToLoader.remove(loader.file.getUniqueKey());
>>> +                }
>>> +            }
>>> +        }
>>> +    }
>>>
>>
>> Synchronizing over Integer (or Long) has always looked scary to me.
>> I am not sure if this is a problem in this case, but as soon as you
>> do the post increment, the object (useCount) has changed. If another
>> thread comes along at this point, it can now synchronize over the
>> new object and it would be possible to have two threads enter the
>> same synchronized block. I am attaching a program that shows that
>> the Integer has indeed changed.
>>
>
> Doh! I hadn't even thought of that. You're right, it is unsafe to use
> that. I will fix it to synchronize around the loader itself. It looks
> like it should be safe, but please let me know if you prefer a lock
> specific for the counter.
>

I am fine with synchronized methods.

>>> diff -r 41f03d932cdf plugin/icedteanp/java/sun/applet/PluginAppletViewer.java
>>> --- a/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java	Mon Jan 09 18:45:31 2012 -0500
>>> +++ b/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java	Thu Jan 26 13:15:59 2012 -0500
>>> @@ -1625,6 +1625,9 @@
>>>
>>>                   appletShutdown(p);
>>>                   appletPanels.removeElement(p);
>>> +
>>> +                // Mark classloader unusable
>>> +                JNLPClassLoader.decrementLoaderCount((JNLPClassLoader) cl);
>>>
>>>                   try {
>>>                       SwingUtilities.invokeAndWait(new Runnable() {
>>>
>>
>> Not being an expert, I am not sure about the details of how this
>> fixes the problem. If the _only_ problem was that we are maintaining
>> a reference to the classloader, perhaps making urlToLoader a
>> Map<String, WeakReference<JNLPClassLoader>>  might be more
>> appropriate? Or is there something I am missing?
>>
>> Sorry if this is not very helpful - I took a look at bugzilla but I
>> could not find the actual issue described there.
>>
>
> Please see attached test case I created for the issue.
>

Ah. Okay, so we need to use a new classlaoder for a new instance of an 
applet once all the older instances are gone. Makes much more sense now.

> New patch with changes attached.
>
> Thanks for reviewing this!

Looks good to me.

Cheers,
Omair