Reentrant listener notification in Emitter leads to buggy cases

[zepumph]

From https://github.com/phetsims/buoyancy/issues/67.

Over there we found a case in Buoyancy where changing a DynamicProperty such that it reentrantly changes itself again can lead to a listener order in which you cannot depend on the "oldValue". We were able to write unit tests that show this issue. Basically in a reentrant case where we increment a counter on each emit callback, we notify on the last change first, so the listener counts backwards instead of forwards (as you'd expect).

Taking bits and pieces from that issue, here are unit tests that demonstrate that problem:

```js QUnit.test( 'TinyEmitter listener order bugz', assert => { const emitter = new TinyEmitter<[ number ]>(); let count = 1; emitter.addListener( number => { if ( number < 10 ) { emitter.emit( number + 1 ); } } ); emitter.addListener( number => { console.log( number ); assert.ok( number === count++, `should go in order of emitting: ${number}` ); } ); emitter.emit( count ); } ); QUnit.test( 'TinyProperty notify in value-change order', assert => { let count = 2; // starts as a value of 1, so 2 is the first value we change to. const myProperty = new TinyProperty( 1 ); myProperty.lazyLink( value => { if ( value < 10 ) { myProperty.value = value + 1; } } ); myProperty.lazyLink( ( value, oldValue ) => { console.log( `asserts ${oldValue} => ${value}` ); assert.ok( value === oldValue + 1, `increment each time: ${oldValue} -> ${value}` ); assert.ok( value === count++, `increment in order expected: ${count - 2}->${count - 1}, received: ${oldValue} -> ${value}` ); } ); myProperty.value = count; // We hope: // 1->2 // 2->3 // 3->4 // ... // 8->9 // We think since it is buggy we will instead get: // We hope: // 8->9 // 7->8 // 6->7 // ... // 1->2 } ); ```

The patch in https://github.com/phetsims/buoyancy/issues/67#issuecomment-1738104059 has a first working prototype of updating TinyEmitter to work as a "bread first" notification strategy instead of what we currently have (depth first):

```diff Subject: [PATCH] d --- Index: js/TinyEmitter.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/TinyEmitter.ts b/js/TinyEmitter.ts --- a/js/TinyEmitter.ts (revision 91c1bcb9bddc352f1b8fdebda8a3d2f70c4796f1) +++ b/js/TinyEmitter.ts (date 1707340496087) @@ -31,6 +31,7 @@ type EmitContext = { index: number; listenerArray?: TEmitterListener[]; + args: T; }; // Store the number of listeners from the single TinyEmitter instance that has the most listeners in the whole runtime. @@ -114,30 +115,36 @@ if ( this.listeners.size > 0 ) { const emitContext: EmitContext = { - index: 0 + index: 0, + args: args //.slice() as T // TODO: do we need to slice? https://github.com/phetsims/buoyancy/issues/67 // listenerArray: [] // {Array.|undefined} assigned if a mutation is made during emit }; this.emitContexts.push( emitContext ); + // assert && assert( this.emitContexts.length <= 1000, 'reentrant depth of 1000 seems like a bug to me!' ); - for ( const listener of this.listeners ) { - listener( ...args ); - emitContext.index++; - - // If a listener was added or removed, we cannot continue processing the mutated Set, we must switch to - // iterate over the guarded array - if ( emitContext.listenerArray ) { - break; - } - } + if ( this.emitContexts.length === 1 ) { + while ( this.emitContexts.length > 0 ) { + const emitContext = this.emitContexts[ 0 ]; + const listeners = emitContext.listenerArray || Array.from( this.listeners ); + for ( let i = 0; i < listeners.length; i++ ) { + listeners[ i ]( ...emitContext.args ); + emitContext.index++; + } - // If the listeners were guarded during emit, we bailed out on the for..of and continue iterating over the original - // listeners in order from where we left off. - if ( emitContext.listenerArray ) { - for ( let i = emitContext.index; i < emitContext.listenerArray.length; i++ ) { - emitContext.listenerArray[ i ]( ...args ); + // If the listeners were guarded during emit, we bailed out on the for..of and continue iterating over the original + // listeners in order from where we left off. + if ( emitContext.listenerArray ) { + for ( let i = emitContext.index; i < emitContext.listenerArray.length; i++ ) { + emitContext.listenerArray[ i ]( ...args ); + } + } + this.emitContexts.shift(); } } - this.emitContexts.pop(); + else { + // TODO: delete or assert out? "too many levels deep, it must be an infinite loop" 1000 worked well in testing, https://github.com/phetsims/buoyancy/issues/67 + console.log( 'this.emitContexts.length', this.emitContexts.length ); + } } }

[zepumph]

This issue is primarily to determine if it is worth fixing how TinyEmitter notifies across the project. When you apply the above patch, changing TinyEmitter to "breadth-first" notifying, then we immediately ran into two infinite loops. We should investigate these to determine how much energy it may be to tackle this generally in axon. Remember to apply the above TinyEmitter patch before reproducing the below:

Problem 1 - "stringTest=dynamic" infinite loop (originally reported in https://github.com/phetsims/buoyancy/issues/67#issuecomment-1736350681)

1. Launch buoyancy with ?stringTest=dynamic
2. Use arrow keys and on first change, an infinite loop occurs

Problem 2 - "maxWidth" infinite loop (https://github.com/phetsims/buoyancy/issues/67#issuecomment-1738104059)

1. Apply this patch
2. Load buoyancy
3. In preferences, switch the volume units. This will cause an infinite loop from the valueText being layout-thrashed (new word) in NumberDisplay.

[zepumph]

Ok, I cleaned up the patch in the first comment and added doc and TODOs. This will be a good starting point for our conversation.

```diff Subject: [PATCH] FIFO listener order calling, https://github.com/phetsims/axon/issues/447 --- Index: js/TinyEmitterTests.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/TinyEmitterTests.ts b/js/TinyEmitterTests.ts --- a/js/TinyEmitterTests.ts (revision 347828227d4ace993a8d2c759423c968bce4a893) +++ b/js/TinyEmitterTests.ts (date 1707510199559) @@ -204,4 +204,40 @@ // Check these values when running with ?listenerOrder=reverse or ?listenerOrder=random or ?listenerOrder=random(123) console.log( values.join( '' ) ); -} ); \ No newline at end of file +} ); + +QUnit.test( 'TinyEmitter listener order should match emit order (when reentrant)', assert => { + const emitter = new TinyEmitter<[ number ]>(); + let count = 1; + emitter.addListener( number => { + if ( number < 10 ) { + emitter.emit( number + 1 ); + } + } ); + emitter.addListener( number => { + assert.ok( number === count++, `should go in order of emitting: ${number}` ); + } ); + emitter.emit( count ); +} ); + + +QUnit.test( 'TinyEmitter reentrant listener order should not call newly added listener', assert => { + const emitter = new TinyEmitter<[ number ]>(); + let count = 1; + const neverCall = ( addedNumber: number ) => { + return ( number: number ) => { + assert.ok( number > addedNumber, `this should never be called for ${addedNumber} or earlier since it was added after that number's emit call` ); + }; + }; + emitter.addListener( number => { + if ( number < 10 ) { + emitter.addListener( neverCall( number ) ); + emitter.emit( number + 1 ); + + } + } ); + emitter.addListener( number => { + assert.ok( number === count++, `should go in order of emitting: ${number}` ); + } ); + emitter.emit( count ); +} ); Index: js/TinyPropertyTests.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/TinyPropertyTests.ts b/js/TinyPropertyTests.ts --- a/js/TinyPropertyTests.ts (revision 347828227d4ace993a8d2c759423c968bce4a893) +++ b/js/TinyPropertyTests.ts (date 1707509755635) @@ -49,4 +49,39 @@ x.hasFunProperty.value = true; x.hasFunProperty.value = false; x.hasFunProperty.value = true; +} ); + +QUnit.test( 'TinyProperty notify in value-change order', assert => { + let count = 2; // starts as a value of 1, so 2 is the first value we change to. + + const myProperty = new TinyProperty( 1 ); + + myProperty.lazyLink( value => { + if ( value < 10 ) { + myProperty.value = value + 1; + } + } ); + + myProperty.lazyLink( ( value, oldValue ) => { + console.log( `asserts ${oldValue} => ${value}` ); + assert.ok( value === oldValue + 1, `increment each time: ${oldValue} -> ${value}` ); + assert.ok( value === count++, `increment in order expected: ${count - 2}->${count - 1}, received: ${oldValue} -> ${value}` ); + } ); + myProperty.value = count; + + // We hope: + // 1->2 + // 2->3 + // 3->4 + // ... + // 8->9 + + // We think since it is buggy we will instead get: + + // We hope: + // 8->9 + // 7->8 + // 6->7 + // ... + // 1->2 } ); \ No newline at end of file Index: js/TinyEmitter.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/TinyEmitter.ts b/js/TinyEmitter.ts --- a/js/TinyEmitter.ts (revision 347828227d4ace993a8d2c759423c968bce4a893) +++ b/js/TinyEmitter.ts (date 1707510363410) @@ -17,6 +17,8 @@ const listenerOrder = _.hasIn( window, 'phet.chipper.queryParameters' ) && phet.chipper.queryParameters.listenerOrder; const listenerLimit = _.hasIn( window, 'phet.chipper.queryParameters' ) && phet.chipper.queryParameters.listenerLimit; +const EMIT_CONTEXT_MAX_LENGTH = 1000; + let random: Random | null = null; if ( listenerOrder && listenerOrder.startsWith( 'random' ) ) { @@ -31,6 +33,7 @@ type EmitContext = { index: number; listenerArray?: TEmitterListener[]; + args: T; }; // Store the number of listeners from the single TinyEmitter instance that has the most listeners in the whole runtime. @@ -114,30 +117,53 @@ if ( this.listeners.size > 0 ) { const emitContext: EmitContext = { - index: 0 + index: 0, + // We may not be able to emit right away, if we are currently emitting, then that needs to finish before we start. + args: args //.slice() as T // TODO: do we need to slice? https://github.com/phetsims/axon/issues/447 + + // TODO: use listenerArray instead of a new Array each time, https://github.com/phetsims/axon/issues/447 // listenerArray: [] // {Array.|undefined} assigned if a mutation is made during emit }; + // TODO: Is it horrible for performance/space to need a new object each time we emit? https://github.com/phetsims/axon/issues/447 this.emitContexts.push( emitContext ); - for ( const listener of this.listeners ) { - listener( ...args ); - emitContext.index++; + // This handles all reentrancy here (with a while loop), instead of doing so with recursion. + if ( this.emitContexts.length === 1 ) { + while ( this.emitContexts.length > 0 ) { + const emitContext = this.emitContexts[ 0 ]; // TODO: Why can't we call shift on this emitContext yet? https://github.com/phetsims/axon/issues/447 + const listeners = Array.from( this.listeners ); // TODO: use listenerArray instead of a new Array each time, https://github.com/phetsims/axon/issues/447 + for ( let i = 0; i < listeners.length; i++ ) { + listeners[ i ]( ...emitContext.args ); + emitContext.index++; - // If a listener was added or removed, we cannot continue processing the mutated Set, we must switch to - // iterate over the guarded array - if ( emitContext.listenerArray ) { - break; - } - } + // TODO: use listenerArray instead of a new Array each time, https://github.com/phetsims/axon/issues/447 + // // If the listeners were guarded during emit, we bailed out on the for..of and continue iterating over the original + // // listeners in order from where we left off. + // if ( emitContext.listenerArray ) { + // for ( let i = emitContext.index; i < emitContext.listenerArray.length; i++ ) { + // emitContext.listenerArray[ i ]( ...args ); + // } + // } + } - // If the listeners were guarded during emit, we bailed out on the for..of and continue iterating over the original - // listeners in order from where we left off. - if ( emitContext.listenerArray ) { - for ( let i = emitContext.index; i < emitContext.listenerArray.length; i++ ) { - emitContext.listenerArray[ i ]( ...args ); + // // TODO: use listenerArray instead of a new Array each time, https://github.com/phetsims/axon/issues/447 + // // If the listeners were guarded during emit, we bailed out on the for..of and continue iterating over the original + // // listeners in order from where we left off. + // if ( emitContext.listenerArray ) { + // for ( let i = emitContext.index; i < emitContext.listenerArray.length; i++ ) { + // emitContext.listenerArray[ i ]( ...args ); + // } + // } + this.emitContexts.shift(); } } - this.emitContexts.pop(); + else { + + // TODO: This may be a helpful infinite loop catcher, https://github.com/phetsims/axon/issues/447 + assert && assert( this.emitContexts.length <= EMIT_CONTEXT_MAX_LENGTH, 'reentrant depth of 1000 seems like a bug to me!' ); + console.log( 'emitContexts:', this.emitContexts.length ); + + } } } ```

[zepumph]

```diff Subject: [PATCH] tinyemitter --- Index: axon/js/TinyEmitterTests.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/axon/js/TinyEmitterTests.ts b/axon/js/TinyEmitterTests.ts --- a/axon/js/TinyEmitterTests.ts (revision 347828227d4ace993a8d2c759423c968bce4a893) +++ b/axon/js/TinyEmitterTests.ts (date 1707510199559) @@ -204,4 +204,40 @@ // Check these values when running with ?listenerOrder=reverse or ?listenerOrder=random or ?listenerOrder=random(123) console.log( values.join( '' ) ); -} ); \ No newline at end of file +} ); + +QUnit.test( 'TinyEmitter listener order should match emit order (when reentrant)', assert => { + const emitter = new TinyEmitter<[ number ]>(); + let count = 1; + emitter.addListener( number => { + if ( number < 10 ) { + emitter.emit( number + 1 ); + } + } ); + emitter.addListener( number => { + assert.ok( number === count++, `should go in order of emitting: ${number}` ); + } ); + emitter.emit( count ); +} ); + + +QUnit.test( 'TinyEmitter reentrant listener order should not call newly added listener', assert => { + const emitter = new TinyEmitter<[ number ]>(); + let count = 1; + const neverCall = ( addedNumber: number ) => { + return ( number: number ) => { + assert.ok( number > addedNumber, `this should never be called for ${addedNumber} or earlier since it was added after that number's emit call` ); + }; + }; + emitter.addListener( number => { + if ( number < 10 ) { + emitter.addListener( neverCall( number ) ); + emitter.emit( number + 1 ); + + } + } ); + emitter.addListener( number => { + assert.ok( number === count++, `should go in order of emitting: ${number}` ); + } ); + emitter.emit( count ); +} ); Index: axon/js/TinyPropertyTests.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/axon/js/TinyPropertyTests.ts b/axon/js/TinyPropertyTests.ts --- a/axon/js/TinyPropertyTests.ts (revision 347828227d4ace993a8d2c759423c968bce4a893) +++ b/axon/js/TinyPropertyTests.ts (date 1707510755864) @@ -49,4 +49,37 @@ x.hasFunProperty.value = true; x.hasFunProperty.value = false; x.hasFunProperty.value = true; +} ); + +QUnit.test( 'TinyProperty notify in value-change order', assert => { + let count = 2; // starts as a value of 1, so 2 is the first value we change to. + + const myProperty = new TinyProperty( 1 ); + + myProperty.lazyLink( value => { + if ( value < 10 ) { + myProperty.value = value + 1; + } + } ); + + myProperty.lazyLink( ( value, oldValue ) => { + console.log( `asserts ${oldValue} => ${value}` ); + assert.ok( value === oldValue + 1, `increment each time: ${oldValue} -> ${value}` ); + assert.ok( value === count++, `increment in order expected: ${count - 2}->${count - 1}, received: ${oldValue} -> ${value}` ); + } ); + myProperty.value = count; + + // Breadth first : + // 1->2 + // 2->3 + // 3->4 + // ... + // 8->9 + + // Depth first: + // 8->9 + // 7->8 + // 6->7 + // ... + // 1->2 } ); \ No newline at end of file Index: axon/js/TinyEmitter.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/axon/js/TinyEmitter.ts b/axon/js/TinyEmitter.ts --- a/axon/js/TinyEmitter.ts (revision 347828227d4ace993a8d2c759423c968bce4a893) +++ b/axon/js/TinyEmitter.ts (date 1707511591494) @@ -17,6 +17,8 @@ const listenerOrder = _.hasIn( window, 'phet.chipper.queryParameters' ) && phet.chipper.queryParameters.listenerOrder; const listenerLimit = _.hasIn( window, 'phet.chipper.queryParameters' ) && phet.chipper.queryParameters.listenerLimit; +const EMIT_CONTEXT_MAX_LENGTH = 1000; + let random: Random | null = null; if ( listenerOrder && listenerOrder.startsWith( 'random' ) ) { @@ -31,6 +33,7 @@ type EmitContext = { index: number; listenerArray?: TEmitterListener[]; + args: T; }; // Store the number of listeners from the single TinyEmitter instance that has the most listeners in the whole runtime. @@ -114,30 +117,53 @@ if ( this.listeners.size > 0 ) { const emitContext: EmitContext = { - index: 0 + index: 0, + // We may not be able to emit right away, if we are currently emitting, then that needs to finish before we start. + args: args //.slice() as T // TODO: do we need to slice? https://github.com/phetsims/axon/issues/447 + + // TODO: use listenerArray instead of a new Array each time, https://github.com/phetsims/axon/issues/447 // listenerArray: [] // {Array.|undefined} assigned if a mutation is made during emit }; + // TODO: Is it horrible for performance/space to need a new object each time we emit? https://github.com/phetsims/axon/issues/447 this.emitContexts.push( emitContext ); - for ( const listener of this.listeners ) { - listener( ...args ); - emitContext.index++; + // This handles all reentrancy here (with a while loop), instead of doing so with recursion. + if ( this.emitContexts.length === 1 ) { + while ( this.emitContexts.length > 0 ) { + const emitContext = this.emitContexts[ 0 ]; // TODO: Why can't we call shift on this emitContext yet? https://github.com/phetsims/axon/issues/447 + const listeners = emitContext.listenerArray || Array.from( this.listeners ); // TODO: use listenerArray instead of a new Array each time, https://github.com/phetsims/axon/issues/447 + for ( let i = 0; i < listeners.length; i++ ) { + listeners[ i ]( ...emitContext.args ); + emitContext.index++; - // If a listener was added or removed, we cannot continue processing the mutated Set, we must switch to - // iterate over the guarded array - if ( emitContext.listenerArray ) { - break; - } - } + // TODO: use listenerArray instead of a new Array each time, https://github.com/phetsims/axon/issues/447 + // // If the listeners were guarded during emit, we bailed out on the for..of and continue iterating over the original + // // listeners in order from where we left off. + // if ( emitContext.listenerArray ) { + // for ( let i = emitContext.index; i < emitContext.listenerArray.length; i++ ) { + // emitContext.listenerArray[ i ]( ...args ); + // } + // } + } - // If the listeners were guarded during emit, we bailed out on the for..of and continue iterating over the original - // listeners in order from where we left off. - if ( emitContext.listenerArray ) { - for ( let i = emitContext.index; i < emitContext.listenerArray.length; i++ ) { - emitContext.listenerArray[ i ]( ...args ); + // // TODO: use listenerArray instead of a new Array each time, https://github.com/phetsims/axon/issues/447 + // // If the listeners were guarded during emit, we bailed out on the for..of and continue iterating over the original + // // listeners in order from where we left off. + // if ( emitContext.listenerArray ) { + // for ( let i = emitContext.index; i < emitContext.listenerArray.length; i++ ) { + // emitContext.listenerArray[ i ]( ...args ); + // } + // } + this.emitContexts.shift(); } } - this.emitContexts.pop(); + else { + + // TODO: This may be a helpful infinite loop catcher, https://github.com/phetsims/axon/issues/447 + assert && assert( this.emitContexts.length <= EMIT_CONTEXT_MAX_LENGTH, `emitting reentrant depth of ${EMIT_CONTEXT_MAX_LENGTH} seems like a infinite loop to me!` ); + console.log( 'emitContexts:', this.emitContexts.length ); + + } } } Index: scenery/js/nodes/Node.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/scenery/js/nodes/Node.ts b/scenery/js/nodes/Node.ts --- a/scenery/js/nodes/Node.ts (revision 1e0a69172bf7cdf55b2002de53a2b9a623f808e1) +++ b/scenery/js/nodes/Node.ts (date 1707512215799) @@ -2808,6 +2808,10 @@ */ private onTransformChange(): void { // TODO: why is local bounds invalidation needed here? https://github.com/phetsims/scenery/issues/1581 + + if ( this.constructor.name === 'RichText' ) { + console.log( this.matrix.toString() ); + } this.invalidateBounds(); this._picker.onTransformChange(); Index: scenery-phet/js/NumberDisplay.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/scenery-phet/js/NumberDisplay.ts b/scenery-phet/js/NumberDisplay.ts --- a/scenery-phet/js/NumberDisplay.ts (revision b491faff4bed411f7a1576961e47245f593f96cb) +++ b/scenery-phet/js/NumberDisplay.ts (date 1707512096292) @@ -275,6 +275,12 @@ this.valueText = valueText; this.backgroundNode = backgroundNode; + + this.valueText.transformEmitter.listeners = new Set( [ () => { + Error.stackTraceLimit = 30; + console.log( new Error().stack ); + }, ...Array.from( this.valueText.transformEmitter.listeners ) ] ); + // Align the value in the background. ManualConstraint.create( this, [ valueText, backgroundNode ], ( valueTextProxy, backgroundNodeProxy ) => { @@ -289,11 +295,15 @@ valueTextProxy.left = backgroundNodeProxy.left + options.xMargin; } else { // right - valueTextProxy.right = backgroundNodeProxy.right - options.xMargin; + const right = backgroundNodeProxy.right - options.xMargin; + console.log( 'right', right ); + valueTextProxy.right = right; } // vertical alignment - valueTextProxy.centerY = backgroundNodeProxy.centerY; + const centerY = backgroundNodeProxy.centerY; + console.log( 'centerY', centerY ); + valueTextProxy.centerY = centerY; } ); this.mutate( options ); Index: axon/js/DynamicProperty.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/axon/js/DynamicProperty.ts b/axon/js/DynamicProperty.ts --- a/axon/js/DynamicProperty.ts (revision 347828227d4ace993a8d2c759423c968bce4a893) +++ b/axon/js/DynamicProperty.ts (date 1707511699449) @@ -247,14 +247,11 @@ * undefined. */ private onPropertyChange( newPropertyValue: OuterValueType | null, oldPropertyValue: OuterValueType | null | undefined ): void { - if ( this.lastPropertyPropertyValue ) { - this.derive( this.lastPropertyPropertyValue ).unlink( this.propertyPropertyListener ); - this.lastPropertyPropertyValue = null; + if ( oldPropertyValue ) { + this.derive( oldPropertyValue ).unlink( this.propertyPropertyListener ); } - if ( newPropertyValue ) { this.derive( newPropertyValue ).link( this.propertyPropertyListener ); - this.lastPropertyPropertyValue = newPropertyValue; } else { // Switch to null when our Property's value is null.

[zepumph]

Lots of great progress here. @jonathanolson and I fixed up the TinyEmitter changes and investigated the infinite loops. Here are the details:

1. In general this is a high quality patch that we are excited to get to (be at?) a commit point. Originally the patch above had one bug in it where listenerArray logic was causing listeners to sometimes be called more than once per emit (oops). That fixed the dynamicLocale infinite loop reported above.
2. MaxWidth and other layout don't play well together. There were cases (like in balancing-act and NumberDisplay and density) where a text's maxWidth + a ManualConstraint setting translation causes infinite loops with the new emitting algorithm. (That fixed the number display infinite loop)
3. Some unit tests were hard coded for the old buggy strategy. Now we get to hard code them to the better and more expected strategy. Woo!
4. Hookes law had an infinite loop from two Properties changing each other. It didn't trigger the TinyEmitter assertion, and it also didn't ever fail out in aqua fuzzing. I wonder if CT can handle something like this in a way that we will know what's happening? Also the fix will need a hookes-law issue to be improved to production.

In my opinion this patch is ready to commit, but I didn't want to do this on a Friday afternoon, and I'd like @jonathanolson to take another look before committing. I'm sure that CT will find some more trouble, it will just be a matter of handling things as they come up (since we can't locally reproduce all CT testing).

