RFR: 8289613: Drop use of Thread.stop in jshell [v3]
Rémi Forax
forax at openjdk.org
Tue Sep 6 08:10:48 UTC 2022
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
More information about the core-libs-dev
mailing list