[rfc][icedtea-web] RH976833 ClassLoader deadlock

Jiri Vanek jvanek at redhat.com
Mon Sep 16 07:05:33 PDT 2013

On 09/06/2013 05:57 PM, Andrew Azores wrote:
> On 09/06/13 11:41, Omair Majid wrote:
>> On 09/06/2013 06:08 AM, Jiri Vanek wrote:
>>> General nit - it would be necessary  add javadocs why affected fields
>>> have been made synchronised and why affected methods are no longer
>>> synchronised.
>> Something to watch out for: the more you use low-level synchronization
>> features, the more the chances of unnecessary locking and/or deadlocks.
>> If you can use higher level abstractions for concurrency/locking (eg: a
>> built in data structure or some form of work queue) it will also explain
>> (in code) what's going on _and_ most likely be more correct in terms of
>> locking.
>> Cheers,
>> Omair
> Well I did wrap the relevant member variables in Collections.synchronized{List,Set,Hash,Map,etc.}()
> to provide for atomic operations like add/remove, but even then you still need to synchronize when
> you're going to iterate over a collection. Not very much else is actually happening in this patch,
> if anything. The whole point is that a very very broad-scoped lock (the classloader instance itself)
> was being given to a thread for the sole purpose of ensuring that only one thread entered one
> specific method at a time, which was unnecessary locking and was what lead to issues. I suppose
> simply using a ReentrantLock or something and emulating this behaviour but using the new lock for
> only this one method could have also been done and would have avoided the problem. But to me iit
> seems that that's a bit of a hackish fix for the deadlock, rather than actually addressing the issue
> of not *really* needing to ensure that only one thread is in that method at a time. Rather we just
> need to ensure that all the member variables affected by that method call are used in a thread safe
> manner. Using the classloader instance lock to do this is very heavy handed.
> Thanks,

I think you can post another verison  (maybe final...) of patch

More information about the distro-pkg-dev mailing list