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

[David Holmes]

On 30/10/2019 4:05 am, serguei.spitsyn at oracle.com wrote:
> Hi David,
> 
> The fix looks good to me.

Thanks Serguei!

David

> I did not pay much attention to the Graal related changes though.
> The test coverage for Serviceability is complete.
> Running java/lang/instrument tests is not necessary.
> 
> Thanks,
> Serguei
> 
> 
> On 10/29/19 00:42, 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/
>>
>> 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
>> -----
>