-
Notifications
You must be signed in to change notification settings - Fork 546
8369306: Implement invokeAndWait and finishTerminating on headless platform #1934
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
3812845
Implement invokeAndWait and finishTerminating on headless platform, i…
jperedadnr bcf70f3
Process feedback from reviewer
jperedadnr 47a450a
Process feedback from reviewer
jperedadnr 484f5cb
Process feedback
jperedadnr 9fa25f0
Process feedback
jperedadnr File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
77 changes: 77 additions & 0 deletions
77
tests/system/src/test/java/test/com/sun/glass/ui/headless/HeadlessApplication1Test.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| /* | ||
| * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved. | ||
| * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. | ||
| * | ||
| * This code is free software; you can redistribute it and/or modify it | ||
| * under the terms of the GNU General Public License version 2 only, as | ||
| * published by the Free Software Foundation. Oracle designates this | ||
| * particular file as subject to the "Classpath" exception as provided | ||
| * by Oracle in the LICENSE file that accompanied this code. | ||
| * | ||
| * This code is distributed in the hope that it will be useful, but WITHOUT | ||
| * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or | ||
| * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License | ||
| * version 2 for more details (a copy is included in the LICENSE file that | ||
| * accompanied this code). | ||
| * | ||
| * You should have received a copy of the GNU General Public License version | ||
| * 2 along with this work; if not, write to the Free Software Foundation, | ||
| * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. | ||
| * | ||
| * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA | ||
| * or visit www.oracle.com if you need additional information or have any | ||
| * questions. | ||
| */ | ||
| package test.com.sun.glass.ui.headless; | ||
|
|
||
| import com.sun.glass.ui.Application; | ||
| import javafx.application.Platform; | ||
| import org.junit.jupiter.api.BeforeAll; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import java.util.concurrent.CountDownLatch; | ||
| import java.util.concurrent.TimeUnit; | ||
| import java.util.concurrent.atomic.AtomicBoolean; | ||
| import java.util.concurrent.atomic.AtomicInteger; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertFalse; | ||
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
|
||
| public class HeadlessApplication1Test { | ||
|
|
||
| @BeforeAll | ||
| public static void setup() throws Exception { | ||
| System.setProperty("glass.platform", "Headless"); | ||
jperedadnr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| System.setProperty("prism.order", "sw"); | ||
| } | ||
|
|
||
| @Test | ||
| public void invokeAndWaitFromBackgroundThreadTest() { | ||
| assertFalse(Platform.isFxApplicationThread()); | ||
| AtomicInteger counter = new AtomicInteger(); | ||
| AtomicBoolean fail = new AtomicBoolean(); | ||
| CountDownLatch waitLatch = new CountDownLatch(1); | ||
| Platform.startup(() -> { | ||
| assertTrue(Platform.isFxApplicationThread()); | ||
| new Thread(() -> { | ||
| assertFalse(Platform.isFxApplicationThread()); | ||
jperedadnr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| Application.invokeAndWait(counter::incrementAndGet); | ||
| assertEquals(1, counter.get()); | ||
| waitLatch.countDown(); | ||
| }).start(); | ||
| }); | ||
| try { | ||
| if (!waitLatch.await(1, TimeUnit.SECONDS)) { | ||
| fail.set(true); | ||
| } | ||
| } catch (InterruptedException e) { | ||
| fail.set(true); | ||
| } | ||
| assertFalse(fail.get()); | ||
| assertEquals(0, waitLatch.getCount()); | ||
| assertFalse(Platform.isFxApplicationThread()); | ||
| Platform.exit(); | ||
| } | ||
|
|
||
| } | ||
84 changes: 84 additions & 0 deletions
84
tests/system/src/test/java/test/com/sun/glass/ui/headless/HeadlessApplication2Test.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| /* | ||
| * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved. | ||
| * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. | ||
| * | ||
| * This code is free software; you can redistribute it and/or modify it | ||
| * under the terms of the GNU General Public License version 2 only, as | ||
| * published by the Free Software Foundation. Oracle designates this | ||
| * particular file as subject to the "Classpath" exception as provided | ||
| * by Oracle in the LICENSE file that accompanied this code. | ||
| * | ||
| * This code is distributed in the hope that it will be useful, but WITHOUT | ||
| * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or | ||
| * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License | ||
| * version 2 for more details (a copy is included in the LICENSE file that | ||
| * accompanied this code). | ||
| * | ||
| * You should have received a copy of the GNU General Public License version | ||
| * 2 along with this work; if not, write to the Free Software Foundation, | ||
| * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. | ||
| * | ||
| * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA | ||
| * or visit www.oracle.com if you need additional information or have any | ||
| * questions. | ||
| */ | ||
| package test.com.sun.glass.ui.headless; | ||
|
|
||
| import com.sun.javafx.application.PlatformImplShim; | ||
| import javafx.application.Platform; | ||
| import org.junit.jupiter.api.BeforeAll; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import java.util.concurrent.CountDownLatch; | ||
| import java.util.concurrent.TimeUnit; | ||
| import java.util.concurrent.atomic.AtomicBoolean; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertFalse; | ||
| import static org.junit.jupiter.api.Assertions.assertNotNull; | ||
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
|
||
| public class HeadlessApplication2Test { | ||
|
|
||
| private final CountDownLatch exitLatch = PlatformImplShim.test_getPlatformExitLatch(); | ||
| private Thread fxThread; | ||
|
|
||
| @BeforeAll | ||
| public static void setup() throws Exception { | ||
| System.setProperty("glass.platform", "Headless"); | ||
jperedadnr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| System.setProperty("prism.order", "sw"); | ||
| } | ||
|
|
||
| @Test | ||
| public void userThreadIsShutdownOnPlatformExitTest() { | ||
| assertFalse(Platform.isFxApplicationThread()); | ||
| AtomicBoolean fail = new AtomicBoolean(); | ||
| Platform.startup(() -> { | ||
| assertTrue(Platform.isFxApplicationThread()); | ||
| fxThread = Thread.currentThread(); | ||
| assertEquals(1, exitLatch.getCount()); | ||
| Platform.runLater(Platform::exit); | ||
| }); | ||
| try { | ||
| if (!exitLatch.await(1, TimeUnit.SECONDS)) { | ||
| fail.set(true); | ||
| } | ||
| } catch (InterruptedException e) { | ||
| fail.set(true); | ||
| } | ||
|
|
||
| assertEquals(0, exitLatch.getCount()); | ||
| assertFalse(fail.get()); | ||
|
|
||
| assertNotNull(fxThread); | ||
| try { | ||
| fxThread.join(10); | ||
| } catch (InterruptedException e) { | ||
| fail.set(true); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| } | ||
|
|
||
| assertFalse(fail.get()); | ||
| assertFalse(Thread.getAllStackTraces().keySet().stream().anyMatch(t -> "JavaFX Application Thread".equals(t.getName()))); | ||
| } | ||
|
|
||
| } | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would highly recommend declaring
RunLoopEntry.activevolatile.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very kind ;)
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
HeadlessApplicationstarts, whenApplication.runis called. This creates a new Thread, which is the eventThread. The first thing this thread does is invokingrunLoop(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 acheckEventThread()for this.void leaveCurrentLoop()This is invoked from
Application.leaveNestedEventLoop, which has the same docs + check asenterNestedEventLoopvoid stopProcessing()This is invoked from Application.terminate, which starts with a
checkEventThreadIn 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
checkEventThreadI'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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
volatilemight be a few nanoseconds on modern systems.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.