Re: RFR: 8229516: Thread.isInterrupted() always returns false after thread termination

[David Holmes]

Thanks for the review Dan!

Fixed the nit in thread.c

Thanks,
David

On 30/10/2019 6:51 am, Daniel D. Daugherty wrote:

On 10/29/19 3:42 AM, David Holmes wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8229516
CSR: https://bugs.openjdk.java.net/browse/JDK-8232676 (already approved)
webrev: http://cr.openjdk.java.net/~dholmes/8229516/webrev/

make/hotspot/symbols/symbols-unix
     No comments.

src/hotspot/os/windows/osThread_windows.cpp
     No comments.

src/hotspot/os/windows/osThread_windows.hpp
     No comments.

src/hotspot/share/aot/aotCodeHeap.cpp
     Skipped.

src/hotspot/share/classfile/javaClasses.cpp
     No comments.

src/hotspot/share/classfile/javaClasses.hpp
     No comments.

src/hotspot/share/classfile/vmSymbols.cpp
     No comments.

src/hotspot/share/classfile/vmSymbols.hpp
     No comments.

src/hotspot/share/include/jvm.h
     No comments.

src/hotspot/share/jvmci/jvmciRuntime.cpp
src/hotspot/share/jvmci/jvmciRuntime.hpp
src/hotspot/share/jvmci/vmStructs_jvmci.cpp
     Skipped these three.

src/hotspot/share/oops/oop.hpp
     No comments.

src/hotspot/share/oops/oop.inline.hpp
     No comments.

src/hotspot/share/opto/c2compiler.cpp
     No comments.

src/hotspot/share/opto/library_call.cpp
     No comments.

src/hotspot/share/prims/jvm.cpp
     No comments.

src/hotspot/share/prims/jvmtiEnv.cpp
     No comments.

src/hotspot/share/prims/jvmtiEnvBase.cpp
     No comments.

src/hotspot/share/runtime/osThread.cpp
     No comments.

src/hotspot/share/runtime/osThread.hpp
     No comments.

src/hotspot/share/runtime/thread.cpp
     No comments.

src/hotspot/share/runtime/vmStructs.cpp
     No comments.

src/java.base/share/classes/java/lang/Thread.java
     No comments.

src/java.base/share/native/libjava/Thread.c
     L76:   // Need to reset the interrupt event used by Process.waitFor
     L77:   ResetEvent((HANDLE) JVM_GetThreadInterruptEvent());
         nit - the indent in JDK .c files is four spaces.

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/OSThread.java
     No comments.

src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot.test/src/org/graalvm/compiler/hotspot/test/HotSpotMethodSubstitutionTest.java src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/ThreadSubstitutions.java

     Skipped these 5 graal files.

Thumbs up! I only have one nit above and don't need to see
another webrev if you decide to fix it.

Thanks for being so thorough on your testing.

Dan

This cross-cuts core-libs, hotspot-runtime and the JIT compilers, but only in small pieces each. There is also a small touch to serviceability code.

This change is "simply" moving the "interrupted" field out of the osThread and into the java.lang.Thread so that it can be set independently of whether the thread is alive (and to make things easier for loom in the near future). It is very straightforward:

- old scheme
  - interrupted field is in osThread
  - VM can read/write directly
  - Java code calls into VM to read/write

- new scheme
  - interrupted field is in java.lang.Thread
  - VM has to use javaCalls to read/write "directly"
  - Java code can read/write directly

No changes to any of the semantics regarding the actual interrupt mechanism. Special thanks to Patricio for tracking down a bug I had introduced in that regard!

Special Note (Hi Roger!): on windows we still need to set/clear the _interrupt_event used by the Process.waitFor logic. To facilitate clearing via Thread.interrupted() I had to introduce a native method that is a no-op except on Windows. This seemed the cheapest and least intrusive means to achieve this.

Other changes revolve around the fact we used to have an intrinsic for Thread.isInterrupted and that is not needed any more. So we strip some code out of C1/C2.

The changes in the JVMCI/Graal code are a bit more far reaching as entire classes disappear. I've cc'd Doug and Tom at Vladimir's request so that they can comment on the JVMCI changes and whether I have gone too far or not far enough. There are a bunch of tests for interruption in JVMCI that could potentially be deleted if they are only intended to test the JVMCI handling of interrupt:

./jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.jtt/src/org/graalvm/compiler/jtt/threads/Thread_isInterrupted*.java

Testing:
 - Tiers 1-3 on all Oracle platforms
 - Focused testing on Linux x64:
    - Stress runs of JSR166TestCase
    - Anything that seems to use interrupt():
      - JDK
        - java/lang/Thread
        - java/util/concurrent
        - jdk/internal/loader/InterruptedClassLoad.java
        - javax/management
        - java/nio/file/Files
        - java/nio/channels
        - java/net/Socket/Timeouts.java
        - java/lang/Runtime/shutdown/
        - java/lang/ProcessBuilder/Basic.java
        - com/sun/jdi/
      - Hotspot
        - vmTestbase/nsk/monitoring/
        - vmTestbase/nsk/jdwp
        - vmTestbase/nsk/jdb/
        - vmTestbase/nsk/jdi/
        - vmTestbase/nsk/jvmti/
        - runtime/Thread
        - serviceability/jvmti/
        - serviceability/jdwp
        - serviceability/sa

Thanks,
David
-----