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

[David Holmes]

Hi Remi,

On 30/10/2019 7:38 am, Remi Forax wrote:
> Hi David,
> in java.lang.Thread interrupt0 should be renamed to setInterruptEvent, because what it does now

There is only an "interrupt event" on Windows. What interrupt0 does is 
issue all the necessary unparks so that blocked threads will wake up and 
recheck if they were interrupted. So I don't think a rename is needed.

> and I don't really understand the comment in interrupted(), if a thread is interrupted by two other threads calling interrupt(), you will loose an interrupt anyway.

Suppose a Thread is checking interrupted() ahead of a blocking 
operation, it reads the interrupted field and sees that it is false. 
Then another thread interrupts it, setting the field to true and issuing 
the unparks. The interrupted() call continues and sets the interrupted 
field to false, and then returns false so the blocking operation 
proceeds. Because of the unparks the blocking call returns immediately. 
The thread then checks again to see if it was interrupted, but the field 
is false and so it re-parks (treating this as a spurious wakeup). But 
the thread actually was interrupted and should have thrown 
InterruptedException. The interrupt has been lost.

By only clearing the interrupted field if it was set we avoid this 
problem. This is the logic the VM code has been implementing for 20 
years, so the same logic is needed in the Java code. I added the 
additional comment to the VM code to make it clearer.

Hope that clarifies things.

Thanks,
David
-----

> Rémi
> 
> ----- Mail original -----
>> De: "David Holmes" <david.holmes at oracle.com>
>> À: "hotspot-runtime-dev" <hotspot-runtime-dev at openjdk.java.net>, "hotspot compiler"
>> <hotspot-compiler-dev at openjdk.java.net>, "core-libs-dev" <core-libs-dev at openjdk.java.net>, "Doug Simon"
>> <doug.simon at oracle.com>, "TOM.RODRIGUEZ" <tom.rodriguez at oracle.com>, "serviceability-dev"
>> <serviceability-dev at openjdk.java.net>, "Roger Riggs" <Roger.Riggs at oracle.com>
>> Envoyé: Mardi 29 Octobre 2019 08:42:08
>> Objet: RFR: 8229516: Thread.isInterrupted() always returns false after thread termination
> 
>> 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
>> -----