8289613: Drop use of Thread.stop in jshell

[asotona]

LocalExecutionControl in jdk.jshell actually uses Thread::stop to cancel execution of (long-running or infinite loops) user code in JShell, however Thread::stop is deprecated and planned for removal.

Proposed patch instruments all user code to call LocalExecutionControl::stopCheck method before every branch instruction.
Thread::stop call is replaced by setting global field LocalExecutionControl.allStop to true and stopCheck method then throws ThreadDead when called from the instrumented code.

Proposed patch requires jdk.jshell access to java.base jdk.internal.org.objectweb.asm package.

Please review.

Thanks,
Adam

---

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-8289613: Drop use of Thread.stop in jshell

Reviewers

• Jan Lahoda (@lahodaj - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10166

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

Using diff file

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

[bridgekeeper]

👋 Welcome back asotona! 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]

@asotona The following labels will be automatically applied to this pull request:

• core-libs
• kulla

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

[mlbridge]

Webrevs

• 03: Full - Incremental (c6821bcb)
• 02: Full - Incremental (30c34b6a)
• 01: Full - Incremental (0f0e0dd1)
• 00: Full (355ba7f7)

[AlanBateman]

This is the last usage of Thread.stop in the JDK so this change is welcome.

The instrumentation to check a flag at jump instrumentation looks reasonable, and works here because the method is public on a public class in an exported package. I hadn't noticed that jdk.jshell.execution is exported and the addition to LocalExecutionControl means it's a new API so I assume forces you to write javadoc.

If a user presses control-C in jshell does it also interrupt the thread or will it just set the stop flag? If it also interrupts then you have another choice to check the interrupt status (assumes all code is well behaved of course).

Is there one or many LocalExecutionControl objects when I execute a snippet, control-C, execute another? I'm wondering because the allStop flag is static but STOP_LOCK is per instance.

[forax]

I've checked the code, same question as Alan, i believe you can have multiple instances of LocalExecutionControl, but allStop is a global flag.

Usually, the way to get an instance at runtime is either to use a ClassValue on the generated class or to store the instance in a ClassLoader and several one class loaders.
The last option is to pass an instance when defining a class, it only works with hidden classes, not normal classes but that code can be changed in the JDK.

[lahodaj]

I believe that one instance of JShell will use only once instance of ExecutionControl at one time (although if it fails, it may spawn a new one). But there may be multiple instances of JShell at one time, so I suspect the static field on LocalExecutionControl won't work. I apologize for not realizing this sooner.

I think one way to resolve this is, for every LocalExecutionControl instance, synthesize a class, possibly named REPL.$Cancel$, which would hold the allStop field and the stopCheck method. This would then be load using the LocalExecutionControl's LoaderDelegate, and used by the snippet.

A positive side-effect of this would be that we wouldn't need a new method in the public API, which I think would be preferred.

[forax]

yes, this is a good plan.

[asotona]

I think @AlanBateman proposal to monitor interrupted status is easy to implement and also does not require a new public method nor synthetic class.

[forax]

@asotona, the problem is that user code may also react to the interrupted status,
the cancelling has be a mechanism a user can not trigger directly.

[asotona]

Right, let's try the synthetic class.

[AlanBateman]

LocalExecutionControl.stop invoking Thread::interrupt is good as that will cause any interruptible methods to wakeup.

I'm less sure about instrumenting every "if" and "goto" to invoke Thread::interrupted but more recent comment suggests are going back to a stop field that polled by code in a generated class so I'll wait until you are done.

[asotona]

The latest version combines all comments. It instruments user code, delegates "allStop" to generated REPL.$Cancel$ class (loaded by specific LoaderDelegate), triggers REPL.$Cancel$.allStop and also interrupts all related threads to wake them up.

[forax]

Otherwise, it looks good to me

[AlanBateman]

Would it be possible to summarize how the loader delegate work in jshell? Instrumenting the code to invoke REPL/$Cancel$.stopCheck looks reasonable but I can't immediately see how the generated REPL/$Cancel$ has access to LocalExecutionControl.allStop.

[lahodaj]

Seems reasonable to me.

[lahodaj]

Would it be possible to summarize how the loader delegate work in jshell? Instrumenting the code to invoke REPL/$Cancel$.stopCheck looks reasonable but I can't immediately see how the generated REPL/$Cancel$ has access to LocalExecutionControl.allStop.

Not sure what you mean - as far as I can tell, neither the generated class, nor the snippets access LocalExecutionControl.allStop. The snippets only access the generated class, and LocalExecutionControl sets the cancelled value into the generated class. And there is no problem accessing the generated class, as it is in the unnamed module and public.

[AlanBateman]

Not sure what you mean - as far as I can tell, neither the generated class, nor the snippets access LocalExecutionControl.allStop.

Okay, I see it now. REPL.$Cancel$.allStop is public so there is no issue accessing it from LEC.invoke.

[openjdk]

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

8289613: Drop use of Thread.stop in jshell

Reviewed-by: jlahoda

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 52 new commits pushed to the master branch:

• 8d3399b: 8292758: put support for UNSIGNED5 format into its own header file
• 6677227: 8293497: Build failure due to MaxVectorSize was not declared when C2 is disabled after JDK-8293254
• 986b834: 8293489: Accept CAs with BasicConstraints without pathLenConstraint
• fc5f97f: 8293474: RISC-V: Unify the way of moving function pointer
• 2d13f53: 8293512: ProblemList serviceability/tmtools/jstat/GcNewTest.java in -Xcomp mode
• f84386c: 8293182: Improve testing of CDS archive heap
• 51de765: 8283010: serviceability/sa/ClhsdbThread.java failed with "'Base of Stack:' missing from stdout/stderr "
• 8a48965: 8293514: ProblemList gc/metaspace/TestMetaspacePerfCounters.java#Epsilon-64 on all platforms
• 1e031e6: 8293232: Fix race condition in pkcs11 SessionManager
• 1080c4e: 8293508: ProblemList gc/metaspace/TestMetaspacePerfCounters.java#Epsilon-64
• ... and 42 more: https://git.openjdk.org/jdk/compare/48b3ab02f9339f97e49f775c9d5d3a598a158abf...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.

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

[asotona]

/integrate

[openjdk]

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

• 8d3399b: 8292758: put support for UNSIGNED5 format into its own header file
• 6677227: 8293497: Build failure due to MaxVectorSize was not declared when C2 is disabled after JDK-8293254
• 986b834: 8293489: Accept CAs with BasicConstraints without pathLenConstraint
• fc5f97f: 8293474: RISC-V: Unify the way of moving function pointer
• 2d13f53: 8293512: ProblemList serviceability/tmtools/jstat/GcNewTest.java in -Xcomp mode
• f84386c: 8293182: Improve testing of CDS archive heap
• 51de765: 8283010: serviceability/sa/ClhsdbThread.java failed with "'Base of Stack:' missing from stdout/stderr "
• 8a48965: 8293514: ProblemList gc/metaspace/TestMetaspacePerfCounters.java#Epsilon-64 on all platforms
• 1e031e6: 8293232: Fix race condition in pkcs11 SessionManager
• 1080c4e: 8293508: ProblemList gc/metaspace/TestMetaspacePerfCounters.java#Epsilon-64
• ... and 42 more: https://git.openjdk.org/jdk/compare/48b3ab02f9339f97e49f775c9d5d3a598a158abf...master

Your commit was automatically rebased without conflicts.

[openjdk]

@asotona Pushed as commit ffc249a.

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