RFR: 8323782: Race: Thread::interrupt vs. AbstractInterruptibleChannel.begin [v2]

[David Holmes]

On Wed, 17 Jan 2024 15:38:22 GMT, Richard Reingruber <rrich at openjdk.org> wrote:

>> Set `interrupted` in `Thread::interrupt` before reading `nioBlocker` for correct (Dekker scheme) synchronization with concurrent execution of [`AbstractInterruptibleChannel::begin`](https://github.com/openjdk/jdk/blob/59062402b9c5ed5612a13c1c40eb22cf1b97c41a/src/java.base/share/classes/java/nio/channels/spi/AbstractInterruptibleChannel.java#L176).
>> 
>> The change passed our CI functional testing: JTReg tests: tier1-4 of hotspot and jdk. All of Langtools and jaxp. SPECjvm2008, SPECjbb2015, Renaissance Suite, and SAP specific tests.
>> Testing was done with fastdebug and release builds on the main platforms and also on Linux/PPC64le and AIX.
>
> Richard Reingruber has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Review Alan

src/java.base/share/classes/java/lang/Thread.java line 1709:

> 1707:         // Setting the interrupt status must be done before reading nioBlocker.
> 1708:         interrupted = true;
> 1709:         interrupt0();  // inform VM of interrupt

It is really safe/correct to move this outside the synchronized block? I know things have changed a bit with loom but we've "always" held a lock when doing the actual interrupt. I'd have to check the VM logic to be sure it can be called concurrently from multiple threads for the same target thread.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/17444#discussion_r1457061745