8369306: Implement invokeAndWait and finishTerminating on headless platform #1934

jperedadnr · 2025-10-10T09:25:39Z

This PR implements invokeAndWait and finishTerminating for the headless platform and includes two system tests, one for each, that fail before this PR, and pass after it.

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-8369306: Implement invokeAndWait and finishTerminating on headless platform (Bug - P4)

Reviewers

Kevin Rushforth (@kevinrushforth - Reviewer)
Johan Vos (@johanvos - Reviewer) Review applies to 47a450a6
Andy Goryachev (@andy-goryachev-oracle - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1934

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1934.diff

Using Webrev

Link to Webrev Comment

…ncluding tests

bridgekeeper · 2025-10-10T09:27:15Z

👋 Welcome back jpereda! 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 · 2025-10-10T09:27:36Z

@jperedadnr 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:

8369306: Implement invokeAndWait and finishTerminating on headless platform

Reviewed-by: kcr, angorya, jvos

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 6 new commits pushed to the master branch:

2026a24: 8365635: Add MOUSE_DRAG_DONE event type
4f4540f: 8361286: Allow enabling of background loading for images loaded from an InputStream
c61e269: 8360586: JavaFX build uses deprecated features that will be removed in gradle 9
... and 3 more: https://git.openjdk.org/jfx/compare/eb3fcc72e6be3560cc320ea85dc38c96d2536a53...master

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.

mlbridge · 2025-10-10T09:31:25Z

Webrevs

kevinrushforth

I left a recommendation to also set "prism.order" to "sw" as discussed in this mailing list thread and a couple other minor comments.

tests/system/src/test/java/test/com/sun/glass/ui/headless/HeadlessApplication1Test.java

tests/system/src/test/java/test/com/sun/glass/ui/headless/HeadlessApplication2Test.java

kevinrushforth · 2025-10-10T11:51:04Z

tests/system/src/test/java/test/com/sun/glass/ui/headless/HeadlessApplication2Test.java

+            assertTrue(Platform.isFxApplicationThread());
+            Platform.runLater(Platform::exit);
+        });
+        Util.sleep(10);


Is 10msec enough?

I'm testing on an M1, but you are right, on different environments it could take longer.

I've added now an exit latch from PlatformImplShim.test_getPlatformExitLatch(), so we wait whatever is needed until PlatformImpl.tkExit() finishes, and then those 10 ms should be more than enough (but we still need to wait a couple of ms nonetheless until the JavaFX thread is fully gone).

In general, I'm not a fan of adding hard-numbered sleep times. Either the expected events are delivered, or they are not (yet). The hard sleep pattern leads to either slowdowns or to failing tests on slow environments.
Using CountdownLatches and deterministic flows is much better for testing, imho.
Since the Util.sleep is widely used in other system tests, I'm ok with using it here as well -- but I still think it is a dangerous approach.

I agree with @johanvos , it's always better to wait for the lock (with a timeout)

Better, sure, but it isn't always possible in practice. There are times where a delay is still needed.

This test now waits for the lock on the condition that will lead to the thread shutting down. Good. It's still necessary to sleep for a small time to be sure that it has.

As I said in my previous comment, I don't see this as a showstopper for this PR, but since it might help in general, I keep thinking about approaches to remove the sleep.
Wouldn't it be an option here to obtain the JavaFX Application Thread after the assertTrue(Platform.isFxApplicationThread()); and assign it to e.g. Thread fxThread so that instead of the Util.sleep we can do fxThread.join(10) ? (I might be missing something though)

Yes, something like what you describe might work in this case.

As for the more general problem, which goes well beyond this PR, there are many places in our tests where we do need sleeps. They fall into two cases

Lack of a feature that provides a deterministic way to know when something has occurred. Implementing the missing functionality could allow us to replace sleeps with something more deterministic.

Inherent limitations where there cannot be a deterministic way to know when something has occurred.

Many, if not most, of the instances of 1 could be addressed with an implementation of Robot::waitForIdle. It's a long-standing feature request. It's not rocket science to implement, but isn't trivial either.

Most of the instances of 2 are due to either A) platform delay in realizing certain window system operations (show/hide/minimize/maximize/to-front/etc); or B) inherent GC non-determinism.

