Skip to content
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

8263322: Calling Application.launch on FX thread should throw IllegalStateException, but causes deadlock #421

Conversation

@FlorianKirmaier
Copy link
Member

@FlorianKirmaier FlorianKirmaier commented Mar 9, 2021

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


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8263322: Calling Application.launch on FX thread should throw IllegalStateException, but causes deadlock

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/421/head:pull/421
$ git checkout pull/421

Update a local copy of the PR:
$ git checkout pull/421
$ git pull https://git.openjdk.java.net/jfx pull/421/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 421

View PR using the GUI difftool:
$ git pr show -t 421

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/421.diff

Fixing deadlock when calling Application.launch in the FXThread after Platform.launch
@FlorianKirmaier FlorianKirmaier changed the title JDK-8263322 Deadlock when calling Application.launch in the FXThread after Platform.startup 8263322 Deadlock when calling Application.launch in the FXThread after Platform.startup Mar 9, 2021
@FlorianKirmaier FlorianKirmaier changed the title 8263322 Deadlock when calling Application.launch in the FXThread after Platform.startup 8263322: Deadlock when calling Application.launch in the FXThread after Platform.startup Mar 9, 2021
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 9, 2021

👋 Welcome back fkirmaier! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Mar 9, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 9, 2021

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Mar 9, 2021

/reviewers 2

@openjdk
Copy link

@openjdk openjdk bot commented Mar 9, 2021

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@kevinrushforth kevinrushforth self-requested a review Mar 9, 2021
Copy link
Member

@kevinrushforth kevinrushforth left a comment

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 the FX Application 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.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Mar 10, 2021

/csr

@openjdk openjdk bot added the csr label Mar 10, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 10, 2021

@kevinrushforth has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@FlorianKirmaier please create a CSR request and add link to it in JDK-8263322. This pull request cannot be integrated until the CSR request is approved.

@kevinrushforth kevinrushforth requested a review from arapte Mar 10, 2021
@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Mar 10, 2021

  • Initalize the FX runtime via Platform.startup and then launch an Application on another thread (should throw ISE)

That should be:

  • Initalize the FX runtime via Platform.startup and then launch an Application on the FX Application thread (should throw ISE)

@FlorianKirmaier
Copy link
Member Author

@FlorianKirmaier FlorianKirmaier commented Mar 10, 2021

Contribution.MD states, that it's automatically checked for whitespace errors? Is this not true?

As a clarification, this PR restores the previous behavior before Platform.startup. With this PR Application.launch and Platform.startup behaves the same.

Small changes based on code review
Small changes based on code review
Added missing change
@FlorianKirmaier
Copy link
Member Author

@FlorianKirmaier FlorianKirmaier commented Mar 10, 2021

I forgot to commit a part of the fix, which is why the second test hang. It's now part of the PR.

@FlorianKirmaier
Copy link
Member Author

@FlorianKirmaier FlorianKirmaier commented Mar 10, 2021

About the CSR:
This commit actually restores original behavior, when JavaFX was called with Application.launch, so it's not really a change in the API, it's more like a fix for a regression.
How would I create a CSR? Just by creating a ticket at https://bugs.openjdk.java.net/issues/?jql=issuetype%20%3D%20CSR with the type CSR?

Added the tests for both cases, when JavaFX was initialized with Application.launch and Platform.startup
@FlorianKirmaier
Copy link
Member Author

@FlorianKirmaier FlorianKirmaier commented Mar 10, 2021

Initalize the FX runtime via Platform.startup and then launch an Application on another thread (should succeed)
I don't think that should succeed. I would expect it to throw an exception. If that would be the case, then both startup-methods would result in different states, which wouldn't be so good.

I've now restructured the tests.
It's a total of 4 tests. 2 after Application.launch and 2 after Platform.startup.
In both cases I check what happens, a Application is launched from another thread, or from the javafx thread. In all cases an exception is expected.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Mar 10, 2021

Contribution.MD states, that it's automatically checked for whitespace errors? Is this not true?

Yes, Skara's jcheck does basic sanity checking (tabs, trailing white space, line endings). Why do you ask? I didn't raise this as a question in this PR. Do you have reason to believe that this isn't working?

As a clarification, this PR restores the previous behavior before Platform.startup. With this PR Application.launch and Platform.startup behaves the same.

Not quite. Platform.startup is a new method that was previously available only via an internal implementation class, which simply starts up the FX runtime without running any of the application life cycle. Once this was added to public API, it became possible (but is seldom necessary) to call Platform.startup and then later call Application.launch, which should work fine as long as the latter call is not on the FX Application Thread. What was missed is a check to make sure that Application.launch is not called on the FX Application thread. This should have been documented to throw an exception. It is somewhat similar to, but not the same as, the case of calling Application.launch more than once.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Mar 10, 2021

Initalize the FX runtime via Platform.startup and then launch an Application on another thread (should succeed)
I don't think that should succeed. I would expect it to throw an exception. If that would be the case, then both startup-methods would result in different states, which wouldn't be so good.

No, this really should succeed. Internally, it is a similar case to what the special Java launcher method does when launching a Java class that extends Application and which may have a main method that calls Application.launch. We check the various cases of launching an application with/without it extending Application in the tests under tests/system/src/test/java/test/launchertest/.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Mar 10, 2021

To further clarify, Application.launch will start the FX platform only if it is not already started by some other means. Then it will (in all cases) run the applicaiton life-cycle.

