8349091: Charts: exception initializing in a background thread #1697
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| /* | ||
| * Copyright (c) 2010, 2025, Oracle and/or its affiliates. All rights reserved. | ||
| * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. | ||
| * | ||
| * This code is free software; you can redistribute it and/or modify it | ||
| @@ -25,24 +25,33 @@ | ||
|
|
||
| package javafx.scene.chart; | ||
|
|
||
| import java.util.ArrayList; | ||
| import java.util.Collections; | ||
| import java.util.HashMap; | ||
| import java.util.Iterator; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import javafx.animation.FadeTransition; | ||
| import javafx.animation.Interpolator; | ||
| import javafx.animation.KeyFrame; | ||
| import javafx.animation.KeyValue; | ||
| import javafx.animation.Timeline; | ||
| import javafx.beans.NamedArg; | ||
| import javafx.beans.property.BooleanProperty; | ||
| import javafx.beans.property.DoubleProperty; | ||
| import javafx.beans.property.SimpleDoubleProperty; | ||
| import javafx.collections.FXCollections; | ||
| import javafx.collections.ListChangeListener; | ||
| import javafx.collections.ObservableList; | ||
| import javafx.css.CssMetaData; | ||
| import javafx.css.Styleable; | ||
| import javafx.css.StyleableBooleanProperty; | ||
| import javafx.css.StyleableProperty; | ||
| import javafx.css.converter.BooleanConverter; | ||
| import javafx.scene.AccessibleRole; | ||
| import javafx.scene.Group; | ||
| import javafx.scene.Node; | ||
| import javafx.scene.chart.LineChart.SortingPolicy; | ||
| import javafx.scene.layout.StackPane; | ||
| import javafx.scene.shape.ClosePath; | ||
| import javafx.scene.shape.LineTo; | ||
| @@ -51,14 +60,7 @@ | ||
| import javafx.scene.shape.PathElement; | ||
| import javafx.scene.shape.StrokeLineJoin; | ||
| import javafx.util.Duration; | ||
| import com.sun.javafx.charts.Legend.LegendItem; | ||
|
|
||
| /** | ||
| * AreaChart - Plots the area between the line that connects the data points and | ||
| @@ -541,7 +543,7 @@ private Node createSymbol(Series<X,Y> series, int seriesIndex, final Data<X,Y> i | ||
| symbol = new StackPane(); | ||
| symbol.setAccessibleRole(AccessibleRole.TEXT); | ||
| symbol.setAccessibleRoleDescription("Point"); | ||
| symbol.setFocusTraversable(isAccessibilityActive()); | ||
|
Comment on lines
-544
to
+546
There was a problem hiding this comment. So, if I understand correctly, we can't directly do this Because on a background thread there may not yet be an initialized There was a problem hiding this comment. The reason we The other reason is https://bugs.openjdk.org/browse/JDK-8351067 - the There was a problem hiding this comment. Was this solution considered: What the above code does is create a binding on the There was a problem hiding this comment. The above does require though that There was a problem hiding this comment. unrelated, but I would rather disallow background access to any platform properties. per the earlier email, this might (read: will) create concurrent access when the node is not yet attached to the scene graph: step 3 is when concurrent access occurs. There was a problem hiding this comment. Yes, but that's because there is no synchronization. Here is a version that does do synchronization; I see no more exceptions (and it runs just as fast really): public class PropTest {
public static void main(String[] args) {
Thread mainThread = Thread.currentThread();
final var prop = new SimpleBooleanProperty(false) {
@Override
public synchronized void addListener(ChangeListener<? super Boolean> listener) {
super.addListener(listener);
}
@Override
public synchronized void set(boolean newValue) {
super.set(newValue);
}
};
// add listeners in a background thread
var thr = new Thread(() -> {
for (int i = 0; i < 1000000; i++) {
ChangeListener<Boolean> listener = (obs, o, n) -> {
if (!mainThread.equals(Thread.currentThread())) {
System.err.println("***** ERROR: listener called on wrong thread: " + Thread.currentThread());
}
};
prop.addListener(listener);
}
});
thr.start();
// Fire events in main thread
for (int jj = 0; jj < 1000000; jj++) {
prop.set(!prop.get());
}
}
}There was a problem hiding this comment. Of course for a generic solution, we could provide a wrapper for properties, or custom synchronized properties. There was a problem hiding this comment. Yes, using a custom property, or a wrapper, would solve this particular synchronization problem, although that wouldn't solve all of the problems. We would then be left with the problem that if the accessibility change listener did fire -- which necessarily happens on the FX app thread -- while the object was being set up on a background thread, the listener might try to modify properties that are being touched on the background thread. This is just another case of a more general problem we already have with animation, and which Andy fixed by not starting animation in a few places unless and until we are on the FX app thread. So I think the solution Andy has chosen of deferring adding the listener is better than trying to fix the accessibility property, followed by fixing the listeners that use it, to be thread-safe. There was a problem hiding this comment. Thank you, @kevinrushforth . Getting back to the rule of allowing Node construction from a background thread but not "use", should we consider enforcing the thread access rules for global and static objects (like Platform properties)? In other words, constructing objects that might modify internal properties is ok, but trying to access shared objects is not? (maybe it's time to move the discussion out of this PR into the mailing list perhaps?) There was a problem hiding this comment. Enforcing the threading restriction when using properties is a larger conversation. If we want to have it, I agree it should be on the mailing list. |
||
| item.setNode(symbol); | ||
| } | ||
| // set symbol styles | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| /* | ||
| * Copyright (c) 2010, 2025, Oracle and/or its affiliates. All rights reserved. | ||
| * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. | ||
| * | ||
| * This code is free software; you can redistribute it and/or modify it | ||
| @@ -28,9 +28,6 @@ | ||
| import java.util.ArrayList; | ||
| import java.util.Collections; | ||
| import java.util.List; | ||
| import javafx.animation.Animation; | ||
| import javafx.animation.KeyFrame; | ||
| import javafx.application.Platform; | ||
| @@ -40,28 +37,29 @@ | ||
| import javafx.beans.property.SimpleBooleanProperty; | ||
| import javafx.beans.property.StringProperty; | ||
| import javafx.beans.property.StringPropertyBase; | ||
| import javafx.beans.value.ChangeListener; | ||
| import javafx.beans.value.ObservableValue; | ||
| import javafx.beans.value.WritableValue; | ||
| import javafx.collections.ObservableList; | ||
| import javafx.css.CssMetaData; | ||
| import javafx.css.Styleable; | ||
| import javafx.css.StyleableBooleanProperty; | ||
| import javafx.css.StyleableObjectProperty; | ||
| import javafx.css.StyleableProperty; | ||
| import javafx.css.converter.BooleanConverter; | ||
| import javafx.css.converter.EnumConverter; | ||
| import javafx.geometry.Pos; | ||
| import javafx.geometry.Side; | ||
| import javafx.scene.Node; | ||
| import javafx.scene.Scene; | ||
| import javafx.scene.control.Label; | ||
| import javafx.scene.layout.Pane; | ||
| import javafx.scene.layout.Region; | ||
| import javafx.stage.Window; | ||
| import com.sun.javafx.charts.ChartLayoutAnimator; | ||
| import com.sun.javafx.charts.Legend; | ||
| import com.sun.javafx.scene.NodeHelper; | ||
| import com.sun.javafx.scene.control.skin.Utils; | ||
|
|
||
| /** | ||
| * Base class for all charts. It has 3 parts the title, legend and chartContent. The chart content is populated by the | ||
| @@ -104,6 +102,9 @@ public abstract class Chart extends Region { | ||
| /** Animator for animating stuff on the chart */ | ||
| private final ChartLayoutAnimator animator = new ChartLayoutAnimator(chartContent); | ||
|
|
||
| // SimpleBooleanProperty or ObjectBinding | ||
| private volatile Object accessibilityActive; | ||
|
There was a problem hiding this comment. You can use There was a problem hiding this comment. It's a design decision - I won't want to waste an extra pointer. There was a problem hiding this comment. While I still think the code would be cleaner with two properties, and the extra memory is not significant, what you have will work, so I won't object. |
||
|
|
||
| // -------------- PUBLIC PROPERTIES -------------------------------------------------------------------------------- | ||
|
|
||
| /** The chart title */ | ||
| @@ -274,7 +275,6 @@ protected ObservableList<Node> getChartChildren() { | ||
| */ | ||
| public Chart() { | ||
| titleLabel.setAlignment(Pos.CENTER); | ||
| getChildren().addAll(titleLabel, chartContent); | ||
| getStyleClass().add("chart"); | ||
| titleLabel.getStyleClass().add("chart-title"); | ||
| @@ -511,6 +511,71 @@ public StyleableProperty<Boolean> getStyleableProperty(Chart node) { | ||
| return getClassCssMetaData(); | ||
| } | ||
|
|
||
| private void handleAccessibilityActive(boolean on) { | ||
| titleLabel.setFocusTraversable(on); | ||
| updateSymbolFocusable(on); | ||
| } | ||
|
|
||
| /** | ||
| * Invoked in the FX application thread when accessibility active platform property changes. | ||
| * The child classes should override this method to set focus traversable flag on every symbol, as well as | ||
| * other elements that need to be focusable. | ||
| * @param on whether the accessibility is active | ||
| */ | ||
| // package protected: custom charts must handle accessbility on their own | ||
| void updateSymbolFocusable(boolean on) { | ||
| } | ||
|
|
||
| /** | ||
| * When called from JavaFX application thread, returns the value of the property. | ||
| * When called from any other thread, returns false and sets up the machinery to | ||
| * invoke {@code updateSymbolFocusable()} when needed. | ||
| * The chart implementations should use this method to set focus travesable flags on the nodes | ||
| * which needs to be focus traversable when accessibility is on. | ||
| * @return | ||
| */ | ||
| // package protected: custom charts must handle accessbility on their own | ||
| final boolean isAccessibilityActive() { | ||
| if (Platform.isFxApplicationThread()) { | ||
| if (accessibilityActive instanceof SimpleBooleanProperty p) { | ||
| return p.get(); | ||
| } else { | ||
| boolean aa = Platform.accessibilityActiveProperty().get(); | ||
| SimpleBooleanProperty active = new SimpleBooleanProperty(aa); | ||
| accessibilityActive = active; | ||
| active.bind(Platform.accessibilityActiveProperty()); | ||
| active.addListener((src, prev, on) -> { | ||
| handleAccessibilityActive(on); | ||
| }); | ||
| return active.get(); | ||
|
There was a problem hiding this comment. I'm confused as to what is happening here. A boolean property What confuses me is what this is supposed to achieve, and also why we're initialising the property, but then bind it anyway... There was a problem hiding this comment. aa is unnecessary, you are right, bind() will set the value. the property is created as a way to signal any subsequent calls that no more work is needed. |
||
| } | ||
| } else { | ||
| // chart and its data are allowed to be constructed in a background thread | ||
| if (accessibilityActive == null) { | ||
| // set up a listener which, once the chart gets connected to a window (always in the context of | ||
| // the fx application thread), calls isAccessibilityActive() again, | ||
| // which in turn installs the listener on the Platform.accessibilityActiveProperty. | ||
| ObservableValue<Window> winProp = sceneProperty().flatMap(Scene::windowProperty); | ||
| accessibilityActive = winProp; // keep the reference so it won't get gc | ||
|
|
||
| // lambda cannot be used in place of a ChangeListener in removeListener() | ||
|
There was a problem hiding this comment. Why not use a Subscription then? It seems tailor-made for what you are trying to do. There was a problem hiding this comment. I don't know how to use Subscription in this case. There was a problem hiding this comment. @hjohn could you help here please? How could we use Subscription in a situation when it has to be unsubscribed from within the lambda? There was a problem hiding this comment. I haven't been tracking these fixes for allowing initialization in background threads, but it seems to me that basically anything should be allowed as long as you're not part of a visible scene graph -- and I think there's also no expectation that all functionality of a control "works" as long as it is not yet part of such a graph (ie. the listeners are only needed once it is part of a scene graph). If you make listeners conditional on being part of a scene graph, then I think you can handle these with a single code path. Such a condition would be: You can then safely install any listeners directly, regardless if they're on a background thread or not: Or: Or with any mappings added, you can even pick where you put the On a background thread, There was a problem hiding this comment. Thank you! This is not what I asked though: my question was about using What you are proposing is a slightly more involved change: for example, registering and unregistering listeners each time might introduce other issues (especially since we have no way to prioritize listeners/handlers). It also complicates the management of skin (as skins can be added/removed), something I would rather avoid. There was a problem hiding this comment. As for this current code: What you want there is not possible in this way as you can't use Note that there is no need to store You can then write it as a single statement which won't be GC'd as long as the subscription is active: |
||
| ChangeListener<Window> li = new ChangeListener<>() { | ||
| @Override | ||
| public void changed(ObservableValue<? extends Window> src, Window old, Window win) { | ||
| if (win != null) { | ||
| if (accessibilityActive == winProp) { | ||
| accessibilityActive = null; | ||
| } | ||
| if (isAccessibilityActive()) { | ||
| handleAccessibilityActive(true); | ||
| } | ||
| winProp.removeListener(this); | ||
| } | ||
| } | ||
| }; | ||
| winProp.addListener(li); | ||
| } | ||
| return false; | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like you are mixing the setting up of the listeners with setting the focus traversable of this symbol to the current state of accessibilityActive, which will be
Platform.accessibilityActivePropertyfor the FX app thread) and "false" (for background thread. As a result, this method, which is called once per symbol, will set up a new listener each time it is called.You might consider refactoring this a bit to only ever set up the listeners when the object is constructed. Either that or ensure that you guard against repeatedly recreating the listener.