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

8232524: SynchronizedObservableMap cannot be be protected for copying/iterating #17

Closed
wants to merge 3 commits into from

Conversation

effad
Copy link

@effad effad commented Oct 18, 2019

By using the collection itself as synchronization lock we achieve behaviour that matches java.util.Collections classes.

I've create test cases that fail with the current way of synchronizing on a separate object.

I've removed unused constructors.

Progress

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

Issue

JDK-8232524: SynchronizedObservableMap cannot be be protected for copying/iterating

Approvers

  • Ambarish Rapte (arapte - Reviewer)
  • Kevin Rushforth (kcr - Reviewer)

By using the collection itself as synchronization lock we achieve behaviour that matches java.util.Collections classes.
@bridgekeeper
Copy link

bridgekeeper bot commented Oct 18, 2019

👋 Welcome back rlichten! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request (refresh this page to view it).

@kevinrushforth
Copy link
Member

You have many whitespace errors in your patch that will need to be fixed before git jcheck will pass. When you fix them, you can just push a new commit.

As an aside, you have uncovered a bug in the Skara PR bot where the server-side jcheck fails to complete if there are more than 50 errors. See SKARA-135.

@effad
Copy link
Author

effad commented Oct 21, 2019

You have many whitespace errors in your patch that will need to be fixed before git jcheck will pass. When you fix them, you can just push a new commit.

I'm trying to setup skara tools so that I can check changes before committing in the future.

As an aside, you have uncovered a bug in the Skara PR bot where the server-side jcheck fails to complete if there are more than 50 errors. See SKARA-135.

OMG, hope I didn't break things ;-)

@openjdk openjdk bot added the rfr Ready for review label Oct 21, 2019
@mlbridge
Copy link

mlbridge bot commented Oct 21, 2019

Webrevs

@effad
Copy link
Author

effad commented Oct 21, 2019

I think I have corrected the whitespace errors (I can see "All checks have passed"), is there anything else I can / should do for this pull request?

@kevinrushforth
Copy link
Member

This is now ready to be reviewed. Nothing more for you to do until there are questions or comments that arise during the review.

Copy link
Member

@arapte arapte left a comment

Choose a reason for hiding this comment

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

The change looks good to me, added a comment for a small change in test.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

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

Looks good. I can confirm that the new tests fail without the fix and pass with the fix.

@openjdk openjdk bot removed the rfr Ready for review label Dec 4, 2019
@openjdk
Copy link

openjdk bot commented Dec 4, 2019

@effad This change can now be integrated. The commit message will be:

8232524: SynchronizedObservableMap cannot be be protected for copying/iterating

Reviewed-by: arapte, kcr
  • If you would like to add a summary, use the /summary command.
  • To list additional contributors, use the /contributor command.

Since the source branch of this PR was last updated there have been 34 commits pushed to the master branch:

  • a68347c: 8235150: IosApplication does not pass the required object in _leaveNestedEventLoopImpl
  • 1c27fbd: 8210955: DOMTest::testEventListenerCascade fails
  • 2d4096a: 8235151: Nonexistent notifyQuit method referred from iOS GlassHelper.m
  • 98035cb: 8211308: Support HTTP/2 in WebView
  • 6892fa1: 8232064: Switch FX build to use JDK 13.0.1 as boot JDK
  • 1d670f1: 8200224: Multiple press event when JFXPanel gains focus
  • 83eb0a7: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
  • 798afbc: 8230610: Upgrade GStreamer to version 1.16.1
  • 126896d: 8234704: Fix attribution in libxslt.md
  • 4d3c723: 8234593: Mark LeakTest.testGarbageCollectability as unstable
  • 5a39824: 8234056: Upgrade to libxslt 1.1.34
  • 8bea7b7: 8229472: Deprecate for removal JavaBeanXxxPropertyBuilders constructors
  • aad1720: 8233420: Upgrade to gcc 8.3 on Linux
  • 42040c4: 8232063: Upgrade gradle to version 6.0
  • aab07a4: 8234239: [TEST_BUG] Reenable few ignored web tests
  • 95ad601: 8233421: Upgrade to Visual Studio 2017 version 15.9.16
  • 3e0557a: 8234303: [TEST_BUG] Correct ignore tag in graphics unit tests
  • e37cb37: 8234150: Address ignored tests in ComboBoxTest, LabeledTest, HyperLinkTest and TextInputControlTest
  • 4f496d4: 8234194: [TEST_BUG] Reenable few graphics unit tests
  • 927fc8a: 8234174: Change IDEA VCS mapping to Git
  • 3d0cb49: 8234189: [TEST_BUG] Remove ignored and invalid graphics unit tests
  • dc01309: 8234110: SwingFXUtilsTest is unsuitable for unit test framework
  • 5b96ee4: 8231188: Update SQLite to version 3.30.1
  • d46dcae: 8233338: FX javadoc headings are out of sequence
  • 94bcf3f: 8231692: Test Infrastructure: enhance KeyEventFirer to inject keyEvents into scene
  • 286d1b5: 8230492: font-family not set in HTMLEditor if font name has a number in it
  • f74f3af: 8233040: ComboBoxPopupControl: remove eventFilter for F4
  • a1cc4ab: 8232210: Update Mesa 3-D Headers to version 19.2.1
  • dca8df4: 8232943: Gesture support is not initialized on iOS
  • 5a70b0c: 8189092: ArrayIndexOutOfBoundsException on Linux in getCachedGlyph
  • ac71396: 8232929: Duplicate symbols when building static libraries
  • ab6ea3b: 8232158: [macOS] Fallback to command line tools if xcode is missing
  • 2ae171a: 8232687: No static JNI loader for libprism-sw
  • a09a0fa: 8232522: FX: Update copyright year in docs, readme files to 2020

