8323782: Race: Thread::interrupt vs. AbstractInterruptibleChannel.begin #17444

reinrich · 2024-01-16T10:57:46Z

Set interrupted in Thread::interrupt before reading nioBlocker for correct (Dekker scheme) synchronization with concurrent execution of AbstractInterruptibleChannel::begin.

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.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8323782: Race: Thread::interrupt vs. AbstractInterruptibleChannel.begin (Bug - P4)

Reviewers

Alan Bateman (@AlanBateman - Reviewer)
David Holmes (@dholmes-ora - Reviewer)

Contributors

Alan Bateman <alanb@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/17444/head:pull/17444
$ git checkout pull/17444

Update a local copy of the PR:
$ git checkout pull/17444
$ git pull https://git.openjdk.org/jdk.git pull/17444/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17444

View PR using the GUI difftool:
$ git pr show -t 17444

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/17444.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-01-16T10:58:39Z

👋 Welcome back rrich! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-01-16T11:00:13Z

@reinrich The following label will be automatically applied to this pull request:

core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

reinrich · 2024-01-16T17:04:37Z

Thanks for the test @AlanBateman
/contributor add @AlanBateman

openjdk · 2024-01-16T17:18:08Z

@reinrich
Contributor Alan Bateman <alanb@openjdk.org> successfully added.

reinrich · 2024-01-16T17:39:59Z

