Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ protected void runLoop(Runnable launchable) {

@Override
protected void _invokeAndWait(Runnable runnable) {
throw new UnsupportedOperationException("Not supported yet.");
processor.invokeAndWait(runnable);
}

@Override
Expand All @@ -79,6 +79,13 @@ protected void _leaveNestedEventLoop(Object retValue) {
processor.leaveCurrentLoop(retValue);
}

@Override
protected void finishTerminating() {
processor.stopProcessing();
setEventThread(null);
super.finishTerminating();
}

@Override
protected int _isKeyLocked(int keyCode) {
return KeyEvent.KEY_LOCK_OFF;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import com.sun.glass.ui.Application;
import java.util.LinkedList;
import java.util.concurrent.BlockingQueue;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.LinkedBlockingQueue;

public class NestedRunnableProcessor implements Runnable {
Expand All @@ -44,6 +45,28 @@ void invokeLater(final Runnable r) {
runnableQueue.add(r);
}

void invokeAndWait(final Runnable r) {
final CountDownLatch latch = new CountDownLatch(1);
runnableQueue.add(() -> {
try {
r.run();
} finally {
latch.countDown();
}
});
try {
latch.await();
} catch (InterruptedException e) {
Application.reportException(e);
}
}

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

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.active volatile.

Copy link
Collaborator

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?

Copy link
Contributor

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.

Copy link
Member

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.

Copy link
Collaborator

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.

Copy link
Contributor

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? ;-)

Copy link
Collaborator

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 :-)

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.

Copy link
Contributor

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 volatile might be a few nanoseconds on modern systems.

Copy link
Member

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.

}
}

public Object newRunLoop() {
RunLoopEntry entry = new RunLoopEntry();

Expand All @@ -68,7 +91,9 @@ public void leaveCurrentLoop(Object returnValue) {

private static class RunLoopEntry {

// This is only accessed on the event thread
boolean active;

Object returnValue;
}

Expand Down
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");
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());
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();
}

}
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");
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assertions.fail() might work too

}

assertFalse(fail.get());
assertFalse(Thread.getAllStackTraces().keySet().stream().anyMatch(t -> "JavaFX Application Thread".equals(t.getName())));
}

}