Skip to content

Fix intermediate states caused by listener order #303

@zepumph

Description

@zepumph

In the beginning of #276, @samreid outlined a case where it was buggy that a DerivedProperty didn't have the right value in a listener of the dependency:

  const aProperty = new Property( 1 );
  const bProperty = new Property( 2 );
  let cProperty = null; // When shuffling, derived properties aren't necessarily first any more.  Simulate this by adding a listener first

  aProperty.lazyLink( a => {
    console.log( 'a updated', `${aProperty.value}+${bProperty.value}=${cProperty.value}` );
  } );
  bProperty.lazyLink( b => {
    console.log( 'c updated', `${aProperty.value}+${bProperty.value}=${cProperty.value}` );
  } );
  cProperty = new DerivedProperty( [ aProperty, bProperty ], ( a, b ) => a + b );
  cProperty.lazyLink( c => {
    console.log( 'c updated', `${aProperty.value}+${bProperty.value}=${cProperty.value}` );
  } );

  console.log( 'setting a to 5' );
  aProperty.value = 5;

in master the console logging will look like:

setting a to 5
a updated 5+2=3
c updated 5+2=7

Here cProperty.value won't be correct in aProperty's listener. Thus there is an intermediate state.

We feel this is buggy, and that we should try to fix this. Here are some approaches we outlined to go about this:

  1. Axon stops automatically firing listeners, and it is up to somewhere to tell axon to flush all the listeners (once we feel confident that each Property has taken its correct value). Potential places to flush:
    • Manually in the client code, like
      myProperty.value = 3;
      otherProperty.value = 2;
      axon.flush();
      
    • In common code, like when the sim is stepped.
  2. Use a strategy that lazily computes listeners, only at end points. This approach was taken from work done in Refactor describer listeners to avoid listener dependencies molarity#180, and may look like axon.flush() in Display.updateDisplay, as well as each other endpoint, like for aria-live, sound, vibration, etc. As well as for the end of input event handling. This could be implemented with a "mark dirty" approach to improve performance. Only dirty items get listeners to fire.
  3. If this sort of intermediate state can be targeted for a specific case, then one solution is to turn multiple problems (that have this listener order issue as a collective) into a single Property that holds a conglomerate state of them all. For the example above, something like:
     const abProperty = new Property( { a: 1, b: 2 } );
     abProperty.link( ab => console.log( 'abProperty', `${ab.a}+${ab.b}=${ab.a+ab.b}` ) );
    
  4. Similar to/expanding on (1), axon can add a graph engine, like we do in PropertyStateHandler for PhET-iO, that registers dependencies that certain Properties have on each other, and axon can then make sure that when flushing, that order is upheld, and that no intermediate states fire listeners for dependencies.

We do not feel like this is a high priority. We don't have a specific example in code that is causing problems (outside of potential usages of ?shuffleListeners). In general it feels like the development team is really used to coding around this deficiency in axon. We know in the back of our head that order dependencies in listeners is bad, but sometimes we do it anyways because it is the easiest path forward. We write listeners (especially for Multilinks) that can handle being called with intermediate states because we expect this behavior. As a result, though we want to investigate this potential improvement, we don't know when we will get to this, or if investigation will even lead to an eventual adoption of a strategy by the larger codebase and team.

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions