Code Review for 7001933: Deadlock in java.lang.classloader.getPackage()

[Valerie (Yu-Ching) Peng]

David,

Thanks for the comments. I've updated the webrev accordingly at:
http://cr.openjdk.java.net/~valeriep/7001933/webrev.01/

In the case of a race condition, we'll just return the earlier defined 
package object, i.e. pkg2 in your code sample.
Or, we could also get rid of this code block altogether, then there 
shouldn't be a race condition. However, this means that we'll  call into 
the parent loader for the packages that they defined which implies some 
performance cost.

For the time being, I'll hold this off for a little longer, so people 
have more time to give comments.

Thanks,
Valerie

On 02/24/11 05:50 PM, David Holmes wrote:
> HI Valerie,
>
> Valerie (Yu-Ching) Peng said the following on 02/25/11 11:36:
>> Can someone please review for 7001933: Deadlock in 
>> java.lang.classloader.getPackage()?
>>
>> http://cr.openjdk.java.net/~valeriep/7001933/webrev.00/
>>
>> The fixes are straightforward - release the lock when delegating to 
>> the parent.
>> The change has been verified by the submitter.
>
> 1642                 synchronized (packages) {
> 1643                     if (packages.get(name) != null) {
> 1644                         packages.put(name, pkg);
> 1645                     }
> 1646                 }
>
> This looks wrong to me. Shouldn't you only do the put if the get 
> returns null? And if the get doesn't return null shouldn't the method 
> return that result ie:
>
>                 synchronized (packages) {
>                     Package pkg2 = null;
>                     if ((pkg2 = packages.get(name)) == null) {
>                         packages.put(name, pkg);
>                     }
>                     else
>                         pkg = pkg2;
>                 }
>                 ...
>                 return pkg;
>
> In general it is easy to break a deadlock by making an open-call, as 
> you have done, but the harder question to answer is: what possible 
> side-effects could there be due to races now that this method is not 
> atomic?
>
> Cheers,
> David