avoid extra park of vthread when using fixed thread pool

[miao-zheng]

The test case of ParkWithFixedThreadPool.java create 300 virtual threads, each vthread(exclude latest vthread) park itself and unpark previous vthread. The expected result is any vthread can finish.

Running this test case in slowdebug and the test will random hang.

There are three vthreads which are vt-1, vt-2, vt-3;
(1) vt-1 take the ReentrantLock, and vt-2 try to unpark vt-1, and vt-2 fail to get ReentrantLock so it park itself( the call stack is shown below);
(2)vt-3 unpark vt-2, and vt-2 try to get ReentrantLock again, but the ReentrantLock is still owned by vt-1, vt-2 park itself again;
(3)vt-1 release the ReentrantLock and unpark vt-2, vt-2 park itself at ParkWithFixedThreadPool.java:55, and it will never unpark.(because the unpark from vt-3 has consumed)

The reason is scheduler.execute() will try to alloc a Reentrant lock when using fix thread pool, the call stack is like:
at java.lang.VirtualThread.tryPark(VirtualThread.java:472)
at java.lang.VirtualThread.park(VirtualThread.java:424)
at java.lang.System$2.parkVirtualThread(System.java:1279)
at sun.misc.VirtualThreads.park(VirtualThreads.java:56)
at java.util.concurrent.locks.LockSupport.park(LockSupport.java:183)
at java.util.concurrent.locks.AbstractQueuedSynchronizer.parkAndCheckInterrupt(AbstractQueuedSynchronizer.java:836)
at java.util.concurrent.locks.AbstractQueuedSynchronizer.acquireQueued(AbstractQueuedSynchronizer.java:870)
at java.util.concurrent.locks.AbstractQueuedSynchronizer.acquire(AbstractQueuedSynchronizer.java:1199)
at java.util.concurrent.locks.ReentrantLock$NonfairSync.lock(ReentrantLock.java:209)
at java.util.concurrent.locks.ReentrantLock.lock(ReentrantLock.java:285)
at java.util.concurrent.LinkedBlockingQueue.offer(LinkedBlockingQueue.java:418)
at java.util.concurrent.ThreadPoolExecutor.execute(ThreadPoolExecutor.java:1371)
at java.lang.VirtualThread.unpark(VirtualThread.java:502)
at java.lang.System$2.unparkVirtualThread(System.java:1287)
at sun.misc.VirtualThreads.unpark(VirtualThreads.java:70)
at ParkWithFixedThreadPool$1.run(ParkWithFixedThreadPool.java:51)

The solution is switch back to carrier thread before call scheduler.execute();

---

Progress

• Change must not contain extraneous whitespace

Reviewers

• Alan Bateman (@AlanBateman - Committer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/loom pull/59/head:pull/59
$ git checkout pull/59

Update a local copy of the PR:
$ git checkout pull/59
$ git pull https://git.openjdk.java.net/loom pull/59/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 59

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/loom/pull/59.diff

[bridgekeeper]

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

[openjdk]

⚠️ @miao-zheng the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout fibers
$ git commit -c user.name='Preferred Full Name' --allow-empty -m 'Update full name'
$ git push

[openjdk]

@miao-zheng 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:

avoid extra park of vthread when using fixed thread pool

Reviewed-by: alanb

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 79 new commits pushed to the fibers branch:

• 53c8342: Restore formatting
• 3f5ba19: Remove invalid test
• c6cfa2b: Restore change to java.naming module
• d203514: Replace lock/condition for termination
• 7dc2637: Improve shared containers
• 5bb5313: Improve start methods
• ef11573: Cleanup
• b505dc4: Expose thread pools as thread containers
• f621fe0: More prep work for integration with shared/owner containers
• 5b23398: Fixed framepop02.java
• ... and 69 more: https://git.openjdk.java.net/loom/compare/0d0962d74012aeb9aec137a8bb2ee89d5bea5ea9...fibers

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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@AlanBateman) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[openjdk]

⚠️ @miao-zheng a branch with the same name as the source branch for this pull request (fibers) is present in the target repository. If you eventually integrate this pull request then the branch fibers in your personal fork will diverge once you sync your personal fork with the upstream repository.

