On Tue, 16 Jan 2024 10:57:46 GMT, Richard Reingruber <rr...@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. Thanks for finding this issue. It duplicates on JDK 8, and looking at the ancient history, it looks like it's been there since JDK 1.4 but just not noticed or diagnosed. Reading the nioBlocker after setting the interrupt status is good. There are about 20 tests for async interrupt of I/O ops in tier2. I see you've run tier1-4 so you've run them. src/java.base/share/classes/java/lang/Thread.java line 1706: > 1704: checkAccess(); > 1705: } > 1706: // Write interrupted before reading nioBlocker for correct > synchronization. I think I'd prefer if this said that it sets the interrupt status, and must be done before reading the nioBlocker. src/java.base/share/classes/java/lang/Thread.java line 1710: > 1708: interrupt0(); // inform VM of interrupt > 1709: if (this != Thread.currentThread()) { > 1710: // thread may be blocked in an I/O operation I think the existing comment "thread may be blocked in an I/O operation" can move to before the `if` statement so that it's a bit clearer than this is code for I/O operations. ------------- PR Review: https://git.openjdk.org/jdk/pull/17444#pullrequestreview-1827537664 PR Review Comment: https://git.openjdk.org/jdk/pull/17444#discussion_r1455826929 PR Review Comment: https://git.openjdk.org/jdk/pull/17444#discussion_r1455825649