I've replaced Util.sleep(10) with fxThread.join(10)

kevinrushforth

LGTM. The two new tests fail without the fix and pass with the fix.

Wait for 1 business day (so, Monday some time) to give others a change to review. In particular @johanvos might want to look at it.

andy-goryachev-oracle · 2025-10-10T14:47:13Z

modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/NestedRunnableProcessor.java

+
+    void stopProcessing() {
+        for (RunLoopEntry entry : activeRunLoops) {
+            runnableQueue.add(() -> entry.active = false);


I would highly recommend declaring RunLoopEntry.active volatile.

Can you elaborate on this? What usecase do you see where 2 threads might see different values, leading to problems that could not occur if active was volatile?

generally speaking, when two threads modify a single boolean variable, there might be scenarios, depending on the exact hardware, when the other thread does not see the modified value right away.

some of the comments in

https://stackoverflow.com/questions/65037547/the-volatile-keyword-and-cpu-cache-coherence-protocol
https://stackoverflow.com/questions/106591/what-is-the-volatile-keyword-useful-for

might explain it better.

Can this variable be accessed by two threads where one of them doesn't synchronize some other way? If so, then Andy has a point.

I'm aware of the purpose of volatile :)
However, I'm also aware of the cost of volatile, hence I prefer not to do it unless I know there is a possible scenario where it is needed.
That is why I'm asking the same question as the one asked by Kevin.

I know you do - the links were added for sake of others :-)

The answer to your question is - I don't know. If one can guarantee that there is only one thread accessing it, we may need to add a comment (though things might change in the future, and no one reads comments elsewhere in the code). And if not, then some form of synchronization is mandatory.

I prefer not to think though, and make sure we don't plant booby traps in the code.

BTW, what is the cost of volatile? 0.1 ns? ;-)

I know you do - the links were added for sake of others :-)

very kind ;)

The answer to your question is - I don't know. If one can guarantee that there is only one thread accessing it, we may need to add a comment (though things might change in the future, and no one reads comments elsewhere in the code). And if not, then some form of synchronization is mandatory.

I ran all system tests to see when/how the active field was read/modified and on which thread. All operations did happen on the main/eventThread, hence single-threaded. Since all of this is about processing events for the eventThread, this makes sense. But to make sure, I did the following research:

There are 3 places in NestedRunnableProcessor where the active field is used (read or write)

Object newRunLoop()

The first time this is invoked is when the HeadlessApplication starts, when Application.run is called. This creates a new Thread, which is the eventThread. The first thing this thread does is invoking runLoop (hence, it is executed on the eventThread).
Apart from the first time, this is invoked from Application.enterNestedEventLoop. which clearly states in its javadoc that the method may only be invoked on the main (event handling) thread, and it even does a checkEventThread() for this.

void leaveCurrentLoop()

This is invoked from Application.leaveNestedEventLoop, which has the same docs + check as enterNestedEventLoop

void stopProcessing()

This is invoked from Application.terminate, which starts with a checkEventThread

In summary, all operations in the NestedRunnableProcessor or controlled/gated by the com.sun.glass.ui.Application which makes sure that all access is guarded by checkEventThread

I prefer not to think though, and make sure we don't plant booby traps in the code.
I don't think I follow this approach. There are many concerns about "JavaFX being slow", so if we can avoid it, I think we should. That doesn't give green light to plant booby traps of course ;)

BTW, what is the cost of volatile? 0.1 ns? ;-)

I'm not an expert, but I believe it is more than 1 ns on "typical desktop systems". Since this is evaluated on every invocation of a Runnable on the AppThread (hence all pulses), plus on every Runnable executed via e.g. Platform.runLater(), this is something that can be invoked very often.

If there was even a single cornercase that we could not rule out, or document as "you're on your own here", I would absolutely agree to make it volatile. But as far as I see, there is no legal case where issues can occur that would have been prevented by adding volatile in this case.

Thank you for the analysis, very helpful! I would like to ask for a comment to be added, explaining the design. The main reason is I've seen too many times when the code was broken because the original assumptions were no longer valid, long after the knowledgeable people have left. At least the comment might help future reviewers. (I'll re-approve if you decide to add the comment).

