[rfc][icedtea-web] RH976833 ClassLoader deadlock

[Andrew Azores]

On 10/04/2013 04:51 PM, Omair Majid wrote:
> 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

Sorry, that was more meant as a code demonstration of the "cl1/r1" 
replacement idea, not as push-ready code. My wording about it, and the 
rest of the email, definitely made it sound otherwise. I still think 
there's some discussion to be had about the safety of this before a 
candidate implementation is put forward.

Even though I think it seems safe to do this replacement (other than the 
implementation issues you've correctly pointed out), I'm not 100% sure. 
I don't see anything else that depends on the cl1 lock other than 
methods JNLPClassLoader is inheriting from URLClassLoader, and this 
replacement really shouldn't have any effect on URLClassLoader. But 
maybe I'm missing something and there is a good reason for our 
overriding loadClass method to be requiring the cl1 lock in particular. 
I don't have any idea what that might actually be, but it's still 
something I'm concerned about. If that is the case then the solution to 
this deadlock probably won't be quite as trivial as the cl1/r1 switch, 
so I don't feel it's quite worth making a push-ready patch implementing 
that change until it's deemed both safe and effective at the conceptual 
level.

If none of us are able to identify any issues with doing this simple 
lock replacement trick then I'll make another push-candidate patch later.

You're right though, the proper alternative to a synchronized() block is 
a try/finally. Might as well just use an Object and synchronized block 
for that for if/when that patch gets made.

Thanks,

-- 
Andrew A