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

[Kevin Rushforth]

On Fri, 12 Mar 2021 14:49:41 GMT, Florian Kirmaier <fkirmaier at openjdk.org> wrote:

>> Fixing deadlock when calling Application.launch in the FXThread after Platform.startup
>
> Florian Kirmaier has updated the pull request incrementally with one additional commit since the last revision:
> 
>   JDK-8263322
>   Seperated the unit-tests into seperate files

Thanks for splitting up the tests. I ran them with the fix and all pass, I then ran them without the fix and only the one I expected to fail did.

Before approving this, can you change the title of the JBS bug and CSR issue to match (after doing that you will also need to update this PR title) as recommended in the CSR? My suggestion is something like:

Calling Application.launch on FX thread should throw IllegalStateException, but causes deadlock

Another thing I recommend is to comment out or remove the print statements in the tests. They are useful while debugging, but in production we prefer tests to only print something as a warning or error. 

I also left a few inline comments.

tests/system/src/test/java/test/com/sun/javafx/application/InitializeJavaFXBase.java line 35:

> 33: public class InitializeJavaFXBase {
> 34: 
> 35: 

Minor: there is an extra blank line here

tests/system/src/test/java/test/com/sun/javafx/application/InitializeJavaFXLaunchBase.java line 22:

> 20:             Application.launch(InitializeApp.class);
> 21:         }).start();
> 22:         appLatch.await(5, TimeUnit.SECONDS);

You should `assertTrue` the return value of `await`.

tests/system/src/test/java/test/com/sun/javafx/application/InitializeJavaFXStartupBase.java line 15:

> 13:             latch.countDown();
> 14:         });
> 15:         latch.await(5, TimeUnit.SECONDS);

You should `assertTrue` the return value of `await`.

tests/system/src/test/java/test/com/sun/javafx/application/InitializeJavaFXLaunch1Test.java line 41:

> 39: 
> 40:     @Test (timeout = 15000)
> 41:     public void testStartupThenLaunchInFX() throws Exception {

Would something like `testLaunchThenLaunchInFX` be a better name? As it is, it uses the same name as the test that really does use `Platform.startup` which I found confusing.

tests/system/src/test/java/test/com/sun/javafx/application/InitializeJavaFXLaunch2Test.java line 41:

> 39: 
> 40:     @Test (timeout = 15000)
> 41:     public void testStartupThenLaunch() throws Exception {

Would something like `testLaunchThenLaunch` be a better name?

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

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