Skip to content

Commit

Permalink
Reentrancy warnings in validateBounds(), see phetsims/joist#725
Browse files Browse the repository at this point in the history
  • Loading branch information
jonathanolson committed Jul 10, 2021
1 parent f0adace commit 7656bb0
Showing 1 changed file with 16 additions and 4 deletions.
20 changes: 16 additions & 4 deletions js/nodes/Node.js
Original file line number Diff line number Diff line change
Expand Up @@ -1248,9 +1248,13 @@ class Node extends PhetioObject {
if ( !ourChildBounds.equals( oldChildBounds ) ) {
// notifies only on an actual change
if ( !ourChildBounds.equalsEpsilon( oldChildBounds, notificationThreshold ) ) {
this.childBoundsProperty.notifyListeners( oldChildBounds );
this.childBoundsProperty.notifyListeners( oldChildBounds ); // RE-ENTRANT CALL HERE, it will validateBounds()
}
}

// WARNING: Think twice before adding code here below the listener notification. The notifyListeners() call can
// trigger re-entrancy, so this function needs to work when that happens. DO NOT set things based on local
// variables here.
}

if ( this._localBoundsDirty && !this._localBoundsOverridden ) {
Expand Down Expand Up @@ -1283,9 +1287,13 @@ class Node extends PhetioObject {
this._boundsDirty = true;

if ( !ourLocalBounds.equalsEpsilon( oldLocalBounds, notificationThreshold ) ) {
this.localBoundsProperty.notifyListeners( oldLocalBounds );
this.localBoundsProperty.notifyListeners( oldLocalBounds ); // RE-ENTRANT CALL HERE, it will validateBounds()
}
}

// WARNING: Think twice before adding code here below the listener notification. The notifyListeners() call can
// trigger re-entrancy, so this function needs to work when that happens. DO NOT set things based on local
// variables here.
}

// TODO: layout here?
Expand Down Expand Up @@ -1332,9 +1340,13 @@ class Node extends PhetioObject {

// TODO: consider changing to parameter object (that may be a problem for the GC overhead)
if ( !ourBounds.equalsEpsilon( oldBounds, notificationThreshold ) ) {
this.boundsProperty.notifyListeners( oldBounds );
this.boundsProperty.notifyListeners( oldBounds ); // RE-ENTRANT CALL HERE, it will validateBounds()
}
}

// WARNING: Think twice before adding code here below the listener notification. The notifyListeners() call can
// trigger re-entrancy, so this function needs to work when that happens. DO NOT set things based on local
// variables here.
}

// if there were side-effects, run the validation again until we are clean
Expand All @@ -1343,7 +1355,7 @@ class Node extends PhetioObject {

// TODO: if there are side-effects in listeners, this could overflow the stack. we should report an error
// instead of locking up
this.validateBounds();
this.validateBounds(); // RE-ENTRANT CALL HERE, it will validateBounds()
}

if ( assert ) {
Expand Down

0 comments on commit 7656bb0

Please sign in to comment.