On Fri, 12 Mar 2021 14:49:41 GMT, Florian Kirmaier <fkirma...@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

Reply via email to