RFR: 8263322: Deadlock when calling Application.launch in the FXThread after Platform.startup

[Kevin Rushforth]

On Tue, 9 Mar 2021 21:49:36 GMT, Florian Kirmaier <fkirmaier at openjdk.org> wrote:

> Fixing deadlock when calling Application.launch in the FXThread after Platform.startup

The idea behind the fix is good. A few changes are needed:

1. The check should be split into two separate `if` statements, each with their own error message (see inline).

2. This should be documented in the API docs for the Application::launch method as an additional case that will throw an IllegalStateException. It will need a reasonably trivial CSR since we are specifying a new case that will cause an exception to be thrown.

3. The system test need to be broken up into separate files, one for each `@Test` method, since application launching tests need to run in their own JVM. If you want to share any code, you could refactor it into a common class that has methods (not annotated with `@Test`) to perform the testing, and separate classes that will subclass the common class, each with a single `@Test` method that simply calls into the method in the common class to do the testing. See any number of examples in `tests/system/src/test/java/test/com/sun/javafx/application/` (which is also a better location for your new test). You will want to add a cleanup method annotated with `@AfterClass` that calls `Platform.exit()`. I think three tests would be good:
    * Initalize the FX runtime via Platform.startup and then launch an Application on another thread (should succeed)
    * Initalize the FX runtime via Platform.startup and then launch an Application on another thread (should throw ISE)
    * Initalize the FX runtime via Application.launch and then launch a second Application on another thread (should throw ISE).


I note that when I run your test, it hangs (on Windows...I didn't try it on other platforms).

gradle --info -PFULL_TEST=true cleanTest :systemTests:test --test InitializeJavaFXTest

This might be resolved by ensuring each test is in its own JVM as requested above.


Additional comments are inline.

modules/javafx.graphics/src/main/java/com/sun/javafx/application/LauncherImpl.java line 174:

> 172:             final String[] args) {
> 173: 
> 174:         if (com.sun.glass.ui.Application.isEventThread() || launchCalled.getAndSet(true)) {

Can you do this as two separate `if` checks, with the `isEventThread` check first? The error message for this case should be something like "Application launch must not be called on the JavaFX Application Thread".

tests/system/src/test/java/test/javafx/scene/InitializeJavaFXTest.java line 1:

> 1: package test.javafx.scene;

1. Missing Copyright header block
2. Other platform startup tests are in `test.com.sun.javafx.application`; can you move this test there as well?

tests/system/src/test/java/test/javafx/scene/InitializeJavaFXTest.java line 24:

> 22:     }
> 23: 
> 24:     public static void initializeApplication() throws Exception {

This method is unused, along with the `InitializeApp` class. Did you plan to use it?

tests/system/src/test/java/test/javafx/scene/InitializeJavaFXTest.java line 28:

> 26:             Application.launch(InitializeApp.class);
> 27:         }).start();
> 28:         appLatch.await();

Presuming you intend to use this (as opposed to removing it), this should be changed to an await with a timeout.

tests/system/src/test/java/test/javafx/scene/InitializeJavaFXTest.java line 36:

> 34:             latch.countDown();
> 35:         });
> 36:         latch.await();

This needs to be changed to a flavor of await with a timeout (you can assert that it doesn't timeout). Also, I don't think this needs to be its own method, since the only thing the `initialize` method does is call this.

tests/system/src/test/java/test/javafx/scene/InitializeJavaFXTest.java line 65:

> 63:             } catch (Exception e) {
> 64:                 System.out.println("got exception:  " + e);
> 65:                 e.printStackTrace();

You should rethrow the exception in this case, since it indicates an unexpected error. Better still, you can remove this block and not catch the Exception in the first place.

tests/system/src/test/java/test/javafx/scene/InitializeJavaFXTest.java line 53:

> 51:     }
> 52: 
> 53:     @Test

All of the test methods should take a timeout parameter: `@Test (timeout = 15000)`

tests/system/src/test/java/test/javafx/scene/InitializeJavaFXTest.java line 77:

> 75:             Application.launch(TestApp.class);
> 76:             System.out.println("Finished launch!");
> 77:             throw new Exception("We excpect an error!");

Usual practice in this case would be to use the junit `Assert.fail` method (or else `throw new AssertionError(...)`).

tests/system/src/test/java/test/javafx/scene/InitializeJavaFXTest.java line 56:

> 54:     public void testStartupThenLaunchInFX() throws Exception {
> 55:         CountDownLatch latch = new CountDownLatch(1);
> 56:         Platform.runLater(() -> {

I recommend using `Util.runAndWait` since it will propagate exceptions from the `runLater` lambda to the caller.

tests/system/src/test/java/test/javafx/scene/InitializeJavaFXTest.java line 68:

> 66:             }
> 67:         });
> 68:         Assert.assertTrue("Timeout", latch.await(10, TimeUnit.SECONDS));

If you use `runAndWait` you won't need this.

-------------

Changes requested by kcr (Lead).

PR: https://git.openjdk.java.net/jfx/pull/421