[rfc][icedtea-web] RH976833 ClassLoader deadlock

[Jiri Vanek]

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