To avoid this situation, create a new branch for your changes and reset the fibers branch. You can do this by running the following commands in a local repository for your personal fork. Note: you do not have to name the new branch NEW-BRANCH-NAME.

$ git checkout -b NEW-BRANCH-NAME
$ git branch -f fibers 0d0962d74012aeb9aec137a8bb2ee89d5bea5ea9
$ git push -f origin fibers

Then proceed to create a new pull request with NEW-BRANCH-NAME as the source branch and close this one.

[mlbridge]

Webrevs

• 01: Full - Incremental (b08d720f)
• 00: Full (6654bfe6)

[AlanBateman]

Thanks for this issue. I think we have a regression here as unpark used to switch to the carrier to avoid parking on the virtual thread. Can you try this:

--- a/src/java.base/share/classes/java/lang/VirtualThread.java
+++ b/src/java.base/share/classes/java/lang/VirtualThread.java
@@ -664,7 +664,19 @@ class VirtualThread extends Thread {
         if (!getAndSetParkPermit(true) && Thread.currentThread() != this) {
             int s = state();
             if (s == PARKED && compareAndSetState(PARKED, RUNNABLE)) {
-                submitRunContinuation(tryPush);
+
+                if (Thread.currentThread() instanceof VirtualThread vthread) {
+                    Thread carrier = vthread.carrierThread;
+                    carrier.setCurrentThread(carrier);
+                    try {
+                        submitRunContinuation(tryPush);
+                    } finally {
+                        carrier.setCurrentThread(vthread);
+                    }
+                } else {
+                    submitRunContinuation(tryPush);
+                }
+
             } else if (s == PINNED) {
                 // signal pinned thread so that it continues
                 final ReentrantLock lock = getLock();

For the test, would you mind changing it to use try-with-resources so that it will shutdown the thread pool even if there is an exception.

As we're here, I should mention that it is still "TBD" if we will keep the support for custom schedulers in the first version. As you found, custom schedulers mean running arbitrary code at potentially critical points (like unparking). It might be that we need to give custom schedulers further thought before deciding whether to expose or not.

[miao-zheng]

@AlanBateman Thanks for your review and answer.
I have changed this PR on your suggestions.

[AlanBateman]

@AlanBateman Thanks for your review and answer.
I have changed this PR on your suggestions.

Thanks for the update. I'd like to do a bit more testing with this as I suspect it may require an update to at least one of the tests that exercises custom schedulers. Will get back to you soon on this.

[AlanBateman]

The CustomScheduler test was indeed failing with this change but it's due to a test issue where it runs the scheduler tasks on the current thread. I've remove that case from the test so I think we are good to go now.

[miao-zheng]

@AlanBateman Thanks for your review.
/integrate

[openjdk]

@miao-zheng
Your change (at version b08d720) is now ready to be sponsored by a Committer.

[AlanBateman]

/sponsor

[openjdk]

Going to push as commit e045367.
Since your change was applied there have been 79 commits pushed to the fibers branch:

• 53c8342: Restore formatting
• 3f5ba19: Remove invalid test
• c6cfa2b: Restore change to java.naming module
• d203514: Replace lock/condition for termination
• 7dc2637: Improve shared containers
• 5bb5313: Improve start methods
• ef11573: Cleanup
• b505dc4: Expose thread pools as thread containers
• f621fe0: More prep work for integration with shared/owner containers
• 5b23398: Fixed framepop02.java
• ... and 69 more: https://git.openjdk.java.net/loom/compare/0d0962d74012aeb9aec137a8bb2ee89d5bea5ea9...fibers

Your commit was automatically rebased without conflicts.

[openjdk]

@AlanBateman @miao-zheng Pushed as commit e045367.

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

[AlanBateman]

@miao-zheng The copyright header on the test is causing problems for a script that checks the copyright header on files. It doesn't seem to match the headers on files contributed by THL A29 Limited or Tencent. Can you check this?

[dholmes-ora]

The header has extra blank lines at the start and end that need to be removed.

[miao-zheng]

Sorry for this problem, I have create a new PR to fix the problem.