BTW, you are right - according to https://shipilev.net/blog/2014/on-the-fence-with-dependencies/ the overhead of volatile might be a few nanoseconds on modern systems.

Glass is single-threaded by design, except for those methods that may be called on any thread. So I don't think a detailed comment is needed. A brief comment saying "this is only accessed on the JavaFX application thread (or event thread) might be helpful.

johanvos

This is a good PR that adds previously missing functionality.

andy-goryachev-oracle · 2025-10-14T15:32:05Z

tests/system/src/test/java/test/com/sun/glass/ui/headless/HeadlessApplication2Test.java

+        try {
+            fxThread.join(10);
+        }  catch (InterruptedException e) {
+            fail.set(true);


Assertions.fail() might work too

kevinrushforth · 2025-10-14T15:50:10Z

modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/NestedRunnableProcessor.java

+
+    void stopProcessing() {
+        for (RunLoopEntry entry : activeRunLoops) {
+            runnableQueue.add(() -> entry.active = false);


Glass is single-threaded by design, except for those methods that may be called on any thread. So I don't think a detailed comment is needed. A brief comment saying "this is only accessed on the JavaFX application thread (or event thread) might be helpful.

jperedadnr · 2025-10-14T16:46:30Z

/integrate

openjdk · 2025-10-14T16:47:38Z

Going to push as commit 38300ca.
Since your change was applied there have been 6 commits pushed to the master branch:

2026a24: 8365635: Add MOUSE_DRAG_DONE event type
4f4540f: 8361286: Allow enabling of background loading for images loaded from an InputStream
c61e269: 8360586: JavaFX build uses deprecated features that will be removed in gradle 9
... and 3 more: https://git.openjdk.org/jfx/compare/eb3fcc72e6be3560cc320ea85dc38c96d2536a53...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-10-14T16:47:47Z

@jperedadnr Pushed as commit 38300ca.

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

Implement invokeAndWait and finishTerminating on headless platform, i…

3812845

…ncluding tests

openjdk bot added the rfr Ready for review label Oct 10, 2025

kevinrushforth reviewed Oct 10, 2025

View reviewed changes

jperedadnr added 2 commits October 10, 2025 14:41

Process feedback from reviewer

bcf70f3

Process feedback from reviewer

47a450a

kevinrushforth approved these changes Oct 10, 2025

View reviewed changes

openjdk bot added the ready Ready to be integrated label Oct 10, 2025

andy-goryachev-oracle reviewed Oct 10, 2025

View reviewed changes

johanvos approved these changes Oct 11, 2025

View reviewed changes

Process feedback

484f5cb

openjdk bot removed the ready Ready to be integrated label Oct 14, 2025

andy-goryachev-oracle approved these changes Oct 14, 2025

View reviewed changes

openjdk bot added the ready Ready to be integrated label Oct 14, 2025

andy-goryachev-oracle reviewed Oct 14, 2025

View reviewed changes

kevinrushforth approved these changes Oct 14, 2025

View reviewed changes

Process feedback

9fa25f0

andy-goryachev-oracle approved these changes Oct 14, 2025

View reviewed changes

kevinrushforth approved these changes Oct 14, 2025

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Oct 14, 2025

openjdk bot closed this Oct 14, 2025

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Oct 14, 2025

tresf mentioned this pull request Oct 20, 2025

Fetch dependencies using Ivy qzind/tray#1216

Open

8369306: Implement invokeAndWait and finishTerminating on headless platform #1934

8369306: Implement invokeAndWait and finishTerminating on headless platform #1934

Uh oh!

Conversation

jperedadnr commented Oct 10, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Oct 10, 2025

Uh oh!

openjdk bot commented Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

kevinrushforth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinrushforth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johanvos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jperedadnr commented Oct 14, 2025

Uh oh!

openjdk bot commented Oct 14, 2025

Uh oh!

openjdk bot commented Oct 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

jperedadnr commented Oct 10, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Oct 10, 2025 •

edited

Loading

mlbridge bot commented Oct 10, 2025 •

edited

Loading