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