On Sun, 19 Feb 2023 18:41:18 GMT, liach <d...@openjdk.org> wrote:
> 8302822: Method/Field/Constructor/RecordComponent::getGenericInfo() is not
> thread safe
Hello @liach, I don't follow what this change is achieving. I think I might be
missing something though. I read through the linked JIRA which states:
> In the getGenericInfo() methods of Method, Field, Constructor, and
> RecordComponent, the genericInfo field is read twice, and the second read
> returned may be null under race conditions.
Considering the `Constructor` class as an example, which looks like this:
@Override
ConstructorRepository getGenericInfo() {
// lazily initialize repository if necessary
if (genericInfo == null) {
// create and cache generic info repository
genericInfo =
ConstructorRepository.make(getSignature(),
getFactory());
}
return genericInfo; //return cached repository
}
I can understand that the `ConstructorRepository.make(getSignature(),
getFactory());` might end up getting called more than once in case of race (or
if ConstructorRepository.make(getSignature(), getFactory()) really returns
null), but those should be harmless races. Plus, the getSignature() isn't
expensive, since it returns an already assigned `final` field.
Is there some other race condition here?
The JBS issue also states:
> Class::getGenericInfo() originally had the same issue, but was fixed in
> 8016236.
I had a look at the RFR
https://mail.openjdk.org/pipermail/core-libs-dev/2013-June/017798.html. That's
a slightly different issue, from what I understand. In that case, the call to
getGenericInfo() was being preceded by a call to some other expensive method.
The change there proposed to first call getGenericInfo() and let it be
initialized and only then decide whether to call the other expensive methods.
In that change, I can see that the `getGenericInfo()` method on the `Class`
class was changed too and that change is almost similar to what's being
proposed in this current PR. However, `Class.genericInfo` field is `volatile`
and I think that's why Doug changed that method to first write it to a local
field and then use that local field for the rest of the work. In the current PR
however, which touches `Field`, `Method`, `Constructor` and `RecordComponent`,
the `genericInfo` isn't a `volatile` field in any of those classes, so I don't
see why this local assignment is needed or would help. Am I missing something?
-------------
PR: https://git.openjdk.org/jdk/pull/12643
