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

[Daniel D. Daugherty]

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
> -----