[rfc][icedtea-web] RH976833 ClassLoader deadlock
Andrew Azores
aazores at redhat.com
Fri Sep 6 08:57:33 PDT 2013
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,
--
Andrew A
More information about the distro-pkg-dev
mailing list