RFR: 8349091: Charts: exception initializing in a background thread [v6]

[Kevin Rushforth]

On Tue, 25 Feb 2025 22:33:40 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> Root Cause:
>> (Multiple) properties are getting bound to the global `Platform.accessibilityActive` property. Binding (and I say, accessing) of properties is not thread-safe.
>> 
>> I also changed the design a bit.  Originally, every symbol in a chart had its `focusTraversableProperty` bound to `Platform.accessibilityActive` in order to enable the accessibility navigation across the chart data points.  This is rather inefficient, as the property has to manage (thousands?) of listeners.
>> 
>> Instead, a single boolean property is added to each chart, with a listener added to it which iterates over data symbols to toggle the `focusTraversableProperty` directly.
>> 
>> The exact moment when the new property gets bound is also important, and has to happen in the FX application thread.
>> 
>> With this change, it is safe to create and populate charts with data in a background thread.
>> 
>> ---
>> 
>> ## NOTES
>> 
>> 1. It looks like the `Platform.accessibilityActive` property never transitions back to false  after it transitioned to true.  Some say it is because there is no mechanism in the platform to get notified which cannot possibly be true.
>> 2. The javadoc for `Platform.accessibilityActiveProperty()` method stipulates that "_This method may be called from any thread_" which is patently not true as the current implementation is simply not thread-safe.
>> 
>> ## Note to the Reviewers
>> 
>> To avoid merge conflicts, the preferred order of integrations:
>> 
>> #1697 
>> #1713 
>> #1717
>
> Andy Goryachev has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 25 commits:
> 
>  - Merge remote-tracking branch 'origin/master' into 8349091.charts.thread.safety
>  - review comments
>  - Merge remote-tracking branch 'origin/master' into 8349091.charts.thread.safety
>  - Merge remote-tracking branch 'origin/master' into 8349091.charts.thread.safety
>  - enabled pie chart test
>  - Merge branch 'master' into 8349091.charts.thread.safety
>  - Merge branch 'master' into 8349091.charts.thread.safety
>  - whitespace
>  - Merge remote-tracking branch 'origin/master' into 8349091.charts.thread.safety
>  - cleanup
>  - ... and 15 more: https://git.openjdk.org/jfx/compare/7a7854c9...4288d1d0

What you have now works in all cases I've tried. I left a couple suggestions and will reapprove if you decide to make changes.

modules/javafx.controls/src/main/java/javafx/scene/chart/Chart.java line 106:

> 104: 
> 105:     // SimpleBooleanProperty or ObjectBinding
> 106:     private volatile Object accessibilityActive;

You can use `ObservableValue<?>` instead of `Object` as the type. Alternatively, use two fields, a `SimpleBooleanProperty` for use by the FX app thread and an ObjectBinding for use by a background thread. They wouldn't need to be volatile in that case. What you have is OK, but using two properties might simplify the logic a bit.

modules/javafx.controls/src/main/java/javafx/scene/chart/Chart.java line 561:

> 559:                 accessibilityActive = winProp; // keep the reference so it won't get gc
> 560: 
> 561:                 // lambda cannot be used in place of a ChangeListener in removeListener()

Why not use a Subscription then? It seems tailor-made for what you are trying to do.

-------------

Marked as reviewed by kcr (Lead).

PR Review: https://git.openjdk.org/jfx/pull/1697#pullrequestreview-2644733749
PR Review Comment: https://git.openjdk.org/jfx/pull/1697#discussion_r1971704069
PR Review Comment: https://git.openjdk.org/jfx/pull/1697#discussion_r1971713667