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

[liach]

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8302822: Method/Field/Constructor/RecordComponent::getGenericInfo() is not thread safe

Reviewers

• Sergey Tsypanov (@stsypanov - Author) ⚠️ Review applies to f8e05eff
• Claes Redestad (@cl4es - Reviewer) ⚠️ Review applies to f8e05eff

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/12643/head:pull/12643
$ git checkout pull/12643

Update a local copy of the PR:
$ git checkout pull/12643
$ git pull https://git.openjdk.org/jdk.git pull/12643/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12643

View PR using the GUI difftool:
$ git pr show -t 12643

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/12643.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back liach! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@liach The following label will be automatically applied to this pull request:

• core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 01: Full - Incremental (85f2f350)
• 00: Full (f8e05eff)

[openjdk]

@liach This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: stsypanov, redestad

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 688 new commits pushed to the master branch:

• 0dca573: 8301739: AArch64: Add optimized rules for vector compare with immediate for SVE
• 3d3eaed: 8306941: Open source several datatransfer and dnd AWT tests
• 1f57ce0: 8307446: RISC-V: Improve performance of floating point to integer conversion
• 4e4828e: 8307553: Remove dead code MetaspaceClosure::push_method_entry
• 7d58978: 8280031: Deprecate GTK2 for removal
• b5922c3: 8305846: Support compilation in Proc test utility
• 73ac710: 8307425: Socket input stream read burns CPU cycles with back-to-back poll(0) calls
• e2b1013: 8306326: [BACKOUT] 8277573: VmObjectAlloc is not generated by intrinsics methods which allocate objects
• 4386d42: 8307381: Open Source JFrame, JIF related Swing Tests
• 27764e6: 8306583: Add JVM crash check in CDSTestUtils.executeAndLog
• ... and 678 more: https://git.openjdk.org/jdk/compare/065d3e0d58c96b8a84f3c02bb8704fab6459eaa7...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@cl4es) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[jaikiran]

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?

[cl4es]

I think of this pattern of reading a to-be-lazily-initialized value into a local as simple hygiene, volatile or not. The code might seem solid without it - but stranger things than eliding a field load has happened. Storing into the local variable removes some doubt about how this code will be executed.

[dholmes-ora]

The field needs to be volatile for these construction races to be thread-safe, otherwise no guarantee that seeing a non-null genericInfo will mean you see any writes done by the factory methods.

[liach]

We don't fear calling the factory twice for benign races, as the distinct constructor factory instances are behaviorally the same.

The true issue lies in the double getfield operations: Java memory model doesn't require the second read to happen-after a write reflected in the first read, so return this.genericInfo may return null while this.genericInfo == null evaluates to false, in case genericInfo is initialized lazily by another thread. See https://bugs.openjdk.org/browse/JDK-8261404

[plevart]

Hi @liach,

I think @dholmes-ora is worried about the fields in the object being returned by the getGenericInfo() method and similar. In above case this means fields in class ConstructorRepository.
I checked it and the entire hierarchy based on sun.reflect.generics.repository.AbstractRepository with subclasses including ConstructorRepository is modeled such that all fields are either:

• volatile and lazily initialized; or
• final and initialized in constructor

Such objects may be published via data race and still be seen consistent on the accepting side.

[liach]

/integrate

[openjdk]

@liach
Your change (at version f8e05ef) is now ready to be sponsored by a Committer.

[dholmes-ora]

Thanks @plevart that was exactly my concern but I didn't have time to check whether the returned object could be safely published regardless of any race condition. Is it specified that way, or just a fortuitous occurrence?

I would also be concerned about the guarantee of idempotency from the factory method - I hope its requirements in that area are clearly documented.

[AlanBateman]

I would also be concerned about the guarantee of idempotency from the factory method - I hope its requirements in that area are clearly documented.

The spec for the getGenericXXX methods are "Return a" rather than "Return the" so there shouldn't be any expectation on identity. The question about idempotency might be worth checking into as the underlying factory for reflective generic type objects does interact with the defining class loader.

[liach]

I've updated the fields to be volatile.

I would also be concerned about the guarantee of idempotency from the factory method - I hope its requirements in that area are clearly documented.

The spec for the getGenericXXX methods are "Return a" rather than "Return the" so there shouldn't be any expectation on identity. The question about idempotency might be worth checking into as the underlying factory for reflective generic type objects does interact with the defining class loader.

These objects should always be resolving types with the class loader of the declaring class in CoreReflectionsFctory::getDeclsLoader, so the resolved Class instances should be always the same. As far as I see, Type instances are otherwise compared by equals instead of identity, so returning distinct but equal type instances should be safe.

[bridgekeeper]

@liach This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[liach]

keep-alive. Using volatile to ensure correctness of program order is still better than reading null on the second non-volatile read at return.

[liach]

/integrate

[openjdk]

@liach
Your change (at version 85f2f35) is now ready to be sponsored by a Committer.

[cl4es]

I don't have any issue with this version. Making the fields volatile is currently unnecessary to ensure correctness -- thanks @plevart for double-checking that all fields in the hierarchy is either volatile or final -- but adding it is relatively benign (very minor performance cost to a likely-not-very-performance-sensitive code path), reduces fragility and might avoid extraneous allocations of under race conditions.

[cl4es]

/sponsor

[openjdk]

Going to push as commit be36096.
Since your change was applied there have been 1086 commits pushed to the master branch:

• c6f20db: 8308232: nsk/jdb tests don't pass -verbose flag to the debuggee
• d987176: 8307794: Test for HSS/LMS Signature Verification
• 050425b: 8298127: HSS/LMS Signature Verification
• a6109bf: 8308856: jdk.internal.classfile.impl.EntryMap::nextPowerOfTwo math problem
• 6adc242: 8308943: jdk.internal.le build fails on AIX
• 39f6d80: 8307990: jspawnhelper must close its writing side of a pipe before reading from it
• 4460429: 8308803: Improve java/util/UUID/UUIDTest.java
• dfd3da3: 8307683: Loop Predication should not hoist range checks with trap on success projection by negating their condition
• 96ed139: 8308766: TLAB initialization may cause div by zero
• 6c7225f: 8303417: RISC-V: Merge vector instructs with similar match rules
• ... and 1076 more: https://git.openjdk.org/jdk/compare/065d3e0d58c96b8a84f3c02bb8704fab6459eaa7...master

Your commit was automatically rebased without conflicts.

[openjdk]

@cl4es @liach Pushed as commit be36096.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.