The new test LotsOfInterrupts.java hangs after a few repetitions (using jtreg's REPEAT_COUNT). With the fix it always terminates successfully.

mlbridge · 2024-01-17T09:37:18Z

Webrevs

AlanBateman

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.

AlanBateman · 2024-01-17T15:14:49Z

src/java.base/share/classes/java/lang/Thread.java

+        // Write interrupted before reading nioBlocker for correct synchronization.
+        interrupted = true;
+        interrupt0();  // inform VM of interrupt
+        if (this != Thread.currentThread()) {
            // 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.

AlanBateman · 2024-01-17T15:15:20Z

src/java.base/share/classes/java/lang/Thread.java

@@ -1702,24 +1702,23 @@ public final void stop() {
    public void interrupt() {
        if (this != Thread.currentThread()) {
            checkAccess();
-
+        }
+        // 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.

openjdk · 2024-01-17T15:43:05Z

@reinrich This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8323782: Race: Thread::interrupt vs. AbstractInterruptibleChannel.begin

Co-authored-by: Alan Bateman <alanb@openjdk.org>
Reviewed-by: alanb, dholmes

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 3 new commits pushed to the master branch:

a0e5e16: 8325162: Remove duplicate GCMParameters class
0d51b76: 8325877: Split up NativeCompilation.gmk
2b1a840: 8325860: Serial: Move Generation.java to serial folder

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

reinrich · 2024-01-17T15:44:15Z

Thanks for your help Alan.

RealCLanger · 2024-01-17T21:18:27Z

test/jdk/java/nio/channels/Selector/LotsOfInterrupts.java

@@ -0,0 +1,91 @@
+/*
+ * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.


I guess you should credit SAP here.

Alan wrote the test. I added him as contributor.

dholmes-ora · 2024-01-18T08:02:27Z

src/java.base/share/classes/java/lang/Thread.java

+        interrupted = true;
+        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.

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.

This hasn't changed. The interruptLock is used to coordinate the add/remove of the nioBlocker. When there is no nioBlocker set then the interrupt status and unparking (as in JavaThread::interrupt) has always executed without the interruptLock (named "blockerLock" in the past).

I think that interrupting is just asynchronous to some extent.
E.g. a thread polls its interrupt status clearing it thereby (without lock) before calling nio. A concurrent interrupt can be lost then even if the lock is acquired.
(Maybe clearing should not be done by a public method)

Yep my bad on the VM side of things - no change there. But in the nioBlocker case doesn't this inherently make things more racy? Now maybe those races are allowed, but this might lead to a change in behaviour.

Yep my bad on the VM side of things - no change there. But in the nioBlocker case doesn't this inherently make things more racy? Now maybe those races are allowed, but this might lead to a change in behaviour.

I/O threads always check their interrupt status after installing the nioBlocker. The interrupter and the I/O thread (can be several) may race calling postInterrupt but that is okay.

reinrich · 2024-01-19T07:47:26Z

I noticed that VirtualThread overrides isInterrupted

jdk/src/java.base/share/classes/java/lang/VirtualThread.java

Lines 902 to 905 in 05dad67

    
           @Override 
        
           public boolean isInterrupted() { 
        
               return interrupted; 
        
           }

with just the same implementation as Thread has:

jdk/src/java.base/share/classes/java/lang/Thread.java

Lines 1741 to 1751 in 05dad67

    
               /** 
        
                * Tests whether this thread has been interrupted.  The <i>interrupted 
        
                * status</i> of the thread is unaffected by this method. 
        
                * 
        
                * @return  {@code true} if this thread has been interrupted; 
        
                *          {@code false} otherwise. 
        
                * @see     #interrupted() 
        
                */ 
        
               public boolean isInterrupted() { 
        
                   return interrupted; 
        
               }

This potentially hinders performance. Is there a reason to have this override?

AlanBateman · 2024-01-19T07:57:36Z

I noticed that VirtualThread overrides isInterrupted
Is there a reason to have this override?

It was necessary at one point but no reason to now except to keep it close at the source level with the other methods that access the interrupt status.

reinrich · 2024-01-19T08:17:00Z

I noticed that VirtualThread overrides isInterrupted
Is there a reason to have this override?

It was necessary at one point but no reason to now except to keep it close at the source level with the other methods that access the interrupt status.

Do you want to keep it then or should I open an RFE to remove it?

AlanBateman · 2024-01-21T18:28:18Z

Do you want to keep it then or should I open an RFE to remove it?

Maybe leave it for now as there is significant churn in this code right now, with more accumulating in the loom repo in advance of the monitors work.

…actInterruptibleChannel_begin

dholmes-ora

Sorry for leaving this hanging. Change looks good.

Thanks

…actInterruptibleChannel_begin

reinrich · 2024-02-16T08:39:20Z

Thanks for helping and reviewing.
Richard.
/integrate

openjdk · 2024-02-16T08:40:14Z

Going to push as commit 4018b2b.
Since your change was applied there have been 15 commits pushed to the master branch:

2705ed0: 8325074: ZGC fails assert(index == 0 || is_power_of_2(index)) failed: Incorrect load shift: 11
3d85103: 8316813: NMT: Using WhiteBox API, virtual memory tracking should also be stressed in JMH tests
ba8db1f: 8325876: crashes in docker container tests on Linuxppc64le Power8 machines
18cea82: 8319801: Recursive lightweight locking: aarch64 implementation
9029bf6: 8316451: 6 java/lang/instrument/PremainClass tests ignore VM flags
99c9ae1: 8323664: java/awt/font/JNICheck/FreeTypeScalerJNICheck.java still fails with JNI warning on some Windows configurations
0fdfdf7: 8325983: Build failure after JDK-8324580
3b1062d: 8322239: [macos] a11y : java.lang.NullPointerException is thrown when focus is moved on the JTabbedPane
5a988a5: 8322750: Test "api/java_awt/interactive/SystemTrayTests.html" failed because A blue ball icon is added outside of the system tray
a231706: 8324580: SIGFPE on THP initialization on kernels < 4.10
... and 5 more: https://git.openjdk.org/jdk/compare/b718ae35a87e5696cd6d26952ab1f7d3fda27691...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-02-16T08:40:21Z

@reinrich Pushed as commit 4018b2b.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8323782: Race: Thread::interrupt vs. AbstractInterruptibleChannel.begin

7a7286c

openjdk bot added the core-libs core-libs-dev@openjdk.org label Jan 16, 2024

reinrich added 2 commits January 16, 2024 15:26

Must checkAccess before changing interrupt state of other thread

d0c66fb

Add Alan's LotsOfInterrupts.java test

dae6d21

New version of LotsOfInterrupts.java supporting virtual threads

2c1d383

reinrich marked this pull request as ready for review January 17, 2024 09:01

openjdk bot added the rfr Pull request is ready for review label Jan 17, 2024

AlanBateman reviewed Jan 17, 2024

View reviewed changes

Review Alan

ab3513c

AlanBateman approved these changes Jan 17, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 17, 2024

RealCLanger reviewed Jan 17, 2024

View reviewed changes

dholmes-ora reviewed Jan 18, 2024

View reviewed changes

Merge branch 'master' into 8323782__Race__Thread__interrupt_vs__Abstr…

81a9f81

…actInterruptibleChannel_begin

dholmes-ora approved these changes Feb 14, 2024

View reviewed changes

Merge branch 'master' into 8323782__Race__Thread__interrupt_vs__Abstr…

d9a71c2

…actInterruptibleChannel_begin

openjdk bot added the integrated Pull request has been integrated label Feb 16, 2024

openjdk bot closed this Feb 16, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 16, 2024

reinrich deleted the 8323782__Race__Thread__interrupt_vs__AbstractInterruptibleChannel_begin branch February 19, 2024 09:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8323782: Race: Thread::interrupt vs. AbstractInterruptibleChannel.begin #17444

8323782: Race: Thread::interrupt vs. AbstractInterruptibleChannel.begin #17444

reinrich commented Jan 16, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 16, 2024

openjdk bot commented Jan 16, 2024

reinrich commented Jan 16, 2024

openjdk bot commented Jan 16, 2024

reinrich commented Jan 16, 2024

mlbridge bot commented Jan 17, 2024 •

edited

Loading

AlanBateman left a comment

AlanBateman Jan 17, 2024

AlanBateman Jan 17, 2024

openjdk bot commented Jan 17, 2024 •

edited

Loading

reinrich commented Jan 17, 2024

RealCLanger Jan 17, 2024

reinrich Jan 18, 2024

dholmes-ora Jan 18, 2024

AlanBateman Jan 18, 2024 •

edited

Loading

reinrich Jan 18, 2024

dholmes-ora Jan 19, 2024

AlanBateman Jan 19, 2024 •

edited

Loading

reinrich commented Jan 19, 2024

AlanBateman commented Jan 19, 2024

reinrich commented Jan 19, 2024

AlanBateman commented Jan 21, 2024

dholmes-ora left a comment

reinrich commented Feb 16, 2024

openjdk bot commented Feb 16, 2024

openjdk bot commented Feb 16, 2024

		@@ -0,0 +1,91 @@
		/*
		* Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.

8323782: Race: Thread::interrupt vs. AbstractInterruptibleChannel.begin #17444

8323782: Race: Thread::interrupt vs. AbstractInterruptibleChannel.begin #17444

Conversation

reinrich commented Jan 16, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Contributors

Reviewing

Webrev

bridgekeeper bot commented Jan 16, 2024

openjdk bot commented Jan 16, 2024

reinrich commented Jan 16, 2024

openjdk bot commented Jan 16, 2024

reinrich commented Jan 16, 2024

mlbridge bot commented Jan 17, 2024 • edited Loading

Webrevs

AlanBateman left a comment

Choose a reason for hiding this comment

AlanBateman Jan 17, 2024

Choose a reason for hiding this comment

AlanBateman Jan 17, 2024

Choose a reason for hiding this comment

openjdk bot commented Jan 17, 2024 • edited Loading

reinrich commented Jan 17, 2024

RealCLanger Jan 17, 2024

Choose a reason for hiding this comment

reinrich Jan 18, 2024

Choose a reason for hiding this comment

dholmes-ora Jan 18, 2024

Choose a reason for hiding this comment

AlanBateman Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

reinrich Jan 18, 2024

Choose a reason for hiding this comment

dholmes-ora Jan 19, 2024

Choose a reason for hiding this comment

AlanBateman Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

reinrich commented Jan 19, 2024

AlanBateman commented Jan 19, 2024

reinrich commented Jan 19, 2024

AlanBateman commented Jan 21, 2024

dholmes-ora left a comment

Choose a reason for hiding this comment

reinrich commented Feb 16, 2024

openjdk bot commented Feb 16, 2024

openjdk bot commented Feb 16, 2024

reinrich commented Jan 16, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 17, 2024 •

edited

Loading

openjdk bot commented Jan 17, 2024 •

edited

Loading

AlanBateman Jan 18, 2024 •

edited

Loading

AlanBateman Jan 19, 2024 •

edited

Loading