JDK-8204336 : Platform.exit() throws ISE when a nested event loop is active
  • Type: Bug
  • Component: javafx
  • Sub-Component: application-lifecycle
  • Affected Version: 8,9,10,openjfx11
  • Priority: P3
  • Status: Resolved
  • Resolution: Fixed
  • OS: generic
  • CPU: x86_64
  • Submitted: 2018-06-05
  • Updated: 2018-07-27
  • Resolved: 2018-07-27
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
Other
openjfx11Fixed
Related Reports
Relates :  
JDK-8088859 - [Mac] Cannot exit FX application while file chooser is opened
Description
A DESCRIPTION OF THE PROBLEM :
java.lang.IllegalStateException is thrown if Platform.exit() is called when there is a "dialog" window open (i.e. a Stage opened via Stage.showAndWait()).

In typical use-cases, there isn't a reason why an Application would call Platform.exit() when there is an open dialog window. However, in my use-case, the Application has a main Window, which stays open until the Application is closed. The main window is capable of spawning more child windows. Some of the child windows are capable of opening dialog window with window-level modality. Lastly, the main window allows the user to end the whole Application with a single click, regardless of the state of the child windows.

STEPS TO FOLLOW TO REPRODUCE THE PROBLEM :
Use the sample code provided. The sample code does not show my use-case, but it does show how the exception can be triggered.

EXPECTED VERSUS ACTUAL BEHAVIOR :
EXPECTED -
Application would end normally/gracefully.
ACTUAL -
An exception is being thrown. This is the stacktrace:

Exception in thread "JavaFX Application Thread" java.lang.IllegalStateException: This operation is permitted on the event thread only; currentThread = JavaFX Application Thread
    at com.sun.glass.ui.Application.checkEventThread(Application.java:443)
    at com.sun.glass.ui.Application.isNestedLoopRunning(Application.java:544)
    at com.sun.javafx.tk.quantum.QuantumToolkit.isNestedLoopRunning(QuantumToolkit.java:1139)
    at com.sun.javafx.tk.quantum.QuantumToolkit.enterNestedEventLoop(QuantumToolkit.java:585)
    at javafx.stage.Stage.showAndWait(Stage.java:474)
    at test.lambda$0(test.java:43)
    at com.sun.javafx.application.PlatformImpl.lambda$null$173(PlatformImpl.java:295)
    at java.security.AccessController.doPrivileged(Native Method)
    at com.sun.javafx.application.PlatformImpl.lambda$runLater$174(PlatformImpl.java:294)
    at com.sun.glass.ui.InvokeLaterDispatcher$Future.run(InvokeLaterDispatcher.java:95)
    at com.sun.glass.ui.win.WinApplication._runLoop(Native Method)
    at com.sun.glass.ui.win.WinApplication.lambda$null$148(WinApplication.java:191)
    at java.lang.Thread.run(Thread.java:745)

---------- BEGIN SOURCE ----------
public class test extends Application {
    @Override
    public void start(final Stage primaryStage) throws Exception {
        final VBox root = new VBox();
        final Scene sc = new Scene(root);
        primaryStage.setScene(sc);
        primaryStage.show();

        // Platform.runLater is necessary because this start() must not be blocked (i.e. start() must finish executing)
        Platform.runLater(() -> {
            final Button button = new Button("Exit");
            button.setOnAction(e -> Platform.exit());

            Stage st = new Stage();
            st.setScene(new Scene(button));

            st.initOwner(primaryStage);
            st.initModality(Modality.WINDOW_MODAL); // Just need window-level modality

            st.showAndWait(); // Shows the "dialog" window
        });
    }

    public static void main(final String[] args) {
        Application.launch(args);
    }
}
---------- END SOURCE ----------