Note this from the Application class docs:

Life-cycle

The entry point for JavaFX applications is the Application class. The JavaFX runtime does the following, in order, whenever an application is launched:

1. Starts the JavaFX runtime, if not already started (see Platform.startup(Runnable) for more information)
...

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Mar 10, 2021

About the CSR:
This commit actually restores original behavior, when JavaFX was called with Application.launch, so it's not really a change in the API, it's more like a fix for a regression.

Not quite. See my reply above.

How would I create a CSR? Just by creating a ticket at https://bugs.openjdk.java.net/issues/?jql=issuetype%20%3D%20CSR with the type CSR?

Almost. Go to your bug in JBS, and use the "More" pull-down menu. There is a "Create CSR" option.

Updated the unit-test so they match the wanted behavior discussed in the PR
removed unused imports, added missing change
@openjdk
Copy link

@openjdk openjdk bot commented Apr 7, 2021

@kevinrushforth The command sponsor can only be used in open pull requests.

28 similar comments
@openjdk
Copy link

@openjdk openjdk bot commented Apr 7, 2021

@kevinrushforth The command sponsor can only be used in open pull requests.

@openjdk
Copy link

@openjdk openjdk bot commented Apr 7, 2021

@kevinrushforth The command sponsor can only be used in open pull requests.

@openjdk
Copy link

@openjdk openjdk bot commented Apr 7, 2021

@kevinrushforth The command sponsor can only be used in open pull requests.

@openjdk
Copy link

@openjdk openjdk bot commented Apr 7, 2021

@kevinrushforth The command sponsor can only be used in open pull requests.

@openjdk
Copy link

@openjdk openjdk bot commented Apr 7, 2021

@kevinrushforth The command sponsor can only be used in open pull requests.

@openjdk
Copy link

@openjdk openjdk bot commented Apr 7, 2021

@kevinrushforth The command sponsor can only be used in open pull requests.

@openjdk
Copy link

@openjdk openjdk bot commented Apr 7, 2021

@kevinrushforth The command sponsor can only be used in open pull requests.

@openjdk
Copy link

@openjdk openjdk bot commented Apr 7, 2021

@kevinrushforth The command sponsor can only be used in open pull requests.

@openjdk
Copy link

@openjdk openjdk bot commented Apr 7, 2021

@kevinrushforth The command sponsor can only be used in open pull requests.

@openjdk
Copy link

@openjdk openjdk bot commented Apr 7, 2021

@kevinrushforth The command sponsor can only be used in open pull requests.

@openjdk
Copy link

@openjdk openjdk bot commented Apr 7, 2021

@kevinrushforth The command sponsor can only be used in open pull requests.

@openjdk
Copy link

@openjdk openjdk bot commented Apr 7, 2021

@kevinrushforth The command sponsor can only be used in open pull requests.

@openjdk
Copy link

@openjdk openjdk bot commented Apr 7, 2021

@kevinrushforth The command sponsor can only be used in open pull requests.

@openjdk
Copy link

@openjdk openjdk bot commented Apr 7, 2021

@kevinrushforth The command sponsor can only be used in open pull requests.

@openjdk
Copy link

@openjdk openjdk bot commented Apr 7, 2021

@kevinrushforth The command sponsor can only be used in open pull requests.

@openjdk
Copy link

@openjdk openjdk bot commented Apr 7, 2021

@kevinrushforth The command sponsor can only be used in open pull requests.

@openjdk
Copy link

@openjdk openjdk bot commented Apr 7, 2021

@kevinrushforth The command sponsor can only be used in open pull requests.

@openjdk
Copy link

@openjdk openjdk bot commented Apr 7, 2021

@kevinrushforth The command sponsor can only be used in open pull requests.

@openjdk
Copy link

@openjdk openjdk bot commented Apr 7, 2021

@kevinrushforth The command sponsor can only be used in open pull requests.

@openjdk
Copy link

@openjdk openjdk bot commented Apr 7, 2021

@kevinrushforth The command sponsor can only be used in open pull requests.

@openjdk
Copy link

@openjdk openjdk bot commented Apr 7, 2021

@kevinrushforth The command sponsor can only be used in open pull requests.

@openjdk
Copy link

@openjdk openjdk bot commented Apr 7, 2021

@kevinrushforth The command sponsor can only be used in open pull requests.

@openjdk
Copy link

@openjdk openjdk bot commented Apr 7, 2021

@kevinrushforth The command sponsor can only be used in open pull requests.

@openjdk
Copy link

@openjdk openjdk bot commented Apr 7, 2021

@kevinrushforth The command sponsor can only be used in open pull requests.

@openjdk
Copy link

@openjdk openjdk bot commented Apr 7, 2021

@kevinrushforth The command sponsor can only be used in open pull requests.

@openjdk
Copy link

@openjdk openjdk bot commented Apr 7, 2021

@kevinrushforth The command sponsor can only be used in open pull requests.

@openjdk
Copy link

@openjdk openjdk bot commented Apr 7, 2021

@kevinrushforth The command sponsor can only be used in open pull requests.

@openjdk
Copy link

@openjdk openjdk bot commented Apr 7, 2021

@kevinrushforth The command sponsor can only be used in open pull requests.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Apr 7, 2021

Looks like the Skara bot is in a loop continually reprocessing the sponsor command it already processed. I filed SKARA-956 to track this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants