[rfc][icedtea-web] RH976833 ClassLoader deadlock

[Omair Majid]

On 10/04/2013 04:27 PM, Andrew Azores wrote:
> On 10/04/2013 03:42 PM, Omair Majid wrote:
>>
>>> As for the rest of your response... well, I'm going to rethink this
>>> whole thing and try to come up with a better, higher-level solution to
>>> this. It is scary dealing with low-level synchronization like this,
>>> especially in somewhere like the JNLPClassLoader. Like I said, it's
>>> possible to simply use a ReentrantLock or other similar construct and
>>> retain the coarse-grained synch control over the
>>> JNLPClassLoader.loadClass method, without resorting to low-level
>>> synchronization on instance variables. In terms of the diagram this
>>> would mean introducing a new lock "r1", and causing the second box on
>>> the chart to cause the acquisition of r1 rather than cl1. As far as I
>>> can tell there's nothing really wrong with enforcing that only one
>>> thread enters that method at a time, the issue is just that the lock
>>> used for this purpose happens to be an inappropriate lock to use in this
>>> particular scenario.
>> I await the patch :)
>>
> 
> This should fix it. It's still a bit heavy-handed IMO, but on the other
> hand it avoids messy synchronization of individual fields.

I think the idea is sensible, but the implementation has a few issues.

> +++ b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java

>      /**
>       * Find a JAR in the shared 'extension' classloaders, this
>       * classloader, or one of the classloaders for the JNLP file's
> -     * extensions.
> +     * extensions. This method should be synchronized so that only
> +     * one thread is executing within it at a time, however this
> +     * classloader's instance lock is NOT appropriate and its use
> +     * can result in deadlock. This ReentrantLock is provided instead.
>       */
> -    public synchronized Class<?> loadClass(String name) throws ClassNotFoundException {
> +    private ReentrantLock loadClassLock = new ReentrantLock();

Please make this field final to ensure all threads see the same value
(and it's never reassigned).

I don't think it's good style to hide members in the middle of a 1000+
line file. Could you move it to the top of the file?

Also, the javadocs need to be associated with the method, not with the
variable (moving the declaration will fix that).

> +        loadClassLock.unlock();
>          return result;

This is problematic: it may never get invoked. ClassLoader.loadClass()
communicates failure to find a class by throwing an exception. So if a
class is found earlier (and the method returns) or not found at all the
execution will not reach this line and unlock() will not be called.

I think it's safer to call unlock in a finally clause:

  lock.lock()
  try {
    // do stuff
    // maybe throw an exception
    // or maybe return early
  } finally {
    lock.unlock();
  }


But then, your concerns with a synchronized block (indention) are back.

Thanks,
Omair
-- 
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681