Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8251397: NPE on ClassValue.ClassValueMap.cacheArray #94

Closed
wants to merge 1 commit into from

Conversation

@galderz
Copy link
Contributor

@galderz galderz commented Sep 9, 2020

  • Release fence guarantees that cacheArray field will published with a non-null value.
  • Without this fix, CacheValueMap.cacheArray can sometimes be seen as null.

This is a follow up to @PaulSandoz's feedback here for the first attempt to fix JDK-8251397.

In this update, the fence has been moved to initializeMap() and added Paul's suggested comment.

Annotating classValueMap with @Stable is outside the scope of this issue.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8251397: NPE on ClassValue.ClassValueMap.cacheArray

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/94/head:pull/94
$ git checkout pull/94

@bridgekeeper bridgekeeper bot added the oca label Sep 9, 2020
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 9, 2020

Hi @galderz, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user galderz" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

@openjdk
Copy link

@openjdk openjdk bot commented Sep 9, 2020

@galderz 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 (add|remove) "label" command.

@openjdk openjdk bot added the core-libs label Sep 9, 2020
Copy link
Contributor

@shipilev shipilev left a comment

I believe current patch is incorrect.

// published safely in the non-volatile field Class.classValueMap,
// since stores to the fields of ClassValueMap will not be reordered
// to occur after the store to the field type.classValueMap
UNSAFE.storeFence();

This comment has been minimized.

@shipilev

shipilev Sep 9, 2020
Contributor

Wait a second, how's that supposed to work? The storeFence should be between the end of new ClassValueMap() and the store to classValueMap.

This comment has been minimized.

@galderz

galderz Sep 9, 2020
Author Contributor

Ah yes, my bad 🤦‍♂️

This comment has been minimized.

@galderz

galderz Sep 9, 2020
Author Contributor

Updated PR.

This comment has been minimized.

@shipilev

shipilev Sep 9, 2020
Contributor

Now I wonder (after reading the discussion), why not UNSAFE.storeStoreFence()? I am basically okay with either, given they currently map to the same thing. But it seems that semantically only storeStoreFence is really needed here.

This comment has been minimized.

@shipilev

shipilev Sep 9, 2020
Contributor

Scratch that. Hotspot does MemBar_Release for final stores, low-level Java code is better do the same:
https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/parse1.cpp#L1005

It deserves a separate follow-up to homogenize the uses of Unsafe.storeStoreFence and Unsafe.storeFence in other places in JDK.

This comment has been minimized.

@Sanne

Sanne Sep 16, 2020

@galderz did you really update the PR? I still see the storeFence before the write.

This comment has been minimized.

@Sanne

Sanne Sep 17, 2020

ah, sorry I got it now. Please ignore my previous comment.

@galderz galderz force-pushed the galderz:t_classvalue_npe_v2_8251397 branch from 40dd793 to c4baeb8 Sep 9, 2020
@galderz
Copy link
Contributor Author

@galderz galderz commented Sep 9, 2020

/covered

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 9, 2020

Thank you! Please allow for a few business days to verify that your employer has signed the OCA. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

