RFR: 8289613: Drop use of Thread.stop in jshell [v3]

[Rémi Forax]

On Mon, 5 Sep 2022 19:23:30 GMT, Adam Sotona <asotona at openjdk.org> wrote:

>> 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
>
> Adam Sotona has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - allStop field and stopCheck method moved to synthetic REPL.$Cancel$
>  - Revert "alternative implementation where instrumented code is directly checking Thread::interrupted"
>    
>    This reverts commit 0f0e0dd17c121955e7806073bb8cc78da1f133ea.

Otherwise, it looks good to me

src/jdk.jshell/share/classes/jdk/jshell/execution/LocalExecutionControl.java line 98:

> 96:         var cancelWriter = new ClassWriter(ClassWriter.COMPUTE_FRAMES|ClassWriter.COMPUTE_MAXS);
> 97:         cancelWriter.visit(Opcodes.V19, Opcodes.ACC_PUBLIC, "REPL/$Cancel$", null, "java/lang/Object", null);
> 98:         cancelWriter.visitField(Opcodes.ACC_PUBLIC|Opcodes.ACC_STATIC|Opcodes.ACC_VOLATILE, "allStop", "Z", null, null);

fomatting: operators like '|' usually have a space before and a space after

src/jdk.jshell/share/classes/jdk/jshell/execution/LocalExecutionControl.java line 119:

> 117:     protected String invoke(Method doitMethod) throws Exception {
> 118:         if (allStop == null) {
> 119:             super.load(new ClassBytecodes[]{genCancelClass()});

fomatting: usually, people use spaces like this

  new ClassBytecodes[] {  genCancelClass()  }

src/jdk.jshell/share/classes/jdk/jshell/execution/LocalExecutionControl.java line 200:

> 198:                 allStop.set(null, true);
> 199:             } catch (IllegalArgumentException| IllegalAccessException ex) {
> 200:                 throw new InternalException(ex.getMessage());

to avoid to drop the stack trace of the original exception, it's better to chain the exceptions,

  throw (InternalException) new InternalException().initCause(ex);

-------------

PR: https://git.openjdk.org/jdk/pull/10166