```diff Subject: [PATCH] tinyemitter emitting order, https://github.com/phetsims/axon/issues/447 --- Index: balancing-act/js/common/view/ImageMassNode.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/balancing-act/js/common/view/ImageMassNode.js b/balancing-act/js/common/view/ImageMassNode.js --- a/balancing-act/js/common/view/ImageMassNode.js (revision 006ed4082126413fbd01347bd32180252329feec) +++ b/balancing-act/js/common/view/ImageMassNode.js (date 1707521090606) @@ -42,6 +42,7 @@ const self = this; let massLabel; + let massLabelContainer; if ( isLabeled ) { // Add the mass indicator label. Note that it is positioned elsewhere. @@ -53,7 +54,8 @@ formatNames: [ '0', '1' ] } ); massLabel = new Text( massLabelStringProperty, { font: new PhetFont( 12 ) } ); - this.addChild( massLabel ); + massLabelContainer = new Node( { children: [ massLabel ] } ); + this.addChild( massLabelContainer ); // Observe changes to mass indicator label visibility. massLabelVisibleProperty.link( visible => { @@ -83,11 +85,15 @@ if ( isLabeled ) { - ManualConstraint.create( this, [ massLabel, imageNode ], ( massLabelProxy, imageNodeProxy ) => { - massLabelProxy.maxWidth = imageNodeProxy.width; - massLabelProxy.centerX = imageNodeProxy.centerX + modelViewTransform.modelToViewDeltaX( imageMass.centerOfMassXOffset ); - massLabelProxy.bottom = imageNodeProxy.top; - } ); + ManualConstraint.create( this, [ massLabel, massLabelContainer, imageNode ], + ( massLabelProxy, massLabelContainerProxy, imageNodeProxy ) => { + + // Avoid infinite loops like https://github.com/phetsims/axon/issues/447 by applying the maxWidth to a different Node + // than the one that is used for translation. + massLabelProxy.maxWidth = imageNodeProxy.width; + massLabelContainerProxy.centerX = imageNodeProxy.centerX + modelViewTransform.modelToViewDeltaX( imageMass.centerOfMassXOffset ); + massLabelContainerProxy.bottom = imageNodeProxy.top; + } ); // Increase the touchArea and mouseArea bounds in the x direction if the massLabel extends beyond the imageNode bounds. const bounds = imageNode.bounds.copy(); Index: axon/js/TinyEmitterTests.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/axon/js/TinyEmitterTests.ts b/axon/js/TinyEmitterTests.ts --- a/axon/js/TinyEmitterTests.ts (revision 347828227d4ace993a8d2c759423c968bce4a893) +++ b/axon/js/TinyEmitterTests.ts (date 1707518820711) @@ -142,26 +142,17 @@ assert.equal( entries.length, 7, 'Should have 7 callbacks' ); - assert.equal( entries[ 0 ].listener, 'a' ); - assert.equal( entries[ 0 ].arg, 'first' ); - - assert.equal( entries[ 1 ].listener, 'a' ); - assert.equal( entries[ 1 ].arg, 'second' ); - - assert.equal( entries[ 2 ].listener, 'b' ); - assert.equal( entries[ 2 ].arg, 'second' ); - - assert.equal( entries[ 3 ].listener, 'a' ); - assert.equal( entries[ 3 ].arg, 'third' ); - - assert.equal( entries[ 4 ].listener, 'b' ); - assert.equal( entries[ 4 ].arg, 'third' ); - - assert.equal( entries[ 5 ].listener, 'c' ); - assert.equal( entries[ 5 ].arg, 'third' ); - - assert.equal( entries[ 6 ].listener, 'b' ); - assert.equal( entries[ 6 ].arg, 'first' ); + const testCorrect = ( index: number, listenerName: string, emitCall: string ) => { + assert.equal( entries[ index ].listener, listenerName, `${index} correctness` ); + assert.equal( entries[ index ].arg, emitCall, `${index} correctness` ); + }; + testCorrect( 0, 'a', 'first' ); + testCorrect( 1, 'b', 'first' ); + testCorrect( 2, 'a', 'second' ); + testCorrect( 3, 'b', 'second' ); + testCorrect( 4, 'a', 'third' ); + testCorrect( 5, 'b', 'third' ); + testCorrect( 6, 'c', 'third' ); } ); @@ -204,4 +195,39 @@ // Check these values when running with ?listenerOrder=reverse or ?listenerOrder=random or ?listenerOrder=random(123) console.log( values.join( '' ) ); -} ); \ No newline at end of file +} ); + +QUnit.test( 'TinyEmitter listener order should match emit order (when reentrant)', assert => { + const emitter = new TinyEmitter<[ number ]>(); + let count = 1; + emitter.addListener( number => { + if ( number < 10 ) { + emitter.emit( number + 1 ); + } + } ); + emitter.addListener( number => { + assert.ok( number === count++, `should go in order of emitting: ${number}` ); + } ); + emitter.emit( count ); +} ); + +QUnit.test( 'TinyEmitter reentrant listener order should not call newly added listener', assert => { + const emitter = new TinyEmitter<[ number ]>(); + let count = 1; + const neverCall = ( addedNumber: number ) => { + return ( number: number ) => { + assert.ok( number > addedNumber, `this should never be called for ${addedNumber} or earlier since it was added after that number's emit call` ); + }; + }; + emitter.addListener( number => { + if ( number < 10 ) { + emitter.addListener( neverCall( number ) ); + emitter.emit( number + 1 ); + + } + } ); + emitter.addListener( number => { + assert.ok( number === count++, `should go in order of emitting: ${number}` ); + } ); + emitter.emit( count ); +} ); Index: axon/js/TinyPropertyTests.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/axon/js/TinyPropertyTests.ts b/axon/js/TinyPropertyTests.ts --- a/axon/js/TinyPropertyTests.ts (revision 347828227d4ace993a8d2c759423c968bce4a893) +++ b/axon/js/TinyPropertyTests.ts (date 1707510755864) @@ -49,4 +49,37 @@ x.hasFunProperty.value = true; x.hasFunProperty.value = false; x.hasFunProperty.value = true; +} ); + +QUnit.test( 'TinyProperty notify in value-change order', assert => { + let count = 2; // starts as a value of 1, so 2 is the first value we change to. + + const myProperty = new TinyProperty( 1 ); + + myProperty.lazyLink( value => { + if ( value < 10 ) { + myProperty.value = value + 1; + } + } ); + + myProperty.lazyLink( ( value, oldValue ) => { + console.log( `asserts ${oldValue} => ${value}` ); + assert.ok( value === oldValue + 1, `increment each time: ${oldValue} -> ${value}` ); + assert.ok( value === count++, `increment in order expected: ${count - 2}->${count - 1}, received: ${oldValue} -> ${value}` ); + } ); + myProperty.value = count; + + // Breadth first : + // 1->2 + // 2->3 + // 3->4 + // ... + // 8->9 + + // Depth first: + // 8->9 + // 7->8 + // 6->7 + // ... + // 1->2 } ); \ No newline at end of file Index: axon/js/TinyEmitter.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/axon/js/TinyEmitter.ts b/axon/js/TinyEmitter.ts --- a/axon/js/TinyEmitter.ts (revision 347828227d4ace993a8d2c759423c968bce4a893) +++ b/axon/js/TinyEmitter.ts (date 1707518336985) @@ -17,6 +17,8 @@ const listenerOrder = _.hasIn( window, 'phet.chipper.queryParameters' ) && phet.chipper.queryParameters.listenerOrder; const listenerLimit = _.hasIn( window, 'phet.chipper.queryParameters' ) && phet.chipper.queryParameters.listenerLimit; +const EMIT_CONTEXT_MAX_LENGTH = 1000; + let random: Random | null = null; if ( listenerOrder && listenerOrder.startsWith( 'random' ) ) { @@ -31,6 +33,7 @@ type EmitContext = { index: number; listenerArray?: TEmitterListener[]; + args: T; }; // Store the number of listeners from the single TinyEmitter instance that has the most listeners in the whole runtime. @@ -114,30 +117,46 @@ if ( this.listeners.size > 0 ) { const emitContext: EmitContext = { - index: 0 + index: 0, + + // We may not be able to emit right away. If we are already emitting and this is a recursive call, then that + // first emit needs to finish notifying its listeners before we start our notifications. + args: args //.slice() as T // TODO: do we need to slice? https://github.com/phetsims/axon/issues/447 + // listenerArray: [] // {Array.|undefined} assigned if a mutation is made during emit }; + // TODO: Is it horrible for performance/space to need a new object each time we emit? https://github.com/phetsims/axon/issues/447 this.emitContexts.push( emitContext ); - for ( const listener of this.listeners ) { - listener( ...args ); - emitContext.index++; + // This handles all reentrancy here (with a while loop), instead of doing so with recursion. + if ( this.emitContexts.length === 1 ) { + while ( this.emitContexts.length > 0 ) { + const emitContext = this.emitContexts[ 0 ]; + for ( const listener of this.listeners ) { + listener( ...emitContext.args ); + emitContext.index++; - // If a listener was added or removed, we cannot continue processing the mutated Set, we must switch to - // iterate over the guarded array - if ( emitContext.listenerArray ) { - break; - } - } + // If a listener was added or removed, we cannot continue processing the mutated Set, we must switch to + // iterate over the guarded array + if ( emitContext.listenerArray ) { + break; + } + } - // If the listeners were guarded during emit, we bailed out on the for..of and continue iterating over the original - // listeners in order from where we left off. - if ( emitContext.listenerArray ) { - for ( let i = emitContext.index; i < emitContext.listenerArray.length; i++ ) { - emitContext.listenerArray[ i ]( ...args ); + // If the listeners were guarded during emit, we bailed out on the for...of and continue iterating over the original + // listeners in order from where we left off. + if ( emitContext.listenerArray ) { + for ( let i = emitContext.index; i < emitContext.listenerArray.length; i++ ) { + emitContext.listenerArray[ i ]( ...emitContext.args ); + } + } + + this.emitContexts.shift(); } } - this.emitContexts.pop(); + else { + assert && assert( this.emitContexts.length <= EMIT_CONTEXT_MAX_LENGTH, `emitting reentrant depth of ${EMIT_CONTEXT_MAX_LENGTH} seems like a infinite loop to me!` ); + } } } Index: axon/js/EmitterTests.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/axon/js/EmitterTests.ts b/axon/js/EmitterTests.ts --- a/axon/js/EmitterTests.ts (revision 347828227d4ace993a8d2c759423c968bce4a893) +++ b/axon/js/EmitterTests.ts (date 1707518765291) @@ -140,28 +140,20 @@ */ _.each( entries, entry => { assert.ok( !( entry.listener === 'c' && entry.arg === 'first' ), 'not C,first' ); + assert.ok( !( entry.listener === 'c' && entry.arg === 'second' ), 'not C,first' ); } ); assert.equal( entries.length, 7, 'Should have 7 callbacks' ); - assert.equal( entries[ 0 ].listener, 'a' ); - assert.equal( entries[ 0 ].arg, 'first' ); - - assert.equal( entries[ 1 ].listener, 'a' ); - assert.equal( entries[ 1 ].arg, 'second' ); - - assert.equal( entries[ 2 ].listener, 'b' ); - assert.equal( entries[ 2 ].arg, 'second' ); - - assert.equal( entries[ 3 ].listener, 'a' ); - assert.equal( entries[ 3 ].arg, 'third' ); - - assert.equal( entries[ 4 ].listener, 'b' ); - assert.equal( entries[ 4 ].arg, 'third' ); - - assert.equal( entries[ 5 ].listener, 'c' ); - assert.equal( entries[ 5 ].arg, 'third' ); - - assert.equal( entries[ 6 ].listener, 'b' ); - assert.equal( entries[ 6 ].arg, 'first' ); + const testCorrect = ( index: number, listenerName: string, emitCall: string ) => { + assert.equal( entries[ index ].listener, listenerName, `${index} correctness` ); + assert.equal( entries[ index ].arg, emitCall, `${index} correctness` ); + }; + testCorrect( 0, 'a', 'first' ); + testCorrect( 1, 'b', 'first' ); + testCorrect( 2, 'a', 'second' ); + testCorrect( 3, 'b', 'second' ); + testCorrect( 4, 'a', 'third' ); + testCorrect( 5, 'b', 'third' ); + testCorrect( 6, 'c', 'third' ); } ); Index: scenery-phet/js/NumberDisplay.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/scenery-phet/js/NumberDisplay.ts b/scenery-phet/js/NumberDisplay.ts --- a/scenery-phet/js/NumberDisplay.ts (revision b491faff4bed411f7a1576961e47245f593f96cb) +++ b/scenery-phet/js/NumberDisplay.ts (date 1707521029071) @@ -23,6 +23,7 @@ import sceneryPhet from './sceneryPhet.js'; import Tandem from '../../tandem/js/Tandem.js'; import StringIO from '../../tandem/js/types/StringIO.js'; +import Vector2 from '../../dot/js/Vector2.js'; // constants const DEFAULT_FONT = new PhetFont( 20 ); @@ -267,7 +268,12 @@ backgroundNode.rectHeight = ( options.useFullHeight ? originalTextHeight : demoText.height ) + 2 * options.yMargin; } ); - options.children = [ backgroundNode, valueText ]; + // Avoid infinite loops like https://github.com/phetsims/axon/issues/447 by applying the maxWidth to a different Node + // than the one that is used for layout. + const valueTextContainer = new Node( { + children: [ valueText ] + } ); + options.children = [ backgroundNode, valueTextContainer ]; super(); @@ -275,25 +281,25 @@ this.valueText = valueText; this.backgroundNode = backgroundNode; - // Align the value in the background. - ManualConstraint.create( this, [ valueText, backgroundNode ], ( valueTextProxy, backgroundNodeProxy ) => { +// Align the value in the background. + ManualConstraint.create( this, [ valueTextContainer, backgroundNode ], ( valueTextContainerProxy, backgroundNodeProxy ) => { // Alignment depends on whether we have a non-null value. const align = ( numberProperty.value === null ) ? options.noValueAlign : options.align; + // vertical alignment + const centerY = backgroundNodeProxy.centerY; + // horizontal alignment if ( align === 'center' ) { - valueTextProxy.centerX = backgroundNodeProxy.centerX; + valueTextContainerProxy.center = new Vector2( backgroundNodeProxy.centerX, centerY ); } else if ( align === 'left' ) { - valueTextProxy.left = backgroundNodeProxy.left + options.xMargin; + valueTextContainerProxy.leftCenter = new Vector2( backgroundNodeProxy.left + options.xMargin, centerY ); } else { // right - valueTextProxy.right = backgroundNodeProxy.right - options.xMargin; + valueTextContainerProxy.rightCenter = new Vector2( backgroundNodeProxy.right - options.xMargin, centerY ); } - - // vertical alignment - valueTextProxy.centerY = backgroundNodeProxy.centerY; } ); this.mutate( options ); Index: density-buoyancy-common/js/density/view/DensityReadoutNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/density-buoyancy-common/js/density/view/DensityReadoutNode.ts b/density-buoyancy-common/js/density/view/DensityReadoutNode.ts --- a/density-buoyancy-common/js/density/view/DensityReadoutNode.ts (revision 1d5316c184749f2924dd14acdc91785da8a44da2) +++ b/density-buoyancy-common/js/density/view/DensityReadoutNode.ts (date 1707519974089) @@ -61,11 +61,12 @@ font: new PhetFont( 12 ), maxWidth: materialsMaxWidths[ index ] } ); - ManualConstraint.create( this, [ label ], labelProxy => { - labelProxy.centerX = x; - labelProxy.centerY = HEIGHT / 2; + const labelContainer = new Node( { children: [ label ] } ); + ManualConstraint.create( this, [ labelContainer ], labelContainerProxy => { + labelContainerProxy.centerX = x; + labelContainerProxy.centerY = HEIGHT / 2; } ); - this.addChild( label ); + this.addChild( labelContainer ); this.addChild( new Line( x, 0, x, label.top - LINE_PADDING, lineOptions ) ); this.addChild( new Line( x, HEIGHT, x, label.bottom + LINE_PADDING, lineOptions ) ); } ); @@ -110,10 +111,11 @@ const primaryLabel = new RichText( createDensityStringProperty( densityAProperty ), combineOptions( { fill: DensityBuoyancyCommonColors.labelAProperty }, labelOptions ) ); + const primaryLabelContainer = new Node( { children: [ primaryLabel ] } ); const primaryMarker = new Node( { children: [ primaryArrow, - primaryLabel + primaryLabelContainer ] } ); this.addChild( primaryMarker ); @@ -124,10 +126,11 @@ const secondaryLabel = new RichText( createDensityStringProperty( densityBProperty ), combineOptions( { fill: DensityBuoyancyCommonColors.labelBProperty }, labelOptions ) ); + const secondaryLabelContainer = new Node( { children: [ secondaryLabel ] } ); const secondaryMarker = new Node( { children: [ secondaryArrow, - secondaryLabel + secondaryLabelContainer ], y: HEIGHT } ); @@ -138,16 +141,16 @@ densityAProperty.link( density => { primaryMarker.x = mvt( density ); } ); - ManualConstraint.create( this, [ primaryLabel, primaryArrow ], ( primaryLabelProxy, primaryArrowProxy ) => { - primaryLabelProxy.centerBottom = primaryArrowProxy.centerTop; + ManualConstraint.create( this, [ primaryLabelContainer, primaryArrow ], ( primaryLabelContainerProxy, primaryArrowProxy ) => { + primaryLabelContainerProxy.centerBottom = primaryArrowProxy.centerTop; } ); // This instance lives for the lifetime of the simulation, so we don't need to remove this listener densityBProperty.link( density => { secondaryMarker.x = mvt( density ); } ); - ManualConstraint.create( this, [ secondaryLabel, secondaryArrow ], ( secondaryLabelProxy, secondaryArrowProxy ) => { - secondaryLabelProxy.centerTop = secondaryArrowProxy.centerBottom; + ManualConstraint.create( this, [ secondaryLabelContainer, secondaryArrow ], ( secondaryLabelContainerProxy, secondaryArrowProxy ) => { + secondaryLabelContainerProxy.centerTop = secondaryArrowProxy.centerBottom; } ); // This instance lives for the lifetime of the simulation, so we don't need to remove this listener Index: axon/js/DynamicProperty.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/axon/js/DynamicProperty.ts b/axon/js/DynamicProperty.ts --- a/axon/js/DynamicProperty.ts (revision 347828227d4ace993a8d2c759423c968bce4a893) +++ b/axon/js/DynamicProperty.ts (date 1707521650404) @@ -157,10 +157,6 @@ private propertyPropertyListener: ( value: InnerValueType, oldValue: InnerValueType | null, innerProperty: TReadOnlyProperty | null ) => void; private propertyListener: ( newPropertyValue: OuterValueType | null, oldPropertyValue: OuterValueType | null | undefined ) => void; - // We store the last propertyProperty's value as a workaround because Property is currently firing re-entrant Property - // changes in the incorrect order, see https://github.com/phetsims/axon/issues/447 - private lastPropertyPropertyValue: OuterValueType | null = null; - /** * @param valuePropertyProperty - If the value is null, it is considered disconnected. * @param [providedOptions] - options @@ -247,14 +243,11 @@ * undefined. */ private onPropertyChange( newPropertyValue: OuterValueType | null, oldPropertyValue: OuterValueType | null | undefined ): void { - if ( this.lastPropertyPropertyValue ) { - this.derive( this.lastPropertyPropertyValue ).unlink( this.propertyPropertyListener ); - this.lastPropertyPropertyValue = null; + if ( oldPropertyValue ) { + this.derive( oldPropertyValue ).unlink( this.propertyPropertyListener ); } - if ( newPropertyValue ) { this.derive( newPropertyValue ).link( this.propertyPropertyListener ); - this.lastPropertyPropertyValue = newPropertyValue; } else { // Switch to null when our Property's value is null. @@ -289,9 +282,8 @@ public override dispose(): void { this.valuePropertyProperty.unlink( this.propertyListener ); - if ( this.lastPropertyPropertyValue ) { - this.derive( this.lastPropertyPropertyValue ).unlink( this.propertyPropertyListener ); - this.lastPropertyPropertyValue = null; + if ( this.valuePropertyProperty.value !== null ) { + this.derive( this.valuePropertyProperty.value ).unlink( this.propertyPropertyListener ); } super.dispose(); Index: hookes-law/js/common/model/Spring.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/hookes-law/js/common/model/Spring.ts b/hookes-law/js/common/model/Spring.ts --- a/hookes-law/js/common/model/Spring.ts (revision b756bf236a87029b4334f97d14837345953c1c9e) +++ b/hookes-law/js/common/model/Spring.ts (date 1707520977668) @@ -43,6 +43,7 @@ import TReadOnlyProperty from '../../../../axon/js/TReadOnlyProperty.js'; import Range from '../../../../dot/js/Range.js'; import RangeWithValue from '../../../../dot/js/RangeWithValue.js'; +import Utils from '../../../../dot/js/Utils.js'; import optionize from '../../../../phet-core/js/optionize.js'; import PickOptional from '../../../../phet-core/js/types/PickOptional.js'; import PickRequired from '../../../../phet-core/js/types/PickRequired.js'; @@ -216,7 +217,8 @@ // Constrain to range, needed due to floating-point error. appliedForce = this.appliedForceRange.constrainValue( appliedForce ); - this.appliedForceProperty.value = appliedForce; + // TODO: this is not good enough, but it fixes the infinite loop. Definitely ask responsible dev for a real solution, https://github.com/phetsims/axon/issues/447 + this.appliedForceProperty.value = Utils.toFixedNumber( appliedForce, 10 ); } ); //------------------------------------------------

[zepumph]

I'm also marking as high priority because @jonathanolson and I are both excited for this work, and I would hate for it to lose momentum so close to publication.

[zepumph]

After talking with @jonathanolson, we feel like it is best that "depth" first be the default strategy for TinyEmitter(), and that "breadth" first be the default strategy for TinyProperty (since the state about value applies a more intuitive default behavior). So we propose making an option:

reentrantNotificationStrategy: "queue" | "stack".

Here is a good patch that improves on a few things, since may often run into the situation where our emitContext already is starting with a listenerArray (which is different from how main is working right now).

```diff Subject: [PATCH] fdsaf --- Index: balancing-act/js/common/view/ImageMassNode.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/balancing-act/js/common/view/ImageMassNode.js b/balancing-act/js/common/view/ImageMassNode.js --- a/balancing-act/js/common/view/ImageMassNode.js (revision 006ed4082126413fbd01347bd32180252329feec) +++ b/balancing-act/js/common/view/ImageMassNode.js (date 1707761241494) @@ -42,6 +42,7 @@ const self = this; let massLabel; + let massLabelContainer; if ( isLabeled ) { // Add the mass indicator label. Note that it is positioned elsewhere. @@ -53,7 +54,8 @@ formatNames: [ '0', '1' ] } ); massLabel = new Text( massLabelStringProperty, { font: new PhetFont( 12 ) } ); - this.addChild( massLabel ); + massLabelContainer = new Node( { children: [ massLabel ] } ); + this.addChild( massLabelContainer ); // Observe changes to mass indicator label visibility. massLabelVisibleProperty.link( visible => { @@ -83,11 +85,15 @@ if ( isLabeled ) { - ManualConstraint.create( this, [ massLabel, imageNode ], ( massLabelProxy, imageNodeProxy ) => { - massLabelProxy.maxWidth = imageNodeProxy.width; - massLabelProxy.centerX = imageNodeProxy.centerX + modelViewTransform.modelToViewDeltaX( imageMass.centerOfMassXOffset ); - massLabelProxy.bottom = imageNodeProxy.top; - } ); + ManualConstraint.create( this, [ massLabel, massLabelContainer, imageNode ], + ( massLabelProxy, massLabelContainerProxy, imageNodeProxy ) => { + + // Avoid infinite loops like https://github.com/phetsims/axon/issues/447 by applying the maxWidth to a different Node + // than the one that is used for translation. + massLabelProxy.maxWidth = imageNodeProxy.width; + massLabelContainerProxy.centerX = imageNodeProxy.centerX + modelViewTransform.modelToViewDeltaX( imageMass.centerOfMassXOffset ); + massLabelContainerProxy.bottom = imageNodeProxy.top; + } ); // Increase the touchArea and mouseArea bounds in the x direction if the massLabel extends beyond the imageNode bounds. const bounds = imageNode.bounds.copy(); Index: axon/js/TinyEmitterTests.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/axon/js/TinyEmitterTests.ts b/axon/js/TinyEmitterTests.ts --- a/axon/js/TinyEmitterTests.ts (revision 075c5a61434380dc206e52b4b57eabb1e797494d) +++ b/axon/js/TinyEmitterTests.ts (date 1707771379669) @@ -142,26 +142,17 @@ assert.equal( entries.length, 7, 'Should have 7 callbacks' ); - assert.equal( entries[ 0 ].listener, 'a' ); - assert.equal( entries[ 0 ].arg, 'first' ); - - assert.equal( entries[ 1 ].listener, 'a' ); - assert.equal( entries[ 1 ].arg, 'second' ); - - assert.equal( entries[ 2 ].listener, 'b' ); - assert.equal( entries[ 2 ].arg, 'second' ); - - assert.equal( entries[ 3 ].listener, 'a' ); - assert.equal( entries[ 3 ].arg, 'third' ); - - assert.equal( entries[ 4 ].listener, 'b' ); - assert.equal( entries[ 4 ].arg, 'third' ); - - assert.equal( entries[ 5 ].listener, 'c' ); - assert.equal( entries[ 5 ].arg, 'third' ); - - assert.equal( entries[ 6 ].listener, 'b' ); - assert.equal( entries[ 6 ].arg, 'first' ); + const testCorrect = ( index: number, listenerName: string, emitCall: string ) => { + assert.equal( entries[ index ].listener, listenerName, `${index} correctness` ); + assert.equal( entries[ index ].arg, emitCall, `${index} correctness` ); + }; + testCorrect( 0, 'a', 'first' ); + testCorrect( 1, 'b', 'first' ); + testCorrect( 2, 'a', 'second' ); + testCorrect( 3, 'b', 'second' ); + testCorrect( 4, 'a', 'third' ); + testCorrect( 5, 'b', 'third' ); + testCorrect( 6, 'c', 'third' ); } ); @@ -204,4 +195,107 @@ // Check these values when running with ?listenerOrder=reverse or ?listenerOrder=random or ?listenerOrder=random(123) console.log( values.join( '' ) ); -} ); \ No newline at end of file +} ); + +QUnit.test( 'TinyEmitter listener order should match emit order (reentrantNotify:queue)', assert => { + const emitter = new TinyEmitter<[ number ]>(); + let count = 1; + emitter.addListener( number => { + if ( number < 10 ) { + emitter.emit( number + 1 ); + } + } ); + emitter.addListener( number => { + assert.ok( number === count++, `should go in order of emitting: ${number}` ); + } ); + emitter.emit( count ); +} ); + +// TODO: for notify-stack too, https://github.com/phetsims/axon/issues/447 +QUnit.test( 'TinyEmitter reentrant listener order should not call newly added listener', assert => { + const emitter = new TinyEmitter<[ number ]>(); + let count = 1; + const neverCall = ( addedNumber: number ) => { + return ( number: number ) => { + assert.ok( number > addedNumber, `this should never be called for ${addedNumber} or earlier since it was added after that number's emit call` ); + }; + }; + emitter.addListener( number => { + if ( number < 10 ) { + emitter.addListener( neverCall( number ) ); + emitter.emit( number + 1 ); + } + } ); + emitter.addListener( number => { + assert.ok( number === count++, `should go in order of emitting: ${number}` ); + } ); + emitter.emit( count ); +} ); + +QUnit.test( 'TinyEmitter reentrant emit and addListener (reentrantNotify:queue)', assert => { + const emitter = new TinyEmitter<[ number ]>(); + assert.ok( 'hi' ); + + // don't change this number without consulting startNumber below + let count = 1; + const beforeNestedEmitListenerCalls: number[] = []; + const afterNestedEmitListenerCalls: number[] = []; + emitter.addListener( number => { + if ( number < 10 ) { + + // This listener should be called update the next emit, even though it is recursive + emitter.addListener( nestedNumber => { + assert.ok( nestedNumber !== number, 'nope' ); + if ( nestedNumber === number + 1 ) { + beforeNestedEmitListenerCalls.push( nestedNumber ); + } + } ); + emitter.emit( number + 1 ); + + // This listener won't be called until n+2 since it was added after then n+1 emit + emitter.addListener( nestedNumber => { + assert.ok( nestedNumber !== number, 'nope' ); + assert.ok( nestedNumber !== number + 1, 'nope' ); + if ( nestedNumber === number + 2 ) { + afterNestedEmitListenerCalls.push( nestedNumber ); + } + } ); + } + } ); + + emitter.addListener( number => { + assert.ok( number === count++, `should go in order of emitting: ${number}` ); + } ); + emitter.emit( count ); + + [ + beforeNestedEmitListenerCalls, + afterNestedEmitListenerCalls + ].forEach( ( collection, index ) => { + + const startNumber = index + 2; + collection.forEach( ( number, index ) => { + assert.ok( number === startNumber + index, `called correctly when emitting ${number}` ); + } ); + } ); +} ); + +// TODO: test multi emitters, https://github.com/phetsims/axon/issues/447 +QUnit.test( 'TinyEmitter reentrant multi emitters', assert => { + // const emitter = new TinyEmitter<[ number ]>(); + // assert.ok( 'hi' ); + // let count = 1; + // emitter.addListener( number => { + // if ( number < 10 ) { + // console.log( 'start original listener', number ); + // emitter.addListener( () => { console.log( `before nested emit addListener:${number}` ); } ); + // emitter.emit( number + 1 ); + // emitter.addListener( () => { console.log( `after nested emit addListener:${number}` ); } ); + // console.log( 'end original listener', number ); + // } + // } ); + // emitter.addListener( number => { + // // assert.ok( number === count++, `should go in order of emitting: ${number}` ); + // } ); + // emitter.emit( count ); +} ); Index: axon/js/TinyPropertyTests.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/axon/js/TinyPropertyTests.ts b/axon/js/TinyPropertyTests.ts --- a/axon/js/TinyPropertyTests.ts (revision 075c5a61434380dc206e52b4b57eabb1e797494d) +++ b/axon/js/TinyPropertyTests.ts (date 1707761241485) @@ -49,4 +49,37 @@ x.hasFunProperty.value = true; x.hasFunProperty.value = false; x.hasFunProperty.value = true; +} ); + +QUnit.test( 'TinyProperty notify in value-change order', assert => { + let count = 2; // starts as a value of 1, so 2 is the first value we change to. + + const myProperty = new TinyProperty( 1 ); + + myProperty.lazyLink( value => { + if ( value < 10 ) { + myProperty.value = value + 1; + } + } ); + + myProperty.lazyLink( ( value, oldValue ) => { + console.log( `asserts ${oldValue} => ${value}` ); + assert.ok( value === oldValue + 1, `increment each time: ${oldValue} -> ${value}` ); + assert.ok( value === count++, `increment in order expected: ${count - 2}->${count - 1}, received: ${oldValue} -> ${value}` ); + } ); + myProperty.value = count; + + // Breadth first : + // 1->2 + // 2->3 + // 3->4 + // ... + // 8->9 + + // Depth first: + // 8->9 + // 7->8 + // 6->7 + // ... + // 1->2 } ); \ No newline at end of file Index: axon/js/TinyEmitter.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/axon/js/TinyEmitter.ts b/axon/js/TinyEmitter.ts --- a/axon/js/TinyEmitter.ts (revision 075c5a61434380dc206e52b4b57eabb1e797494d) +++ b/axon/js/TinyEmitter.ts (date 1707771729412) @@ -17,6 +17,8 @@ const listenerOrder = _.hasIn( window, 'phet.chipper.queryParameters' ) && phet.chipper.queryParameters.listenerOrder; const listenerLimit = _.hasIn( window, 'phet.chipper.queryParameters' ) && phet.chipper.queryParameters.listenerLimit; +const EMIT_CONTEXT_MAX_LENGTH = 1000; + let random: Random | null = null; if ( listenerOrder && listenerOrder.startsWith( 'random' ) ) { @@ -31,6 +33,7 @@ type EmitContext = { index: number; listenerArray?: TEmitterListener[]; + args: T; }; // Store the number of listeners from the single TinyEmitter instance that has the most listeners in the whole runtime. @@ -113,31 +116,53 @@ // Notify wired-up listeners, if any if ( this.listeners.size > 0 ) { + // TODO: Pool emitContexts, figure out how to handle listenerArray already created. Same with args? https://github.com/phetsims/axon/issues/447 + // TODO: make an option for either: reentrantNotificationStrategy: "queue" "stack" https://github.com/phetsims/axon/issues/447 + const emitContext: EmitContext = { - index: 0 + index: 0, + + // We may not be able to emit right away. If we are already emitting and this is a recursive call, then that + // first emit needs to finish notifying its listeners before we start our notifications. + args: args //.slice() as T // TODO: do we need to slice? https://github.com/phetsims/axon/issues/447 + // listenerArray: [] // {Array.|undefined} assigned if a mutation is made during emit }; this.emitContexts.push( emitContext ); - for ( const listener of this.listeners ) { - listener( ...args ); - emitContext.index++; + // This handles all reentrancy here (with a while loop), instead of doing so with recursion. + if ( this.emitContexts.length === 1 ) { + while ( this.emitContexts.length > 0 ) { + const emitContext = this.emitContexts[ 0 ]; + const listeners = emitContext.listenerArray || this.listeners; + const startedWithListenerArray = !!emitContext.listenerArray; + + for ( const listener of listeners ) { + listener( ...emitContext.args ); + emitContext.index++; - // If a listener was added or removed, we cannot continue processing the mutated Set, we must switch to - // iterate over the guarded array - if ( emitContext.listenerArray ) { - break; - } - } + // If a listener was added or removed, we cannot continue processing the mutated Set, we must switch to + // iterate over the guarded array + if ( !startedWithListenerArray && emitContext.listenerArray ) { + break; + } + } - // If the listeners were guarded during emit, we bailed out on the for..of and continue iterating over the original - // listeners in order from where we left off. - if ( emitContext.listenerArray ) { - for ( let i = emitContext.index; i < emitContext.listenerArray.length; i++ ) { - emitContext.listenerArray[ i ]( ...args ); + // If the listeners were guarded during emit, we bailed out on the for...of and continue iterating over the original + // listeners in order from where we left off. + if ( !startedWithListenerArray && emitContext.listenerArray ) { + for ( let i = emitContext.index; i < emitContext.listenerArray.length; i++ ) { + emitContext.listenerArray[ i ]( ...emitContext.args ); + } + } + + + this.emitContexts.shift(); } } - this.emitContexts.pop(); + else { + assert && assert( this.emitContexts.length <= EMIT_CONTEXT_MAX_LENGTH, `emitting reentrant depth of ${EMIT_CONTEXT_MAX_LENGTH} seems like a infinite loop to me!` ); + } } } @@ -201,7 +226,10 @@ for ( let i = this.emitContexts.length - 1; i >= 0; i-- ) { // Once we meet a level that was already guarded, we can stop, since all previous levels were already guarded - if ( this.emitContexts[ i ].listenerArray ) { + // TODO: wouldn't we need the below guard, since guarding the listeners may want to overwrite the new listeners + // for future contexts? I don't think so, but I can't help but feel like the listenerArray needs updating if a + // previous listener changed the listener. JO likely we should just delete this one, https://github.com/phetsims/axon/issues/447 + if ( /*i === 0 &&*/ this.emitContexts[ i ].listenerArray ) { break; } else { Index: axon/js/EmitterTests.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/axon/js/EmitterTests.ts b/axon/js/EmitterTests.ts --- a/axon/js/EmitterTests.ts (revision 075c5a61434380dc206e52b4b57eabb1e797494d) +++ b/axon/js/EmitterTests.ts (date 1707761241450) @@ -140,28 +140,20 @@ */ _.each( entries, entry => { assert.ok( !( entry.listener === 'c' && entry.arg === 'first' ), 'not C,first' ); + assert.ok( !( entry.listener === 'c' && entry.arg === 'second' ), 'not C,first' ); } ); assert.equal( entries.length, 7, 'Should have 7 callbacks' ); - assert.equal( entries[ 0 ].listener, 'a' ); - assert.equal( entries[ 0 ].arg, 'first' ); - - assert.equal( entries[ 1 ].listener, 'a' ); - assert.equal( entries[ 1 ].arg, 'second' ); - - assert.equal( entries[ 2 ].listener, 'b' ); - assert.equal( entries[ 2 ].arg, 'second' ); - - assert.equal( entries[ 3 ].listener, 'a' ); - assert.equal( entries[ 3 ].arg, 'third' ); - - assert.equal( entries[ 4 ].listener, 'b' ); - assert.equal( entries[ 4 ].arg, 'third' ); - - assert.equal( entries[ 5 ].listener, 'c' ); - assert.equal( entries[ 5 ].arg, 'third' ); - - assert.equal( entries[ 6 ].listener, 'b' ); - assert.equal( entries[ 6 ].arg, 'first' ); + const testCorrect = ( index: number, listenerName: string, emitCall: string ) => { + assert.equal( entries[ index ].listener, listenerName, `${index} correctness` ); + assert.equal( entries[ index ].arg, emitCall, `${index} correctness` ); + }; + testCorrect( 0, 'a', 'first' ); + testCorrect( 1, 'b', 'first' ); + testCorrect( 2, 'a', 'second' ); + testCorrect( 3, 'b', 'second' ); + testCorrect( 4, 'a', 'third' ); + testCorrect( 5, 'b', 'third' ); + testCorrect( 6, 'c', 'third' ); } ); Index: scenery-phet/js/NumberDisplay.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/scenery-phet/js/NumberDisplay.ts b/scenery-phet/js/NumberDisplay.ts --- a/scenery-phet/js/NumberDisplay.ts (revision b491faff4bed411f7a1576961e47245f593f96cb) +++ b/scenery-phet/js/NumberDisplay.ts (date 1707761241588) @@ -23,6 +23,7 @@ import sceneryPhet from './sceneryPhet.js'; import Tandem from '../../tandem/js/Tandem.js'; import StringIO from '../../tandem/js/types/StringIO.js'; +import Vector2 from '../../dot/js/Vector2.js'; // constants const DEFAULT_FONT = new PhetFont( 20 ); @@ -267,7 +268,12 @@ backgroundNode.rectHeight = ( options.useFullHeight ? originalTextHeight : demoText.height ) + 2 * options.yMargin; } ); - options.children = [ backgroundNode, valueText ]; + // Avoid infinite loops like https://github.com/phetsims/axon/issues/447 by applying the maxWidth to a different Node + // than the one that is used for layout. + const valueTextContainer = new Node( { + children: [ valueText ] + } ); + options.children = [ backgroundNode, valueTextContainer ]; super(); @@ -275,25 +281,25 @@ this.valueText = valueText; this.backgroundNode = backgroundNode; - // Align the value in the background. - ManualConstraint.create( this, [ valueText, backgroundNode ], ( valueTextProxy, backgroundNodeProxy ) => { +// Align the value in the background. + ManualConstraint.create( this, [ valueTextContainer, backgroundNode ], ( valueTextContainerProxy, backgroundNodeProxy ) => { // Alignment depends on whether we have a non-null value. const align = ( numberProperty.value === null ) ? options.noValueAlign : options.align; + // vertical alignment + const centerY = backgroundNodeProxy.centerY; + // horizontal alignment if ( align === 'center' ) { - valueTextProxy.centerX = backgroundNodeProxy.centerX; + valueTextContainerProxy.center = new Vector2( backgroundNodeProxy.centerX, centerY ); } else if ( align === 'left' ) { - valueTextProxy.left = backgroundNodeProxy.left + options.xMargin; + valueTextContainerProxy.leftCenter = new Vector2( backgroundNodeProxy.left + options.xMargin, centerY ); } else { // right - valueTextProxy.right = backgroundNodeProxy.right - options.xMargin; + valueTextContainerProxy.rightCenter = new Vector2( backgroundNodeProxy.right - options.xMargin, centerY ); } - - // vertical alignment - valueTextProxy.centerY = backgroundNodeProxy.centerY; } ); this.mutate( options ); Index: density-buoyancy-common/js/density/view/DensityReadoutNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/density-buoyancy-common/js/density/view/DensityReadoutNode.ts b/density-buoyancy-common/js/density/view/DensityReadoutNode.ts --- a/density-buoyancy-common/js/density/view/DensityReadoutNode.ts (revision 6ed701f126e98d5314a02c497e5c11951470339e) +++ b/density-buoyancy-common/js/density/view/DensityReadoutNode.ts (date 1707761241508) @@ -61,11 +61,12 @@ font: new PhetFont( 12 ), maxWidth: materialsMaxWidths[ index ] } ); - ManualConstraint.create( this, [ label ], labelProxy => { - labelProxy.centerX = x; - labelProxy.centerY = HEIGHT / 2; + const labelContainer = new Node( { children: [ label ] } ); + ManualConstraint.create( this, [ labelContainer ], labelContainerProxy => { + labelContainerProxy.centerX = x; + labelContainerProxy.centerY = HEIGHT / 2; } ); - this.addChild( label ); + this.addChild( labelContainer ); this.addChild( new Line( x, 0, x, label.top - LINE_PADDING, lineOptions ) ); this.addChild( new Line( x, HEIGHT, x, label.bottom + LINE_PADDING, lineOptions ) ); } ); @@ -110,10 +111,11 @@ const primaryLabel = new RichText( createDensityStringProperty( densityAProperty ), combineOptions( { fill: DensityBuoyancyCommonColors.labelAProperty }, labelOptions ) ); + const primaryLabelContainer = new Node( { children: [ primaryLabel ] } ); const primaryMarker = new Node( { children: [ primaryArrow, - primaryLabel + primaryLabelContainer ] } ); this.addChild( primaryMarker ); @@ -124,10 +126,11 @@ const secondaryLabel = new RichText( createDensityStringProperty( densityBProperty ), combineOptions( { fill: DensityBuoyancyCommonColors.labelBProperty }, labelOptions ) ); + const secondaryLabelContainer = new Node( { children: [ secondaryLabel ] } ); const secondaryMarker = new Node( { children: [ secondaryArrow, - secondaryLabel + secondaryLabelContainer ], y: HEIGHT } ); @@ -138,16 +141,16 @@ densityAProperty.link( density => { primaryMarker.x = mvt( density ); } ); - ManualConstraint.create( this, [ primaryLabel, primaryArrow ], ( primaryLabelProxy, primaryArrowProxy ) => { - primaryLabelProxy.centerBottom = primaryArrowProxy.centerTop; + ManualConstraint.create( this, [ primaryLabelContainer, primaryArrow ], ( primaryLabelContainerProxy, primaryArrowProxy ) => { + primaryLabelContainerProxy.centerBottom = primaryArrowProxy.centerTop; } ); // This instance lives for the lifetime of the simulation, so we don't need to remove this listener densityBProperty.link( density => { secondaryMarker.x = mvt( density ); } ); - ManualConstraint.create( this, [ secondaryLabel, secondaryArrow ], ( secondaryLabelProxy, secondaryArrowProxy ) => { - secondaryLabelProxy.centerTop = secondaryArrowProxy.centerBottom; + ManualConstraint.create( this, [ secondaryLabelContainer, secondaryArrow ], ( secondaryLabelContainerProxy, secondaryArrowProxy ) => { + secondaryLabelContainerProxy.centerTop = secondaryArrowProxy.centerBottom; } ); // This instance lives for the lifetime of the simulation, so we don't need to remove this listener Index: axon/js/DynamicProperty.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/axon/js/DynamicProperty.ts b/axon/js/DynamicProperty.ts --- a/axon/js/DynamicProperty.ts (revision 075c5a61434380dc206e52b4b57eabb1e797494d) +++ b/axon/js/DynamicProperty.ts (date 1707761241440) @@ -157,10 +157,6 @@ private propertyPropertyListener: ( value: InnerValueType, oldValue: InnerValueType | null, innerProperty: TReadOnlyProperty | null ) => void; private propertyListener: ( newPropertyValue: OuterValueType | null, oldPropertyValue: OuterValueType | null | undefined ) => void; - // We store the last propertyProperty's value as a workaround because Property is currently firing re-entrant Property - // changes in the incorrect order, see https://github.com/phetsims/axon/issues/447 - private lastPropertyPropertyValue: OuterValueType | null = null; - /** * @param valuePropertyProperty - If the value is null, it is considered disconnected. * @param [providedOptions] - options @@ -247,14 +243,11 @@ * undefined. */ private onPropertyChange( newPropertyValue: OuterValueType | null, oldPropertyValue: OuterValueType | null | undefined ): void { - if ( this.lastPropertyPropertyValue ) { - this.derive( this.lastPropertyPropertyValue ).unlink( this.propertyPropertyListener ); - this.lastPropertyPropertyValue = null; + if ( oldPropertyValue ) { + this.derive( oldPropertyValue ).unlink( this.propertyPropertyListener ); } - if ( newPropertyValue ) { this.derive( newPropertyValue ).link( this.propertyPropertyListener ); - this.lastPropertyPropertyValue = newPropertyValue; } else { // Switch to null when our Property's value is null. @@ -289,9 +282,8 @@ public override dispose(): void { this.valuePropertyProperty.unlink( this.propertyListener ); - if ( this.lastPropertyPropertyValue ) { - this.derive( this.lastPropertyPropertyValue ).unlink( this.propertyPropertyListener ); - this.lastPropertyPropertyValue = null; + if ( this.valuePropertyProperty.value !== null ) { + this.derive( this.valuePropertyProperty.value ).unlink( this.propertyPropertyListener ); } super.dispose(); Index: hookes-law/js/common/model/Spring.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/hookes-law/js/common/model/Spring.ts b/hookes-law/js/common/model/Spring.ts --- a/hookes-law/js/common/model/Spring.ts (revision b756bf236a87029b4334f97d14837345953c1c9e) +++ b/hookes-law/js/common/model/Spring.ts (date 1707761241571) @@ -43,6 +43,7 @@ import TReadOnlyProperty from '../../../../axon/js/TReadOnlyProperty.js'; import Range from '../../../../dot/js/Range.js'; import RangeWithValue from '../../../../dot/js/RangeWithValue.js'; +import Utils from '../../../../dot/js/Utils.js'; import optionize from '../../../../phet-core/js/optionize.js'; import PickOptional from '../../../../phet-core/js/types/PickOptional.js'; import PickRequired from '../../../../phet-core/js/types/PickRequired.js'; @@ -216,7 +217,8 @@ // Constrain to range, needed due to floating-point error. appliedForce = this.appliedForceRange.constrainValue( appliedForce ); - this.appliedForceProperty.value = appliedForce; + // TODO: this is not good enough, but it fixes the infinite loop. Definitely ask responsible dev for a real solution, https://github.com/phetsims/axon/issues/447 + this.appliedForceProperty.value = Utils.toFixedNumber( appliedForce, 10 ); } ); //------------------------------------------------

[zepumph]

OK. Updates here:

1. I finally made some commits. Mostly to a new axon branch, but it was getting too hard to keep track of things.
2. I also committed the changes to sims. With our new decision that emitters don't notify based on queue, and instead stay as a stack, the maxWidth trouble is no longer an issue (it was based on a transform changedEmitter), but I still committed it. It seems really low risk, and prevents us ever needing to worry about these cases ever again if something else changes and causes these to infinite loop again.
3. I updated many tests, this helped guide the implementation
4. There are many (8) TODOs. I'll keep working on that.
5. There is a new infinite loop I found in keplers laws. I'll take a look.

[zepumph]

Ok. I couldn't figure out the Keplers laws recursion, and the hookes law was causing a reentrancy assertion which makes sense. Here is a patch for investigating both. I'd like to investigate and ask questions with @jonathanolson if he is available and interested.

``` Subject: [PATCH] add reentrantNotificationStrategy option to Emitter, https://github.com/phetsims/axon/issues/447 --- Index: axon/js/TinyEmitter.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/axon/js/TinyEmitter.ts b/axon/js/TinyEmitter.ts --- a/axon/js/TinyEmitter.ts (revision 8ab81fadd69e02f6a379ac5fa8d75d95a80b1a09) +++ b/axon/js/TinyEmitter.ts (date 1707801368527) @@ -259,6 +259,7 @@ } } else { + console.log( this.emitContexts.length ); assert && assert( this.emitContexts.length <= EMIT_CONTEXT_MAX_LENGTH, `emitting reentrant depth of ${EMIT_CONTEXT_MAX_LENGTH} seems like a infinite loop to me!` ); } Index: keplers-laws/js/common/model/EllipticalOrbitEngine.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/keplers-laws/js/common/model/EllipticalOrbitEngine.ts b/keplers-laws/js/common/model/EllipticalOrbitEngine.ts --- a/keplers-laws/js/common/model/EllipticalOrbitEngine.ts (revision af32aa8899c9f2cbe8d067e682597708f6f0e419) +++ b/keplers-laws/js/common/model/EllipticalOrbitEngine.ts (date 1707801481633) @@ -94,7 +94,7 @@ public readonly changedEmitter = new Emitter(); // For changes that mostly track the body through its orbit - public readonly ranEmitter = new Emitter(); + public readonly ranEmitter = new Emitter(/*Perhaps a queue? It didn't work on its own, I think it is more about how the body properties are queue based in the recursion*/); // For changes that require a reset of the orbit public readonly resetEmitter = new Emitter(); @@ -369,6 +369,8 @@ this.changedEmitter.emit(); + Error.stackTraceLimit = 100; + console.log( 'ran', new Error().stack ); this.ranEmitter.emit(); } Index: hookes-law/js/common/model/Spring.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/hookes-law/js/common/model/Spring.ts b/hookes-law/js/common/model/Spring.ts --- a/hookes-law/js/common/model/Spring.ts (revision 1dee6a0250157040be4c875eb3acaec01877730c) +++ b/hookes-law/js/common/model/Spring.ts (date 1707801209129) @@ -43,6 +43,7 @@ import TReadOnlyProperty from '../../../../axon/js/TReadOnlyProperty.js'; import Range from '../../../../dot/js/Range.js'; import RangeWithValue from '../../../../dot/js/RangeWithValue.js'; +import Utils from '../../../../dot/js/Utils.js'; import optionize from '../../../../phet-core/js/optionize.js'; import PickOptional from '../../../../phet-core/js/types/PickOptional.js'; import PickRequired from '../../../../phet-core/js/types/PickRequired.js'; @@ -216,7 +217,9 @@ // Constrain to range, needed due to floating-point error. appliedForce = this.appliedForceRange.constrainValue( appliedForce ); - this.appliedForceProperty.value = appliedForce; + // TODO: this is not good enough, but it fixes the infinite loop created in the linked issue. Definitely ask + // responsible dev for a real solution, https://github.com/phetsims/axon/issues/447. Uh oh, looks like it causes reentrancy from rounding errors. Yeah, not great. + this.appliedForceProperty.value = Utils.toFixedNumber( appliedForce, 10 ); } ); //------------------------------------------------ ```

[zepumph]

Lots of good progress here today. I fixed the infinite loops from hookes law and keplers laws with help from @pixelzoom and @AgustinVallejo. @jonathanolson and I were able to discuss the TinyEmitter changes and are feeling quite good. From here we would like to do two things:

1. @jonathanolson please review the current status of TinyEmitter on the reentrantNotificationStrategy branch (as well as the tests created). I know there are still some TODOs, but the main logic is ready to code review.
2. We would like to check in with @samreid and @pixelzoom about how this is going, especially before merging onto main.

[zepumph]

We had a meeting today that went well. Before moving to main I will make sure to another full round of fuzz testing and also snapshot comparison (to help prevent view-related differences). Over to @jonathanolson to code review before continuing. I'm hoping to be able to get back to this Friday. @jonathanolson can you review this by the end of Thursday?

[jonathanolson]

Implemented EmitContext pooling (hah, sims aren't creating more than 10-15 of these, and I think I cut two different places we were creating array copies).

Added review comments and documentation. @zepumph let me know how it looks? This looks like it's in a good state.

[zepumph]

I found two more infinite loops in calculus grapher and bending light. It may have to do with recent shapeProperty work. I'm still investigating.

[zepumph]

The bending light one is not an infinite loop. I have found that propagateTheRay can be called recursively thousands of times depending on certain cases, this patch helped me see that this behavior is also in the "stack only" branch too:

``` Subject: [PATCH] fdsaf --- Index: js/prisms/model/PrismsModel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/prisms/model/PrismsModel.ts b/js/prisms/model/PrismsModel.ts --- a/js/prisms/model/PrismsModel.ts (revision 85f35315f2fb92b06404015122076a41e5c9852d) +++ b/js/prisms/model/PrismsModel.ts (date 1708731179251) @@ -197,6 +197,8 @@ * @param laserInPrism - specifies whether laser in prism */ private propagate( ray: Ray2, power: number, laserInPrism: boolean ): void { + console.log( 'total from previous', window.recursiveCount ); + window.recursiveCount = 0; // Determines whether to use white light or single color light let mediumIndexOfRefraction; @@ -275,6 +277,13 @@ * extrema of white light wavelengths */ private propagateTheRay( incidentRay: ColoredRay, count: number, showIntersection: boolean ): void { + window.recursiveCount++; + if ( window.recursiveCount > 1000 ) { + console.log( 'propagateTheRay', window.recursiveCount ); + if ( window.recursiveCount > 5000 ) { + debugger; + } + } let rayColor; let rayVisibleColor; const waveWidth = CHARACTERISTIC_LENGTH * 5;

[zepumph]

Again for calculus grapher it wan't actually an infinite loop, it was just a large amount of lag in recalculating the curve while fuzzing. I was able to witness this in both stack and queue based branches.

[zepumph]

@jonathanolson went over the remaining thoughts about review comments. From here I will:

1. Snapshot comparison
2. update the implementation to see if I can try to factor out the loops for both strategies. I think it will be quite possible, and I feel really good about all the unit tests we have built up to have confidence as it pertains to regressions.

[zepumph]

I factored out where the loop logic is going, but since sometimes the list is a set, and sometimes it is a list, I don't really think we can always have a list and swap things out. Or, we could, but coercing the set into a list for every emit call seems much worse for performance than the index storage on EmitContext. @jonathanolson can you think of a better way?

[zepumph]

Snapshot comparison showed a couple sims that were quite different behaviorally from the queue-based change:

• Natural Selection
• Charges and Fields
• Center and Variability
• Neuron
• Greenhouse Effect

To solve them, I'm inclined to try to fix by switching things back to stack instead of trying to find them. But I haven't investigated it at all. I also don't know if this is even a problem. It will need some time.

    

[zepumph]

For CAV. I was able to reproduce this without the reentrant cases, so ignore that one.

[zepumph]

For Natural Selection, In >500 snapshots, I wasn't able to get an assertion with this patch. That said, I also want to note that I can see that this can happen in the frame that doesn't have my changes, so I don't believe that specific piece is caused by the TinyEmitter changes.

``` Subject: [PATCH] fd --- Index: js/common/view/environment/OrganismSprites.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/view/environment/OrganismSprites.ts b/js/common/view/environment/OrganismSprites.ts --- a/js/common/view/environment/OrganismSprites.ts (revision 51f25abc46d2d903283a3893a6fe0da929aa8ca3) +++ b/js/common/view/environment/OrganismSprites.ts (date 1710800809687) @@ -310,9 +310,27 @@ // Create the selection rectangle and put it immediately behind the selected bunny. this.selectionRectangleSpriteInstance = new BunnySelectionRectangleSpriteInstance( bunny, this.selectionRectangleSprite ); this.spriteInstances.splice( selectedBunnyIndex, 0, this.selectionRectangleSpriteInstance ); + + if ( assert ) { + let count = 0; + for ( let i = 0; i < this.spriteInstances.length; i++ ) { + this.spriteInstances[ i ] instanceof BunnySelectionRectangleSpriteInstance && count++; + } + assert && assert( count <= 1, 'one selection only' ); + } + if ( !this.isPlayingProperty.value ) { this.update(); } + + if ( assert ) { + let count = 0; + for ( let i = 0; i < this.spriteInstances.length; i++ ) { + this.spriteInstances[ i ] instanceof BunnySelectionRectangleSpriteInstance && count++; + } + assert && assert( count <= 1, 'one selection only' ); + } + } } }

[zepumph]

Hmmm. I'm actually seeing that it is quite possible to fail the snapshot comparison by just testing main/ vs main/ using the multi snapshot comparison tool in firefox. I'm running through all phet sims now to see if there is a pattern to the type of sims in which this can occur. Perhaps we are closer to merging to main than I thought.

[zepumph]

I haven't been back over here for two weeks because I have been so stumped about how to proceed with the snapshot comparison errors I got in https://github.com/phetsims/axon/issues/447#issuecomment-1965693132. It turns out that this was an error with webGL updates during snapshot comparison, and not with our branch. The only snapshot difference I see is in hookes law, which makes sense because we had to solve an infinite loop in that sim, but the image is not noticibly different.

From here we are ready to merge to main/. I will likely do that on monday.

[zepumph]

Alright. This was pushed to main this morning. I'll keep an eye on CT.

[zepumph]

I didn't find any infinite loops on CT this morning, after a full night. I'm ready to close this issue, and handle any other problems in side issues. Closing