type.classValueMap = map = new ClassValueMap();
if ((map = type.classValueMap) == null) {
map = new ClassValueMap();
// Place a Store fence as the last operation to emulate

This comment has been minimized.

@PaulSandoz

PaulSandoz Sep 9, 2020
Member

Since the store was moved i suggest the comment be slightly updated:
"Place a Store fence after construction and before publishing to emulate"

This comment has been minimized.

@galderz

galderz Sep 9, 2020
Author Contributor

Done.

@galderz galderz force-pushed the galderz:t_classvalue_npe_v2_8251397 branch from c4baeb8 to aec3b25 Sep 9, 2020
@galderz galderz force-pushed the galderz:t_classvalue_npe_v2_8251397 branch from aec3b25 to 48ff6dd Sep 10, 2020
@jerboaa
Copy link
Contributor

@jerboaa jerboaa commented Sep 10, 2020

/sponsor

@openjdk
Copy link

@openjdk openjdk bot commented Sep 10, 2020

@jerboaa The change author (@galderz) must issue an integrate command before the integration can be sponsored.

@jerboaa
Copy link
Contributor

@jerboaa jerboaa commented Sep 10, 2020

Just noting here that @galderz is a Red Hat employee and should be covered by the Red Hat OCA:
https://www.oracle.com/technical-resources/oracle-contributor-agreement.html#r

@galderz
Copy link
Contributor Author

@galderz galderz commented Sep 10, 2020

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Sep 10, 2020

@galderz Your merge request cannot be fulfilled at this time, as the status check jcheck did not complete successfully

@jerboaa
Copy link
Contributor

@jerboaa jerboaa commented Sep 10, 2020

@galderz We'll have to wait until somebody marks your github handle as OCA approved or some such :-/

@openjdk
Copy link

@openjdk openjdk bot commented Sep 12, 2020

@galderz This change now passes all automated pre-integration checks. In addition to the automated checks, the change must also fulfill all project specific requirements

After integration, the commit message will be:

8251397: NPE on ClassValue.ClassValueMap.cacheArray

Add release fence to ClassValueMap constructor.

* Release fence guarantees that cacheArray field will published
with a non-null value.
* Without this fix, CacheValueMap.cacheArray can sometimes be
seen as null.

Reviewed-by: shade, psandoz
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /issue command.

Since the source branch of this PR was last updated there have been 89 commits pushed to the master branch:

  • cca3a26: 8252996: Thread safety problem in java.net.ProxySelector
  • a4c6a99: 8252593: [TESTBUG] serviceability/jvmti/GetLocalVariable/GetLocalWithoutSuspendTest.java failed with JVMTI_ERROR_INVALID_SLOT
  • a67f890: 8253050: jfr disassemble command processes --max-chunks incorrectly
  • f972155: 8252196: ZGC: TestUncommit.java fails due to "Exception: Uncommitted too fast" again(2)
  • a9993f9: 8253275: Remove unused methods after CMS removal
  • 4ac6934: 8253232: G1Analytics::compute_pause_time_ratios() uses wrong pause times in calculation
  • 53a4ef2: 8202473: A type variable with multiple bounds does not correctly place type annotation
  • b87a159: 8252100: NumberOverflow in class MemoryCache
  • 9a7dcdc: 8253261: Disable CDS full module graph until JDK-8253081 is fixed
  • 1c84cfa: 8253130: bug7072653.java failed "Popup window height ... is wrong"
  • ... and 79 more: https://git.openjdk.java.net/jdk/compare/c655b703a9f66dad54c8edb0bac332ac2decc820...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate cca3a26e43963435b245779650df7b27c66afdc3.

As you do not have Committer status in this projectan existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@shipilev, @PaulSandoz) 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).

@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 12, 2020

Webrevs

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Sep 14, 2020

Please ensure the PR title and the bug synopsis match before this in integrated!

Please DO NOT force-push updates as you lose the history of commits and anyone following this cannot see what changes were actually made at each step! You do not need to worry about intermediate commits as they will only appear in your branch and in the PR. The actual committed changeset will be a single flattened version of your changes.

@galderz
Copy link
Contributor Author

@galderz galderz commented Sep 14, 2020

Are you expecting anything else from me? @jerboaa already proposed as sponsor above.

@jerboaa
Copy link
Contributor

@jerboaa jerboaa commented Sep 14, 2020

@galderz The PR has this synopsis 8251397: Add release fence to ClassValueMap constructor. The bug has this 8251397: NPE on ClassValue.ClassValueMap.cacheArray. @dholmes-ora asked to make them matching. Let's fix that first.

@jerboaa
Copy link
Contributor

@jerboaa jerboaa commented Sep 14, 2020

You can add what you have as synopsis now via /summary and the bot.

@galderz
Copy link
Contributor Author

@galderz galderz commented Sep 14, 2020

/summary 8251397: NPE on ClassValue.ClassValueMap.cacheArray

@openjdk
Copy link

@openjdk openjdk bot commented Sep 14, 2020

@galderz Setting summary to 8251397: NPE on ClassValue.ClassValueMap.cacheArray

@jerboaa
Copy link
Contributor

@jerboaa jerboaa commented Sep 14, 2020

@galderz Now it's the other way round. Commit message the bot mentions here should be something like this:

8251397: NPE on ClassValue.ClassValueMap.cacheArray

Add release fence to ClassValueMap constructor.

* Release fence guarantees that cacheArray field will published
  with a non-null value.
* Without this fix, CacheValueMap.cacheArray can sometimes be
   seen as null.

Reviewed-by: shade, psandoz

The first line can be achieved by force-pushing with the changed commit message. I know David said to not force push, but the review is done and no other way to achieve this except changing the bug synopsis which isn't great either. The latter lines can be added with /summary e.g.

/summary
Add release fence to ClassValueMap constructor.

* Release fence guarantees that cacheArray field will published
  with a non-null value.
* Without this fix, CacheValueMap.cacheArray can sometimes be
   seen as null.

The Reviewed-by: line comes from the reviewers.

@openjdk
Copy link

@openjdk openjdk bot commented Sep 14, 2020

@jerboaa Only the author (@galderz) is allowed to issue the /summary command.

@jerboaa
Copy link
Contributor

@jerboaa jerboaa commented Sep 14, 2020

Note that I've opened SKARA-633 for pre-population of the commit message with an appropriate summary.

