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.hol...@oracle.com>
À: "hotspot-runtime-dev" <hotspot-runtime-...@openjdk.java.net>, "hotspot
compiler"
<hotspot-compiler-...@openjdk.java.net>, "core-libs-dev"
<core-libs-dev@openjdk.java.net>, "Doug Simon"
<doug.si...@oracle.com>, "TOM.RODRIGUEZ" <tom.rodrig...@oracle.com>,
"serviceability-dev"
<serviceability-...@openjdk.java.net>, "Roger Riggs" <roger.ri...@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
-----
