[rfc][icedtea-web] RH976833 ClassLoader deadlock

Omair Majid omajid at redhat.com
Fri Sep 27 09:42:25 PDT 2013


On 09/16/2013 01:31 PM, Andrew Azores wrote:
> On 09/16/2013 11:17 AM, Jiri Vanek wrote:
>> On 09/16/2013 05:09 PM, Andrew Azores wrote:
>>> On 09/16/2013 10:05 AM, Jiri Vanek wrote:
>>>> (snip)
>>>>
>>>> I think you can post another verison  (maybe final...) of patch
>>>>
>>>>
>>>
>>> Attached. It's the same as last time but with some comments added.
>>>
>>> Thanks,
>>>
>>
>>
>> Ugh. I would say even more javadoc needed. You should include similar
>> statements in javadoc which you are suing to persuade Omair and Me
>> that your synchronisation is correct.
>>
>> The only reason for this is that sometime in future one may
>> accidentaly revoke your patch in favour of more simple (as before)
>> synchronisation. And so easily return back to RH976833
>>
>> Sorry for troubeling
>> J.
> 
> Is this enough?

I am a little afraid of patches that fix concurrency, because it's easy
to miss subtle issues (both when writing the fix and when reviewing it).

I also tend to prefer coarse-grained synchronization over fine grained;
I think it's easier to get things right using higher level primitives
than low level primitives like 'synchronized'.

Perhaps it would be nice if the javadocs on the variables were to state
which lock should be used to guard each object? There's lots of repeated
comments like 'Synchronized because this field ....'. Maybe it would be
better to make a class-level javadoc comment like 'This class is used by
multiple threads and must be thread safe'?

I am not a fan of 'see method X and Y' comments if it's just a pointer
to methods that are using the variable because:
- thanks to refactoring, the comments can become stale
- a search will be more effective

On the other hand, if there are any subtle issues, then the 'see X and
Y' comment might be very nice to have.

Also, can you make the variables 'final' to ensure that the object is
always the same?

> 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
> @@ -165,26 +165,41 @@
>      /** Permissions granted by the user during runtime. */
>      private ArrayList<Permission> runtimePermissions = new ArrayList<Permission>();

Doesn't this need to be thread safe too?

> +     *          Locking on the JNLPClassLoader instance when this method is called
> +     * can result in deadlock if another thread is dealing with the CodeBaseClassLoader
> +     * at the same time.

Actually, I don't follow this. Two different threads will enter the
JNLPClassLoader.loadClass() method, one through CodeBaseClassLoader and
one through JNLPClassLoader. Shouldn't one thread block and another
continue? What causes the deadlock?


> +                         This solution is very heavy-handed as the instance lock is not
> +     * truly required, and taking the lock on the classloader instance when not needed is
> +     * not in general a good idea because it can and will lead to deadlock when multithreaded
> +     * classloading is in effect. This problem could also be avoided by using a construct
> +     * like a ReentrantLock to restrict one thread to calling this method at a time,
> +     * so that the affected fields are still not shared data, but the calling thread also
> +     * does not take the instance lock.

As far as I know, synchronized is more-or-less a ReentrantLock. Leaving
aside the monitor semantics, it is re-entrant (unlike posix mutexes).
The 'lock' associated with a synchronized block is held at a thread
level and allows the same thread to acquire it more than once.

>       *                                  However, this is more of a hack and work-around
> +     * than simply ensuring that the fields are kept thread safe on their own. This is
> +     * accomplished by wrapping them in Collections.synchronized{List,Set,etc.} to provide
> +     * atomic add/remove operations, and synchronizing on them when iterating or performing
> +     * multiple mutations.
> +     *
> +     * See bug report RH976833. On some systems the bug will manifest itself as deadlock on
> +     * every webpage with more than one Java applet, potentially also causing the browser
> +     * process to hang.
> +     * More information in the mailing list archives:
> +     * http://mail.openjdk.java.net/pipermail/distro-pkg-dev/2013-September/024536.html

If you know the condition that triggers a deadlock, can you write a unit
test to simulate it?

A quick-and-dirty approach would be to spawn off two threads, use a
Latch to hold off on actually running code in both of them, then release
the latch to let them both run and invoke the code path that causes them
to deadlock.

But then again, this is one of those cases where I feel more comfortable
with a good design (and JNLPClassLoader is the opposite of that) rather
than lots of unit tests.

> +     * Affected fields: available, classpaths, jarIndexes, jarEntries, jarLocationSecurityMap

What about other fields in the class. Are they thread-safe?

>      private void addNewJar(final JARDesc desc, UpdatePolicy updatePolicy) {

There is a dependency between 'available' in this method and 'available'
in addNextResource. If I am reading it correctly, it's possible for the
same jar to be downloaded more than once.

> +        // not synchronized on since this field is wrapped in Collections.synchronizedList
> +        // and so provides atomic add/remove already. Adding an element to a list that
> +        // already contains the element does nothing. So synchronization is not needed.

I am a little confused what you mean here. Adding the same element to a
list twice will result in two of those elements being in the list.

>          available.add(desc);

Cheers,
Omair

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


More information about the distro-pkg-dev mailing list