Since there are no conflicts, your changes will automatically be rebased on top of the above commits when integrating. If you prefer to do this manually, please merge master into your branch first.

As you do not have Committer status in this project, an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@arapte, @kevinrushforth) but any other Committer may sponsor as well.

  • To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Ready to be integrated label Dec 4, 2019
@kevinrushforth
Copy link
Member

Note that this is still pending a second review from @arapte

Copy link
Member

@arapte arapte left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@kevinrushforth
Copy link
Member

@effad you can now integrate this whenever you are ready. I will sponsor it.

@effad
Copy link
Author

effad commented Dec 5, 2019

/integrate

@openjdk
Copy link

openjdk bot commented Dec 5, 2019

@effad
Your change (at version 7c5cf19) is now ready to be sponsored by a Committer.

@openjdk openjdk bot added the sponsor Ready to sponsor label Dec 5, 2019
@kevinrushforth
Copy link
Member

/sponsor

@openjdk openjdk bot closed this Dec 5, 2019
@openjdk openjdk bot added integrated Pull request has been integrated and removed sponsor Ready to sponsor ready Ready to be integrated labels Dec 5, 2019
@openjdk
Copy link

openjdk bot commented Dec 5, 2019

@kevinrushforth @effad The following commits have been pushed to master since your change was applied:

  • a68347c: 8235150: IosApplication does not pass the required object in _leaveNestedEventLoopImpl
  • 1c27fbd: 8210955: DOMTest::testEventListenerCascade fails
  • 2d4096a: 8235151: Nonexistent notifyQuit method referred from iOS GlassHelper.m
  • 98035cb: 8211308: Support HTTP/2 in WebView
  • 6892fa1: 8232064: Switch FX build to use JDK 13.0.1 as boot JDK
  • 1d670f1: 8200224: Multiple press event when JFXPanel gains focus
  • 83eb0a7: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
  • 798afbc: 8230610: Upgrade GStreamer to version 1.16.1
  • 126896d: 8234704: Fix attribution in libxslt.md
  • 4d3c723: 8234593: Mark LeakTest.testGarbageCollectability as unstable
  • 5a39824: 8234056: Upgrade to libxslt 1.1.34
  • 8bea7b7: 8229472: Deprecate for removal JavaBeanXxxPropertyBuilders constructors
  • aad1720: 8233420: Upgrade to gcc 8.3 on Linux
  • 42040c4: 8232063: Upgrade gradle to version 6.0
  • aab07a4: 8234239: [TEST_BUG] Reenable few ignored web tests
  • 95ad601: 8233421: Upgrade to Visual Studio 2017 version 15.9.16
  • 3e0557a: 8234303: [TEST_BUG] Correct ignore tag in graphics unit tests
  • e37cb37: 8234150: Address ignored tests in ComboBoxTest, LabeledTest, HyperLinkTest and TextInputControlTest
  • 4f496d4: 8234194: [TEST_BUG] Reenable few graphics unit tests
  • 927fc8a: 8234174: Change IDEA VCS mapping to Git
  • 3d0cb49: 8234189: [TEST_BUG] Remove ignored and invalid graphics unit tests
  • dc01309: 8234110: SwingFXUtilsTest is unsuitable for unit test framework
  • 5b96ee4: 8231188: Update SQLite to version 3.30.1
  • d46dcae: 8233338: FX javadoc headings are out of sequence
  • 94bcf3f: 8231692: Test Infrastructure: enhance KeyEventFirer to inject keyEvents into scene
  • 286d1b5: 8230492: font-family not set in HTMLEditor if font name has a number in it
  • f74f3af: 8233040: ComboBoxPopupControl: remove eventFilter for F4
  • a1cc4ab: 8232210: Update Mesa 3-D Headers to version 19.2.1
  • dca8df4: 8232943: Gesture support is not initialized on iOS
  • 5a70b0c: 8189092: ArrayIndexOutOfBoundsException on Linux in getCachedGlyph
  • ac71396: 8232929: Duplicate symbols when building static libraries
  • ab6ea3b: 8232158: [macOS] Fallback to command line tools if xcode is missing
  • 2ae171a: 8232687: No static JNI loader for libprism-sw
  • a09a0fa: 8232522: FX: Update copyright year in docs, readme files to 2020

Your commit was automatically rebased without conflicts.

Pushed as commit 46338d0.

@mlbridge
Copy link

mlbridge bot commented Dec 5, 2019

Mailing list message from Kevin Rushforth on openjfx-dev:

Changeset: 46338d0
Author: Robert Lichtenberger
Committer: Kevin Rushforth
Date: 2019-12-05 12:51:11 +0000
URL: https://git.openjdk.java.net/jfx/commit/46338d02

8232524: SynchronizedObservableMap cannot be be protected for copying/iterating

Reviewed-by: arapte, kcr

! modules/javafx.base/src/main/java/javafx/collections/FXCollections.java
! modules/javafx.base/src/test/java/test/javafx/collections/FXCollectionsTest.java

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated
Development

Successfully merging this pull request may close these issues.

3 participants