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

8196100: javax/swing/text/JTextComponent/5074573/bug5074573.java fails #1424

Closed
wants to merge 9 commits into from

Conversation

@mrserb
Copy link
Member

@mrserb mrserb commented Nov 25, 2020

Here is a fix for one of the annoying bug, which causes random test failures in the CI.

We have a method Robot.waitForIdle(), which supposed to wait until the java and the native queue stabilized. The common use case is to click on the button or show the window, and call waitForIdle() which will wait until the native event will be dispatched by the X11/Windows/macOS as well as followed Java event(paint/focus/etc event) will be dispatched to the EDT.

Currently, it is implemented in this way:

  1. Flush the EventQueue, so all old events will be posted to EDT.
  2. Flush the native event queue, by posting the native event.
  3. Flush the EDT, by posting a java event.
  4. If some events(unrelated to waitForIdle machinery) were dispatched on steps 2. or 3. then repeat since step 1.

It is implemented that they because the native events caused by the OS usually generate the java events, and the java events may generate native events. So we have to wait for both queues (java/native).

But it has the next disadvantages:

  • It is implemented as a busy loop, which does not give a chance for the application to proceed further since it blocks 3 threads EDT/native toolkit thread like appkit/main thread.
  • It throws the InfiniteLoop exception if the number of attempts is bigger than 20. And this limitation is too small because some tests generate much more events during startup.
  • Note that the timeout value for the realSync method is 10 seconds, and it was assumed that this method will not be executed longer, but it uses this timeout for all events flushing which can lead up to 600 seconds (20 iters * 3 calls * 10 seconds).

The fix:

  • Add a small delay to the method, so the app can do something useful when waitForIdle() is called
  • The timeout=10 second is taken care of, we calculate the "end" time and exits if it is exceeded
  • The maximum number of attempts is increased to 100 and InfiniteLoop is removed.

Note that I have made one additional change to the new realSync implementation. At the beginning of the method I call:
EventQueue.invokeAndWait(() -> {/dummy implementation/});
It is needed to be sure that we flush the first event on EDT even if we spend more time than 10 seconds.


Progress

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

Testing

Linux x64 Linux x86 Windows x64 macOS x64
Build (6/6 failed) ✔️ (2/2 passed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed)

Failed test tasks

Issue

  • JDK-8196100: javax/swing/text/JTextComponent/5074573/bug5074573.java fails

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1424/head:pull/1424
$ git checkout pull/1424

@mrserb mrserb marked this pull request as draft Nov 25, 2020
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Nov 25, 2020

👋 Welcome back serb! 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
Copy link

@openjdk openjdk bot commented Nov 25, 2020

@mrserb The following labels will be automatically applied to this pull request:

  • 2d
  • awt
  • swing

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@mrserb mrserb marked this pull request as ready for review Nov 25, 2020
@openjdk openjdk bot added the rfr label Nov 25, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 25, 2020

Webrevs

@mrserb
Copy link
Member Author

@mrserb mrserb commented Nov 25, 2020

/label remove 2d

@openjdk openjdk bot removed the 2d label Nov 25, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 25, 2020

@mrserb
The 2d label was successfully removed.

@SuppressWarnings("serial")
public static class InfiniteLoop extends RuntimeException {
}

Copy link
Member

@azuev-java azuev-java Nov 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the rationale behind this exception as that in case of really stuck loop we could identify it. Why are we throwing it away? Now when we exceed the maximum allowed number of iterations we are assuming that the native event queue synchronization happened while it might not be the case.

Copy link
Member Author

@mrserb mrserb Nov 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that was the initial goal, but currently, it does not work well if some of the components use animation and periodically repainted we hit this exception.

@openjdk
Copy link

@openjdk openjdk bot commented Nov 26, 2020

@mrserb This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8196100: javax/swing/text/JTextComponent/5074573/bug5074573.java fails

Reviewed-by: kizune

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 75 new commits pushed to the master branch:

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Nov 26, 2020
@mrserb
Copy link
Member Author

@mrserb mrserb commented Nov 26, 2020

/integrate

@openjdk openjdk bot closed this Nov 26, 2020
@openjdk openjdk bot added the integrated label Nov 26, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 26, 2020

@mrserb Since your change was applied there have been 76 commits pushed to the master branch:

  • a8e3eab: 8245026: PsAdaptiveSizePolicy::_old_gen_policy_is_ready is unused
  • b1d1499: 8256956: RegisterImpl::max_slots_per_register is incorrect on AMD64
  • 20020d1: 8254360: Re-examine use of CodeBuffer::verify_section_allocation
  • e56a8df: 8257042: [aix] Disable os.release_one_mapping_multi_commits_vm gtest
  • 9d7121c: 8256713: SwingSet2 : Slider leaves tracks in uiScale=2
  • 434b98f: 8257077: ZGC: Remove ZWorkers::run_serial()
  • f3fc0e0: 8257079: ZGC: Fold ZMark::prepare_mark() into ZMark::start()
  • a14f02d: 8256267: Relax compiler/floatingpoint/NaNTest.java for x86_32 and lower -XX:+UseSSE
  • 7c73fff: 8256486: Linux/Windows-x86 builds broken after JDK-8254231
  • 461c5fc: 8256450: Add gz option to jmap to write a gzipped heap dump
  • ... and 66 more: https://git.openjdk.java.net/jdk/compare/1aa90ac6295e2cdda078eabf0a32e109c2e6c38f...master

Your commit was automatically rebased without conflicts.

Pushed as commit 973255c.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

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