Skip to content

Commit

Permalink
fixed potential FX UI thread violations and simplified autonotification
Browse files Browse the repository at this point in the history 
changed auto-notification interface to AtomicBoolean and removed
superfluous setAutoNotification(boolean) interface to check for lock
race-conditions

added FX UI thread guards (didn't find anything this time, but just to
be safe for the future)

notably added additional writeLockGuard around auto-notification guard
-> should fix issue #40
  • Loading branch information
@RalphSteinhagen @wirew0rm
RalphSteinhagen authored and wirew0rm committed Nov 8, 2019
1 parent f7ee3b3 commit b26610e
Show file tree
Hide file tree
Showing 23 changed files with 1,458 additions and 1,444 deletions.
2,503 changes: 1,256 additions & 1,247 deletions chartfx-chart/src/main/java/de/gsi/chart/Chart.java
View file

Large diffs are not rendered by default.

23 changes: 10 additions & 13 deletions chartfx-chart/src/main/java/de/gsi/chart/XYChart.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import de.gsi.chart.renderer.spi.GridRenderer;
import de.gsi.chart.renderer.spi.LabelledMarkerRenderer;
import de.gsi.chart.ui.geometry.Side;
import de.gsi.chart.utils.FXUtils;
import de.gsi.dataset.DataSet;
import de.gsi.dataset.DataSet3D;
import de.gsi.dataset.utils.AssertUtils;
Expand Down Expand Up @@ -248,10 +249,9 @@ public void updateAxisRange() {
// check that all registered data sets have proper ranges defined
dataSets.parallelStream().forEach(dataset -> dataset.getAxisDescriptions().parallelStream()
.filter(axisD -> !axisD.isDefined()).forEach(axisDescription -> {
final boolean oldAutoState = dataset.isAutoNotification();
dataset.setAutoNotifaction(false);
dataset.recomputeLimits(dataset.getAxisDescriptions().indexOf(axisDescription));
dataset.setAutoNotifaction(oldAutoState);
dataset.lock().writeLockGuard(() -> {
dataset.recomputeLimits(dataset.getAxisDescriptions().indexOf(axisDescription));
});
}));

// N.B. possible race condition on this line -> for the future to solve
Expand All @@ -262,10 +262,8 @@ public void updateAxisRange() {
// dataset.lock().writeLock();
// dataset.getAxisDescriptions().parallelStream().filter(axisD -> !axisD.isDefined())
// .forEach(axisDescription -> {
// final boolean oldAutoState = dataset.isAutoNotification();
// dataset.setAutoNotifaction(false);
// dataset.recomputeLimits(dataset.getAxisDescriptions().indexOf(axisDescription));
// dataset.setAutoNotifaction(oldAutoState);
// });
// DefaultDataSetLock<DataSet> myLock = (DefaultDataSetLock<DataSet>) dataset.lock();
// myLock.downGradeWriteLock();
Expand Down Expand Up @@ -381,7 +379,7 @@ protected void redrawCanvas() {
LOGGER.debug(" xychart redrawCanvas() - pre");
}
setAutoNotifaction(false);

FXUtils.assertJavaFxThread();
final long now = System.nanoTime();
final double diffMillisSinceLastUpdate = TimeUnit.NANOSECONDS.toMillis(now - lastCanvasUpdate);
if (diffMillisSinceLastUpdate < XYChart.BURST_LIMIT_MS) {
Expand Down Expand Up @@ -431,11 +429,10 @@ protected static void updateNumericAxis(final Axis axis, final List<DataSet> dat
if (dataSets == null || dataSets.isEmpty()) {
return;
}
final boolean oldFlag = axis.isAutoNotification();
final boolean oldAutoState = axis.autoNotification().getAndSet(false);
final double oldMin = axis.getMin();
final double oldMax = axis.getMax();
final double oldLength = axis.getLength();
axis.setAutoNotifaction(false);

// TODO: add new auto-ranging here
final boolean isHorizontal = axis.getSide().isHorizontal();
Expand All @@ -458,12 +455,12 @@ protected static void updateNumericAxis(final Axis axis, final List<DataSet> dat
if (oldMin != axis.getMin() || oldMax != axis.getMax() || oldLength != axis.getLength()) {
axis.forceRedraw();
}
axis.setAutoNotifaction(oldFlag);
axis.autoNotification().set(oldAutoState);
return;
}

final List<Number> dataMinMax = new ArrayList<>();
dataSets.forEach(dataset -> {
dataSets.forEach(dataset -> dataset.lock().readLockGuardOptimistic(() -> {
// for (AxisDescription axisDescription : dataset.getAxisDescriptions()) {
// if (!axisDescription.isDefined()) {
// dataset.recomputeLimits(dataset.getAxisDescriptions().indexOf(axisDescription));
Expand All @@ -477,7 +474,7 @@ protected static void updateNumericAxis(final Axis axis, final List<DataSet> dat
dataMinMax.add(dataset.getAxisDescription(isHorizontal ? 0 : 1).getMin());
dataMinMax.add(dataset.getAxisDescription(isHorizontal ? 0 : 1).getMax());
}
});
}));

if (axis.isAutoGrowRanging()) {
dataMinMax.add(axis.getMin());
Expand All @@ -499,7 +496,7 @@ protected static void updateNumericAxis(final Axis axis, final List<DataSet> dat
if (oldMin != axis.getMin() || oldMax != axis.getMax() || oldLength != axis.getLength()) {
axis.forceRedraw();
}
axis.setAutoNotifaction(oldFlag);
axis.autoNotification().set(oldAutoState);
}

}
20 changes: 11 additions & 9 deletions chartfx-chart/src/main/java/de/gsi/chart/axes/spi/AbstractAxis.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ public abstract class AbstractAxis extends AbstractAxisParameter implements Axis
private final Canvas canvas = new ResizableCanvas();
protected boolean labelOverlap;
protected double cachedOffset; // for caching
protected final ReentrantLock lock = new ReentrantLock();
protected final transient ReentrantLock lock = new ReentrantLock();
protected double maxLabelHeight;
protected double maxLabelWidth;

Expand Down Expand Up @@ -258,8 +258,10 @@ public void drawAxis(final GraphicsContext gc, final double axisWidth, final dou
*/
@Override
public void fireInvalidated() {
if (!isAutoNotification() || updateEventListener().isEmpty()) {
return;
synchronized (autoNotification()) {
if (!autoNotification().get() || updateEventListener().isEmpty()) {
return;
}
}

if (Platform.isFxApplicationThread()) {
Expand Down Expand Up @@ -387,14 +389,13 @@ public void invalidateRange(final List<Number> data) {
autoRange.add(dataValue.doubleValue());
}

final boolean oldState = isAutoNotification();
setAutoNotifaction(false);
final boolean oldState = autoNotification().getAndSet(false);
final boolean change = set(dataMinValue, dataMaxValue);
if (change) {
data.clear();
autoRange.setAxisLength(getLength() == 0 ? 1 : getLength(), getSide());
}
setAutoNotifaction(oldState);
autoNotification().set(oldState);
invalidateRange();
invokeListener(new AxisChangeEvent(this));
}
Expand Down Expand Up @@ -472,7 +473,8 @@ public boolean setMax(final double value) {
* minimum space between labels
* @return true if labels overlap
*/
private static boolean isTickLabelsOverlap(final Side side, final TickMark m1, final TickMark m2, final double gap) {
private static boolean isTickLabelsOverlap(final Side side, final TickMark m1, final TickMark m2,
final double gap) {
if (!m1.isVisible() || !m2.isVisible()) {
return false;
}
Expand Down Expand Up @@ -1162,8 +1164,8 @@ protected void drawTickLabels(final GraphicsContext gc, final double axisWidth,
gc.restore();
}

protected static void drawTickMarkLabel(final GraphicsContext gc, final double x, final double y, final double rotation,
final TickMark tickMark) {
protected static void drawTickMarkLabel(final GraphicsContext gc, final double x, final double y,
final double rotation, final TickMark tickMark) {
gc.save();
gc.setFont(tickMark.getFont());
gc.setFill(tickMark.getFill());
Expand Down
42 changes: 19 additions & 23 deletions chartfx-chart/src/main/java/de/gsi/chart/axes/spi/AbstractAxisParameter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,8 @@ public abstract class AbstractAxisParameter extends Pane implements Axis {
/** pseudo-class indicating this is a vertical centre Axis. */
private static final PseudoClass CENTRE_VER_PSEUDOCLASS_STATE = PseudoClass.getPseudoClass("verCentre");

private final AtomicBoolean autoNotification = new AtomicBoolean();
private final List<EventListener> updateListeners = new LinkedList<>();
private final transient AtomicBoolean autoNotification = new AtomicBoolean(true);
private final List<EventListener> updateListeners = Collections.synchronizedList(new LinkedList<>());
/**
* Paths used for css-type styling. Not used for actual drawing. Used as a storage contained for the settings
* applied to GraphicsContext which allow much faster (and less complex) drawing routines but do no not allow
Expand Down Expand Up @@ -673,9 +673,11 @@ public AtomicBoolean autoNotification() {
*/
@Override
public void invokeListener(final UpdateEvent updateEvent, final boolean executeParallel) {
if (!isAutoNotification()) {
//avoids duplicate update events
return;
synchronized (autoNotification()) {
if (!autoNotification().get()) {
//avoids duplicate update events
return;
}
}
requestAxisLayout();
Axis.super.invokeListener(updateEvent, executeParallel);
Expand Down Expand Up @@ -1219,12 +1221,11 @@ protected void setScale(final double scale) {
@Override
public boolean add(final double[] values, final int nlength) {
boolean changed = false;
final boolean oldState = isAutoNotification();
setAutoNotifaction(false);
final boolean oldState = autoNotification().getAndSet(false);
for (int i = 0; i < nlength; i++) {
changed |= add(values[i]);
}
setAutoNotifaction(oldState);
autoNotification().set(oldState);
invokeListener(new AxisChangeEvent(this));
return changed;
}
Expand All @@ -1235,8 +1236,7 @@ public boolean add(final double value) {
return false;
}
boolean changed = false;
final boolean oldState = isAutoNotification();
setAutoNotifaction(false);
final boolean oldState = autoNotification().getAndSet(false);
if (value > this.getMax()) {
this.setMax(value);
changed = true;
Expand All @@ -1245,7 +1245,7 @@ public boolean add(final double value) {
this.setMin(value);
changed = true;
}
setAutoNotifaction(oldState);
autoNotification().set(oldState);
if (changed) {
invokeListener(new AxisChangeEvent(this));
}
Expand All @@ -1254,11 +1254,10 @@ public boolean add(final double value) {

@Override
public boolean clear() {
final boolean oldState = isAutoNotification();
setAutoNotifaction(false);
final boolean oldState = autoNotification().getAndSet(false);
minProp.set(DEFAULT_MIN_RANGE);
maxProp.set(DEFAULT_MAX_RANGE);
setAutoNotifaction(oldState);
autoNotification().set(oldState);
invokeListener(new AxisChangeEvent(this));
return false;
}
Expand All @@ -1277,11 +1276,10 @@ public boolean isDefined() {
public boolean set(final double min, final double max) {
final double oldMin = minProp.get();
final double oldMax = maxProp.get();
final boolean oldState = isAutoNotification();
setAutoNotifaction(false);
final boolean oldState = autoNotification().getAndSet(false);
minProp.set(min);
maxProp.set(max);
setAutoNotifaction(oldState);
autoNotification().set(oldState);
final boolean changed = (oldMin != min) || (oldMax != max);
if (changed) {
invokeListener(new AxisChangeEvent(this));
Expand All @@ -1296,8 +1294,7 @@ private static boolean equalString(final String str1, final String str2) {
@Override
public boolean set(final String axisName, final String... axisUnit) {
boolean changed = false;
final boolean oldState = isAutoNotification();
setAutoNotifaction(false);
final boolean oldState = autoNotification().getAndSet(false);
if (!equalString(axisName, getName())) {
setName(axisName);
changed = true;
Expand All @@ -1306,7 +1303,7 @@ public boolean set(final String axisName, final String... axisUnit) {
setName(axisUnit[0]);
changed = true;
}
setAutoNotifaction(oldState);
autoNotification().set(oldState);
if (changed) {
invokeListener(new AxisChangeEvent(this));
}
Expand All @@ -1316,11 +1313,10 @@ public boolean set(final String axisName, final String... axisUnit) {
@Override
public boolean set(final String axisName, final String axisUnit, final double rangeMin, final double rangeMax) {
boolean changed = false;
final boolean oldState = isAutoNotification();
setAutoNotifaction(false);
final boolean oldState = autoNotification().getAndSet(false);
changed |= this.set(axisName, axisUnit);
changed |= this.set(rangeMin, rangeMax);
setAutoNotifaction(oldState);
autoNotification().set(oldState);
if (changed) {
invokeListener(new AxisChangeEvent(this));
}
Expand Down
30 changes: 13 additions & 17 deletions chartfx-chart/src/main/java/de/gsi/chart/plugins/measurements/ValueIndicator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import de.gsi.chart.plugins.AbstractSingleValueIndicator;
import de.gsi.chart.plugins.XValueIndicator;
import de.gsi.chart.plugins.YValueIndicator;
import de.gsi.chart.utils.FXUtils;
import de.gsi.dataset.DataSet;
import de.gsi.dataset.event.UpdateEvent;
import javafx.application.Platform;
Expand Down Expand Up @@ -88,21 +89,15 @@ public void initialize() {

sliderIndicator1.valueProperty().addListener((ch, oldValue, newValue) -> {
if (oldValue != newValue) {
if (Platform.isFxApplicationThread()) {
handle(null);
} else {
Platform.runLater(() -> handle(null));
}
FXUtils.runFX(() -> handle(null));
}
});
// chartPane.addListener(e -> invalidated(null));
super.showConfigDialogue();
if (Platform.isFxApplicationThread()) {
handle(null);
} else {
Platform.runLater(() -> handle(null));
}
chart.requestLayout();
FXUtils.runFX(() -> {
handle(null);
chart.requestLayout();
});
}

@Override
Expand All @@ -125,15 +120,16 @@ protected void removeAction() {

@Override
public void handle(final UpdateEvent observable) {
if (Platform.isFxApplicationThread()) {
Platform.runLater(() -> handle(observable));
return;
}
if (!Platform.isFxApplicationThread()) {
// not running from FX application thread restart via runLater...
FXUtils.runFX(() -> handle(observable));
return;
}
final DataSet selectedDataSet = getDataSet();

final double newValue = sliderIndicator1.getValue();
final int index = selectedDataSet.getIndex(axisMode == X ? DataSet.DIM_X : DataSet.DIM_Y, newValue);
final double val = selectedDataSet.get(axisMode == X ? DataSet.DIM_Y : DataSet.DIM_X,index);
final double val = selectedDataSet.get(axisMode == X ? DataSet.DIM_Y : DataSet.DIM_X, index);
final Axis axis = axisMode == X ? chart.getYAxis() : chart.getXAxis();

// update label unitTextField
Expand Down
5 changes: 0 additions & 5 deletions chartfx-chart/src/main/java/de/gsi/chart/renderer/spi/MountainRangeRenderer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -312,11 +312,6 @@ public DataSet recomputeLimits(int dimension) {
return this;
}

@Override
public void setAutoNotifaction(final boolean flag) {
autoNotification.set(flag);
}

@Override
public DataSet setStyle(final String style) {
return dataSet.setStyle(style);
Expand Down
6 changes: 6 additions & 0 deletions chartfx-chart/src/main/java/de/gsi/chart/utils/FXUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,12 @@ private FXUtils() {
private static class ThrowableWrapper {
Throwable t;
}

public static void assertJavaFxThread() {
if (!Platform.isFxApplicationThread()) {
throw new IllegalStateException("access JavaFX from non-JavaFX thread - please fix");
}
}

/**
* If you run into any situation where all of your scenes end, the thread managing all of this will just peter out.
Expand Down
Loading

0 comments on commit b26610e

Please sign in to comment.