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