FREQUENCY : always
Comments
Changeset: 40c7eb6c04fd Author: pbansal Date: 2018-07-27 12:27 +0530 URL: http://hg.openjdk.java.net/openjfx/jfx-dev/rt/rev/40c7eb6c04fd
27-07-2018
+1
26-07-2018
The .05 version looks good. +1
26-07-2018
[~kcr] and [~mbilla] Sorry, my bad. Yes I added the file twice in file.list and while going through the patch, did not realize I saw the file twice. Please have look at the new webrev. Only different from .04 is the QuantumToolkit file is added once. webrev: http://cr.openjdk.java.net/~pbansal/8204336/webrev.05/
26-07-2018
I guess [~pbansal] might have mentioned the file QuantumToolkit.java 2 times while giving input to webrev.ksh
26-07-2018
Something is very odd with the webrev and patch in the .04 version. There are two copies of QuantumToolkit.java which causes the patch to produce an error when applied. Can you please fix this so there is no confusion over what will get pushed? I note that if I manually delete the second copy of QuantumToolkit from the patch it applies cleanly, so this is probably just a problem with how you generated the webrev (duplicate entries in a files.list file, maybe?) The changes themselves look good to me.
26-07-2018
[~kcr] and [~mbilla] Thanks for the review. I have made changes according to the review comments and the discussions. Please have a look. webrev: http://cr.openjdk.java.net/~pbansal/8204336/webrev.04/index.html
26-07-2018
I only have one comment on the .03 webrev, and it's a minor one: 1. In NestedEventLoopPlatformExitTest, you are missing a space before the '{' : 62 public static void initFX() throws InterruptedException{
25-07-2018
Regarding the above comments: 1. Maybe, but it isn't usually bad idea for unit tests to test that the expected still holds. In this case it does seem redundant, but I would leave it alone. 2. The initial sleep ensures that the exit flag doesn't count down once the platform is otherwise idle. It should remain. 3. Yes, the comments could be fixed. 4. I think there is value in keeping the simpler tests, so I do not recommend doing this. 5. This would be fine, but (as you say) not necessary 6. Yes, that would be good.
25-07-2018
webrev.03 looks fine.. Few minor comments: 1. For test cases "protected void doTestPlatformExit(boolean again)" and "doTestPlatformExitOnAppThread(boolean again)" checking again for assertEquals(0, exitLatch.getCount()); in case of again = true looks redundant since you already checking this assert first time u call Platform::exit on FX application thread. 2. is the first sleep (Util.sleep(DELAY);) required ? + protected void doTestPlatformExit(boolean again) { + Util.sleep(DELAY); + protected void doTestPlatformExitOnAppThread(boolean again) { + Util.sleep(DELAY); 3. Same comment is observed for all 4 test cases: I think you can correct the comment in case of doTestPlatformExit(true); as Test calling Platform.exit() 2 times after starting the FX runtime. Similar case for doTestPlatformExitOnAppThread +/** + * Test calling Platform.exit() after starting the FX runtime + */ 4. You can reduce 4 test cases to 2 by clubbing test case from PlatformExit1Test.java and PlatformExit2Test.java (similarly for 3 and 4 test case). 5. This comment is optional : Since the only difference between doTestPlatformExit and doTestPlatformExitOnAppThread is calling platform exit on app/test thread, I feel you can club both doTestPlatformExit and doTestPlatformExitOnAppThread by passing second parameter to distinguish whether you want to exit on FX app thread/test thread. 6. You can correct the copy right year in PlatformExitSimpleTest.java.
25-07-2018
[~kcr] Thanks for the review. I have made the suggested changes and verified that the changes are not introducing any regression on all three platforms. Please have a look. webrev: http://cr.openjdk.java.net/~pbansal/8204336/webrev.03/
24-07-2018
The .02 version looks much better. All of the existing tests that fail with .01, including the new "exit" tests that I have attached, now pass with the .02 version. Also, to respond to one of your earlier comments: > I see one problem that if we enter the NestedEventLoop and then close the window using the close button (red x), the program hangs indefinitely. That's not a bug, but is the expected behavior. If there are any nested event loops, then the platform is not idle. Here is my feedback on the .02 patch: 1. There is a mismatch in logic between the code in tkExit that waits for allNestedLoopsExitedLatch and the code that does the count-down. The former is run unconditionally, while the latter does a countdown only when platformExit is true. I recommend to qualify the newly added code in tkExit() with "if (platformExit.get())", since there is no need to run it when exiting implicitly: if (initialized.get()) { if (platformExit.get()) { PlatformImpl.runAndWait(() -> { ... } 2. QuantumToolkit.exitAllNestedEventLoops() should probably clear eventLoopMap after iterating over all values. 3. Please add the new platform exit tests found in platform-exit-tests.patch 4. A couple minor suggestions on your new test, relating to the initFx() method: - You could remove the try/catch in initFX() by having it "throws Exception" - You could call assertTrue(launchLatch.await(...)); rather than testing it for false and calling fail. Both of these are changes we want to make globally as part of JDK-8206430 so we might as well start using the new pattern now. Please make sure you run a full test on all three platforms as follows: $ gradle --continue -PFULL_TEST=true test
18-07-2018
What I meant is that SimpleExitTest throws an exception with the .01 version. I just verified that again on Linux and Mac. You already fixed it in the .02 version by moving the PlatformImpl.runAndWait call inside the "if (initialized.get())" block. My point in bringing it up was to highlight additional cases that need testing (including adding some automated tests). As for HelloRectangle, no it doesn't hang before your patch, but does hang with the .01 patch. I haven't yet verified it with the .02 patch. Again, I bring this up as an indication of another automated test that we need. Edited to add: actually we already do have a test for the "HelloRectangle" case: test.com.sun.javafx.application.SingleExitImplicitTest That test shows the error with your .01 patch, although it hangs only intermittently on my Linux system, suggesting that the hang is the result of a timing problem or race.
18-07-2018
[~kcr] <<Regarding the Platform.exit exception, you need to try it with a class that does *not* extend Application. I have attached a simple program, SimpleExitTest. I tried the test you have attached on Windows and Linux. This is not throwing exception for me before and after the fix. May be I am not following correctly. Can you please let me know how you managed to get the exception? <<Another problem I ran into this morning is that a simple test program (HelloRectangle for example) is hanging when you press the red X. Yes, this is present both with and without fix. We may add some new tests to cover these issues.
18-07-2018
Regarding the Platform.exit exception, you need to try it with a class that does *not* extend Application. I have attached a simple program, SimpleExitTest. Another problem I ran into this morning is that a simple test program (HelloRectangle for example) is hanging when you press the red X. Both of the above problems point out that we lack some test in that area, so I will suggest some new tests for this as part of my evaluation. I will review your .02 patch, test it, and give feedback.
18-07-2018
[~kcr] <<If Platform.exit is called without first initializing the FX runtime (e.g., a simple program whose main method calls Platform.exit() as its <<only action), it will now throw an exception. I tested this, but this is not throwing exception. I created main function and called Platform.exit. It did not throw exception. <<Related to this, tkExit might misbehave if called re-entrantly. This is due to where you put the new code that calls runAndWait. You <<need to move it inside the "if (initialized.get())" block before the call to Toolkit.exit. Done. <<The logic in exitedLastNestedLoop is not right. For one thing it won't handle the case where you enter and exit many times. It also <<won't handle the case of implicitExit = false, which you can see by just making that call in the attached test program, and notice that it <<will hang indefinitely. I have tried entering/exiting multiple time. It does not hand for me. But yes I think I should use platformExit.get instead of implicitExit. I have updated this. I have tested the testcase added and the attached test in JBS, they seem to work fine for me on all Windows, linux and Mac. I see one problem that if we enter the NestedEventLoop and then close the window using the close button (red x), the program hangs indefinitely. But the same behavior is present before the fix also. Except this I was not able to find other problems. webrev: http://cr.openjdk.java.net/~pbansal/8204336/webrev.02/
18-07-2018
-1 Here are a quick list of issues that I can see: 1. If Platform.exit is called without first initializing the FX runtime (e.g., a simple program whose main method calls Platform.exit() as its only action), it will now throw an exception. Related to this, tkExit might misbehave if called re-entrantly. This is due to where you put the new code that calls runAndWait. You need to move it inside the "if (initialized.get())" block before the call to Toolkit.exit. 2. The logic in exitedLastNestedLoop is not right. For one thing it won't handle the case where you enter and exit many times. It also won't handle the case of implicitExit = false, which you can see by just making that call in the attached test program, and notice that it will hang indefinitely. I also have an overall concern that not all cases are handled. This will need more thought and more testing.
18-07-2018
I want to look at the logic more closely. This is a tricky area with a lot of cases to consider.
17-07-2018
+1
17-07-2018
[~mbilla] Thanks for the review. Please have a look. webrev: http://cr.openjdk.java.net/~pbansal/8204336/webrev.01/
17-07-2018
Changes looks fine.. Few minor nits: 1. You can add "{" and "}" for below code if (isImplicitExit()) + allNestedLoopsExited.countDown(); 2. You can correct copyright year for DummyToolkit.java, Toolkit.java, QuantumToolkit.java and StubToolkit.java 3. Can you declare a variable for "1024L" in Platform.enterNestedEventLoop(1024L); for better understanding of this parameter (like "nested_event_loop_key") 4. Can you name allNestedLoopsExited as "allNestedLoopsExitedLatch" (though optional)
17-07-2018
[~mbilla]] Yes it does fail without fix without the Assert.assertTrue(Platform.isNestedLoopRunning()); part. The test is supposed to check the behavior of Platform.exit when the NestedEventLoop is running. Assert.assertTrue(Platform.isNestedLoopRunning()) is just to ensure that. But the actual test is to run the Platform.exit(). It throws the exception before the fix and fails.
13-07-2018
In case if you comment Assert.assertTrue(Platform.isNestedLoopRunning()); and test case is failing without fix by calling Platform.exit(), then my above suggestion may not be needed.
13-07-2018
[~pbansal] For test case testPlatformExitWithNestedEventLoop, can you check ISE exception for Platform.exit() (apart from checking isNestedLoopRunning)
13-07-2018
Hi Kevin & Murali, Please review the fix : http://cr.openjdk.java.net/~pbansal/8204336/webrev.00/ Issue: Platform.exit() throws ISE when a nested event loop is active Fix: The platform.exit should first exit all nested loops and then exit the Toolkit. Added a API in Toolkit to exit all the active Nested Event Loops and called the API before exiting the Toolkit. This does fix the problem Added a system test for verifying the issue and tested on Windows, Linux and Mac. The Test attached in JBS NestedEventLoopExitTest can also be used to test the fix by repeating the steps Mentioned by Kevin in comments. The test will not through the ISE and will exit successfully Note: In the added System test, in case of Windows and Linux, without the fix, the exception will be "java.lang.AssertionError: Internal inconsistency - wrong EventLoop" instead of ISE as mentioned in the Bug. The reason is that the Assersions are enabled in gradle by default but are disabled in JVM. So Assertion failed in EventLoop.java while running with gradle and the test gives error before reaching the ISE stage. but it reaches the ISE stage while running standalone test. To verify this, run the standalone test with -ea (for enabling Assertion) JVM argument and the same Assertion will be raised instead of ISE. Also to cross verify, you can comment out the Assertions in the EventLoop.java and you will get the ISE in gradle which is expected.
12-07-2018
I can confirm that this is a bug when calling Platform.exit with an active nested event loop. It isn't necessary for the second stage to be modal or even for it to be shown. The attached test program uses a single stage and allows entering or exiting a nested event loop (using the Platform::enterNestedEventLoop and Platform::exitNestedEventLoop methods from FX 9). To reproduce the bug with this test program: 1. Run the NestedEventLoopExitTest program 2. Press the "Enter nested event loop" button to enter a nested event loop 3. Press the "Platform Exit" button 4. BUG: the same exception as is shown in the Description of this bug will be thrown and the program will fail to exit.
06-06-2018
Additional Information from Submitter: Just to clarify, this exception occurs regardless where it is called from, as long as there is any Stage opened via Stage.showAndWait() that blocks its parent Stage.
06-06-2018
It may be the nested event loop that is preventing the platform from exiting properly. We will evaluate it.
05-06-2018
This behavior is same in all JDKs - 8,9,10,11. When Platform.exit() is called from opened dialog window with window-level modality, it gives "IllegalStateException".
05-06-2018