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

8253063: ScopedAccessError is sometimes thrown spuriously #323

Closed

Conversation

@mcimadamore
Copy link
Collaborator

@mcimadamore mcimadamore commented Sep 11, 2020

After running the test for shared segment handshake during unrelated work, I noted that the test exhibit two kind of failure modes:

  1. First, the test can sometimes fail with a raw ScopedAccessError
  2. More rarely, the test sometimes even crash (this only happens in the vectorized mismatch test)

After some investigation carried out by @fisk, turns out that (2) was caused by the fact that when we exit from deoptimization we don't check for pending async exception (while we check for regulart ones). This condition was not triggered in other tests because, it seems, plain unsafe accesses are effectively atomic once intrinisfied - e.g. there can be no safepoint between check and access. But the vectorized mismatch routine is a biggie Java routine which gets some initial compilation (by C1) - and this is when the bug hit.

The problem (2) was more difficult to pinpoint - basically, there are some routines in the VM which are hostile w.r.t. async exceptions - and are marked with the special JRT_ENTRY_NO_ASYNC entry. One of those routines happens to be the one that throws exceptions, and this routine can safepoint too. Now, since the scope memory access can also throw (if the scope is not alive) it is possible to end up in a situation where we try to install an async exception while the VM code was already in the middle of installing one. We thought we covered this case by checking for pending exceptions - but the check we had was too weak. After trying with different approaches we agreed to address this more explicitly - by having the exception code set a flag on the running thread - so that we can precisely determine, when we do the handshake, as to whether the async exception should be installed or not.

With these fixes, the test passes and no more spurious failures can be observed (I left it running for a long time ;-)).
I've beefed up the test by always having it use max number of available processors, and also by lowering the delay time associated with the closing thread. The longer the wait, the less frequently transient issues such as (1) can be observed.


Progress

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

Issue

  • JDK-8253063: ScopedAccessError is sometimes thrown spuriously

Reviewers

Download

$ git fetch https://git.openjdk.java.net/panama-foreign pull/323/head:pull/323
$ git checkout pull/323

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 11, 2020

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

@openjdk openjdk bot added the rfr label Sep 11, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 11, 2020

Webrevs

@fisk
fisk approved these changes Sep 11, 2020
Copy link
Contributor

@fisk fisk left a comment

This looks good. Sorry I didn't catch this earlier.

@mcimadamore
Copy link
Collaborator Author

@mcimadamore mcimadamore commented Sep 14, 2020

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Sep 14, 2020

@mcimadamore Your merge request cannot be fulfilled at this time, as your changes failed the final jcheck:

  • Too few reviewers with at least role committer found (have 0, need at least 1)
Copy link
Member

@PaulSandoz PaulSandoz left a comment

ScopedAccessException -> ScopedAccessError looks good.

The HotSpot changes look ok to me, although i lack understanding of async exceptions.

@openjdk
Copy link

@openjdk openjdk bot commented Sep 14, 2020

@mcimadamore 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:

8253063: ScopedAccessError is sometimes thrown spuriously

Reviewed-by: eosterlund, 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 62 commits pushed to the foreign-memaccess branch:

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 foreign-memaccess into your branch, and then specify the current head hash when integrating, like this: /integrate 16f0b444414fb03e21bee0bf2fec1663f6f0cde6.

➡️ To integrate this PR with the above commit message to the foreign-memaccess branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Sep 14, 2020
@mcimadamore
Copy link
Collaborator Author

@mcimadamore mcimadamore commented Sep 14, 2020

/integrate

@openjdk openjdk bot closed this Sep 14, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Sep 14, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Sep 14, 2020

@mcimadamore Since your change was applied there have been 62 commits pushed to the foreign-memaccess branch:

Your commit was automatically rebased without conflicts.

Pushed as commit 1cc7a78.

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

@mcimadamore mcimadamore deleted the mcimadamore:scopeaccess-error branch Nov 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants