RFR: 8302822: Method/Field/Constructor/RecordComponent::getGenericInfo() is not thread safe

[Jaikiran Pai]

On Sun, 19 Feb 2023 18:41:18 GMT, liach <duke at 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