Add release fence to ClassValueMap constructor.

* Release fence guarantees that
cacheArray field will published with a non-null value.
* Without this fix,
CacheValueMap.cacheArray can sometimes be seen as null.
@galderz galderz force-pushed the galderz:t_classvalue_npe_v2_8251397 branch from 48ff6dd to 6c4e82d Sep 14, 2020
@galderz
Copy link
Contributor Author

@galderz galderz commented Sep 14, 2020

/summary
Add release fence to ClassValueMap constructor.

  • Release fence guarantees that cacheArray field will published
    with a non-null value.
  • Without this fix, CacheValueMap.cacheArray can sometimes be
    seen as null.
@openjdk
Copy link

@openjdk openjdk bot commented Sep 14, 2020

@galderz Updating existing summary to:

Add release fence to ClassValueMap constructor.

* Release fence guarantees that cacheArray field will published
with a non-null value.
* Without this fix, CacheValueMap.cacheArray can sometimes be
seen as null.
@jerboaa
Copy link
Contributor

@jerboaa jerboaa commented Sep 14, 2020

@galderz Hmm, sorry. Apparently it's the PR title which makes up the synopsis. Probably has to be changed in the UI here. I didn't know that :(

@galderz galderz changed the title 8251397: Add release fence to ClassValueMap constructor 8251397: NPE on ClassValue.ClassValueMap.cacheArray Sep 14, 2020
@galderz
Copy link
Contributor Author

@galderz galderz commented Sep 14, 2020

PR title changed...

@jerboaa
Copy link
Contributor

@jerboaa jerboaa commented Sep 14, 2020

@galderz OK. Lets try the integrate/sponsor workflow. AFAIK it's integrate from you first and sponsor for me second.

@jerboaa
Copy link
Contributor

@jerboaa jerboaa commented Sep 14, 2020

/test builds tier1

@openjdk
Copy link

@openjdk openjdk bot commented Sep 14, 2020

@jerboaa the test group builds tier1 does not exist

@jerboaa
Copy link
Contributor

@jerboaa jerboaa commented Sep 14, 2020

/test tier1

@openjdk
Copy link

@openjdk openjdk bot commented Sep 14, 2020

@jerboaa you need to get approval to run the tests in tier1 for commits up until 6c4e82d

@openjdk openjdk bot added the test-request label Sep 14, 2020
@galderz
Copy link
Contributor Author

@galderz galderz commented Sep 17, 2020

/integrate

@openjdk openjdk bot added the sponsor label Sep 17, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Sep 17, 2020

@galderz
Your change (at version 6c4e82d) is now ready to be sponsored by a Committer.

@jerboaa
Copy link
Contributor

@jerboaa jerboaa commented Sep 17, 2020

/sponsor

@openjdk
Copy link

@openjdk openjdk bot commented Sep 17, 2020

@jerboaa @galderz Since your change was applied there have been 89 commits pushed to the master branch:

  • cca3a26: 8252996: Thread safety problem in java.net.ProxySelector
  • a4c6a99: 8252593: [TESTBUG] serviceability/jvmti/GetLocalVariable/GetLocalWithoutSuspendTest.java failed with JVMTI_ERROR_INVALID_SLOT
  • a67f890: 8253050: jfr disassemble command processes --max-chunks incorrectly
  • f972155: 8252196: ZGC: TestUncommit.java fails due to "Exception: Uncommitted too fast" again(2)
  • a9993f9: 8253275: Remove unused methods after CMS removal
  • 4ac6934: 8253232: G1Analytics::compute_pause_time_ratios() uses wrong pause times in calculation
  • 53a4ef2: 8202473: A type variable with multiple bounds does not correctly place type annotation
  • b87a159: 8252100: NumberOverflow in class MemoryCache
  • 9a7dcdc: 8253261: Disable CDS full module graph until JDK-8253081 is fixed
  • 1c84cfa: 8253130: bug7072653.java failed "Popup window height ... is wrong"
  • ... and 79 more: https://git.openjdk.java.net/jdk/compare/c655b703a9f66dad54c8edb0bac332ac2decc820...master

Your commit was automatically rebased without conflicts.

Pushed as commit 81e2cf8.

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

dankm pushed a commit to dankm/openjdk that referenced this pull request Oct 27, 2020
This seems to be relatively unique to *BSD and causes problems with at
least Jetty 9.x.

Please see Issue openjdk#94 for details.

This doesn't need to be ported to other openjdk versions.  Newer versions
have changed the way socket channel closure works to ignore an IOException
on close.  This could potentially be removed if that is ever backported to
openjdk11

With this change in place no change in Tier 1 internal tests were observed.

Discussed with:	@bsdkurt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants