[rfc][icedtea-web] RH976833 ClassLoader deadlock

[Andrew Azores]

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