8347392: Thread-unsafe implementation of c.s.j.scene.control.skin.Utils #1691

andy-goryachev-oracle · 2025-01-30T23:35:05Z

Thread-safe and re-entrant implementation of Utils.

The new code still uses the static instances of Text and TextLayout for performance reasons, but adds a thread-safe mechanism to keep track of whether any of the instances is in use and when that happens, supplies a new instance instead. This solves both thread safety and re-entrancy.

/reviewers 2

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8347392: Thread-unsafe implementation of c.s.j.scene.control.skin.Utils (Bug - P4)

Reviewers

Kevin Rushforth (@kevinrushforth - Reviewer)
Michael Strauß (@mstr2 - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1691/head:pull/1691
$ git checkout pull/1691

Update a local copy of the PR:
$ git checkout pull/1691
$ git pull https://git.openjdk.org/jfx.git pull/1691/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1691

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1691.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-01-30T23:35:14Z

👋 Welcome back angorya! 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 · 2025-01-30T23:35:19Z

@andy-goryachev-oracle 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:

8347392: Thread-unsafe implementation of c.s.j.scene.control.skin.Utils

Reviewed-by: kcr, mstrauss

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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.

….safety

openjdk · 2025-02-03T19:15:39Z

⚠️ @andy-goryachev-oracle This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

….safety

….utils

openjdk · 2025-02-06T00:04:04Z

@andy-goryachev-oracle
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

mlbridge · 2025-02-06T00:08:49Z

Webrevs

01: Full - Incremental (1e9cb5d5)
00: Full (203f8de6)

andy-goryachev-oracle · 2025-02-06T22:32:14Z

@kevinrushforth I'd suggest to take a look at this first, as it blocks some other fixes. thanks!

kevinrushforth

The fix looks good. My testing is good as well. The now-enabled tests fail pretty reliably for me on Windows without your fix and all pass with your fix.

I left a few minor questions / comments, but will approve as is.

kevinrushforth · 2025-02-06T23:37:56Z

tests/system/src/test/java/test/robot/javafx/scene/NodeInitializationStressTest.java

-    @Disabled("JDK-8347392") // FIX
    @Test
    public void hyperlink() {


Minor: setVisited(nextBoolean())?

kevinrushforth · 2025-02-06T23:39:26Z

tests/system/src/test/java/test/robot/javafx/scene/NodeInitializationStressTest.java

-    @Disabled("JDK-8347392") // FIX
    @Test
    public void checkBox() {


Minor: this test calls setSelected(true). Would introducing randomness via setSelected(nextBoolean()) be useful here?

here and elsewhere, you are right

kevinrushforth · 2025-02-06T23:40:26Z

tests/system/src/test/java/test/robot/javafx/scene/NodeInitializationStressTest.java

-    @Disabled("JDK-8347392") // FIX
    @Test
    public void choiceBox() {


Minor: select random item?

mstr2 · 2025-02-07T07:58:47Z

What's the rationale for using a borrow pattern here, instead of simply synchronizing access to the object in question? I don't think we use such a pattern anywhere else. Do you expect heavy contention?

andy-goryachev-oracle · 2025-02-07T15:56:11Z

What's the rationale for using a borrow pattern here, instead of simply synchronizing access to the object in question?

A very good question!

This pattern seemed to be the most applicable here: the solution needs to be both thread-safe and reentrant, with the expectation that in most cases the simple scenario happens where the static instance is used.

We actually use that pattern using synchronization, see GlyphLayout::getInstance() for example. I think the use of AtomicBoolean is far better, so I took the same approach in #1667 .

Is there a specific scenario that would not work or cause issues?

kevinrushforth · 2025-02-07T16:51:25Z

To add a little more to what Andy said, I think there are three main approaches that could have been used here:

Remove the static fields and always allocate a new object each time it is needed.
Keep the static fields for the common case where there is only one thread (typically the JavaFX application thread); check and allocate a new object if the static fields are already in use
Replace the static fields with ThreadLocalStorage

There might be other solutions, but we don't want anything too complicated. Andy decided to go with 2.

andy-goryachev-oracle · 2025-02-07T17:55:28Z

Hmmm, to further make a point: options 2 and 3 may not be re-entrant, so they probably won't work.

We haven't encounter this re-entrancy issue, but I suppose it might be possible to have a text component embedded in another text component (e.g. a TextArea embedded inside a RichTextArea for some reason). Even if the access happens within the FX application thread, the inner layout might clobber the outer one.

Thank you for a good discussion though!

kevinrushforth · 2025-02-07T18:03:41Z

Hmmm, to further make a point: options 2 and 3 may not be re-entrant, so they probably won't work.

Your solution is basically what I meant by option 2. The way you did it, always using the return value of a method that will return a "safe" instance to use, is reentrant. You are right that a simple approach to option 3 would not be reentrant.

I like the solution you chose.

andy-goryachev-oracle · 2025-02-07T18:09:42Z

Your solution is basically what I meant by option 2.

right. "only one thread" threw me off, sorry.

andy-goryachev-oracle · 2025-02-07T18:12:37Z

/integrate

openjdk · 2025-02-07T18:13:43Z

Going to push as commit 2cf9779.
Since your change was applied there has been 1 commit pushed to the master branch:

5cdbae2: 8333275: ComboBox: adding an item from editor changes editor text

Your commit was automatically rebased without conflicts.

openjdk · 2025-02-07T18:13:51Z

@andy-goryachev-oracle Pushed as commit 2cf9779.

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

andy-goryachev-oracle added 8 commits January 29, 2025 15:52

initial test

40857f5

more tests

4539e56

all controls

aba8476

thread count

e9259a7

pagination

02f3272

cleanup

8786bab

Merge branch 'master' into 8347392.thread.safe.utils

f6dd740

8347392: Thread-unsafe implementation of c.s.j.scene.control.skin.Utils

5a219f9

andy-goryachev-oracle changed the title ~~8347392.thread.safe.utils~~ 8347392: Thread-unsafe implementation of c.s.j.scene.control.skin.Utils Jan 30, 2025

andy-goryachev-oracle added 7 commits January 31, 2025 12:50

review comments

d1356dc

fast loop

e0b3033

better name

3fde513

jiggler

a05d391

skip tests

e25f6f6

Merge remote-tracking branch 'origin/master' into 8348423.node.thread…

597e39d

….safety

Merge branch '8348423.node.thread.safety' into 8347392.thread.safe.utils

0c45ab9

andy-goryachev-oracle added 7 commits February 3, 2025 11:35

disabled titled pane test

0bf20d7

more jitter

30700a6

Merge branch '8348423.node.thread.safety' into 8347392.thread.safe.utils

ea306ab

Merge remote-tracking branch 'origin/master' into 8348423.node.thread…

42b7d8b

….safety

review comments

0078aa1

Merge branch '8348423.node.thread.safety' into 8347392.thread.safe.utils

e1d0908

Merge remote-tracking branch 'origin/master' into 8347392.thread.safe…

203f8de

….utils

andy-goryachev-oracle marked this pull request as ready for review February 6, 2025 00:04

openjdk bot added the rfr Ready for review label Feb 6, 2025

kevinrushforth self-requested a review February 6, 2025 19:44

kevinrushforth approved these changes Feb 7, 2025

View reviewed changes

review comments

1e9cb5d

andy-goryachev-oracle requested a review from kevinrushforth February 7, 2025 16:29

kevinrushforth approved these changes Feb 7, 2025

View reviewed changes

mstr2 approved these changes Feb 7, 2025

View reviewed changes

openjdk bot added the ready Ready to be integrated label Feb 7, 2025

openjdk bot added the integrated Pull request has been integrated label Feb 7, 2025

openjdk bot closed this Feb 7, 2025

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Feb 7, 2025

andy-goryachev-oracle deleted the 8347392.thread.safe.utils branch February 7, 2025 18:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8347392: Thread-unsafe implementation of c.s.j.scene.control.skin.Utils #1691

8347392: Thread-unsafe implementation of c.s.j.scene.control.skin.Utils #1691

andy-goryachev-oracle commented Jan 30, 2025 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 30, 2025

openjdk bot commented Jan 30, 2025 •

edited

Loading

openjdk bot commented Feb 3, 2025

openjdk bot commented Feb 6, 2025

mlbridge bot commented Feb 6, 2025 •

edited

Loading

andy-goryachev-oracle commented Feb 6, 2025

kevinrushforth left a comment

kevinrushforth Feb 6, 2025

kevinrushforth Feb 6, 2025

andy-goryachev-oracle Feb 7, 2025

kevinrushforth Feb 6, 2025

mstr2 commented Feb 7, 2025

andy-goryachev-oracle commented Feb 7, 2025

kevinrushforth commented Feb 7, 2025

andy-goryachev-oracle commented Feb 7, 2025

kevinrushforth commented Feb 7, 2025

andy-goryachev-oracle commented Feb 7, 2025

andy-goryachev-oracle commented Feb 7, 2025

openjdk bot commented Feb 7, 2025

openjdk bot commented Feb 7, 2025

8347392: Thread-unsafe implementation of c.s.j.scene.control.skin.Utils #1691

8347392: Thread-unsafe implementation of c.s.j.scene.control.skin.Utils #1691

Conversation

andy-goryachev-oracle commented Jan 30, 2025 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jan 30, 2025

openjdk bot commented Jan 30, 2025 • edited Loading

openjdk bot commented Feb 3, 2025

openjdk bot commented Feb 6, 2025

mlbridge bot commented Feb 6, 2025 • edited Loading

Webrevs

andy-goryachev-oracle commented Feb 6, 2025

kevinrushforth left a comment

Choose a reason for hiding this comment

kevinrushforth Feb 6, 2025

Choose a reason for hiding this comment

kevinrushforth Feb 6, 2025

Choose a reason for hiding this comment

andy-goryachev-oracle Feb 7, 2025

Choose a reason for hiding this comment

kevinrushforth Feb 6, 2025

Choose a reason for hiding this comment

mstr2 commented Feb 7, 2025

andy-goryachev-oracle commented Feb 7, 2025

kevinrushforth commented Feb 7, 2025

andy-goryachev-oracle commented Feb 7, 2025

kevinrushforth commented Feb 7, 2025

andy-goryachev-oracle commented Feb 7, 2025

andy-goryachev-oracle commented Feb 7, 2025

openjdk bot commented Feb 7, 2025

openjdk bot commented Feb 7, 2025

andy-goryachev-oracle commented Jan 30, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Jan 30, 2025 •

edited

Loading

mlbridge bot commented Feb 6, 2025 •

edited

Loading