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

[Omair Majid]

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.

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

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

Cheers,
Omair
-------------- next part --------------
A non-text attachment was scrubbed...
Name: IntegerSynchronization.java
Type: text/x-java
Size: 423 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20120126/43ace04d/IntegerSynchronization.java