how to handle gotchas related to Property setDeferred

[pixelzoom]

Over in #277, I encountered Property setDeferred for the first time. This feature seems to have gotchas that are not at all guarded.

For example, after a set, you may still get the old value:

> var x = new phet.axon.Property( 'foo' )
undefined
> x.setDeferred( true )
null
> x.value = 'bar'
"bar"
> x.value
"foo"

Should get() fail an assertion if it's called while this.isDeferred === true?

Are there other gotchas?

[zepumph]

Are there other gotchas?

Looking through Property functions, perhaps reset also?

This is a pretty serious bug; one that I am unsure how to solve easily but which is easy to demonstrate.

1. Add this line to the top of Properyt.get():
   assert && assert( !this.isDeferred, 'value may be stale if this Property is deferred' );
2. Add a debugger; to assert.js
3. Fuzz the state wrapper for Charges And Fields.

In the general case, calling get on a Property that is currently deferred could occur anywhere in the code, and this is only exposed by the state wrapper. I'm going to raise priority for the PhET-iO team to investigate.

[pixelzoom]

resetdoes seem like a problem. If reset is called while this.isDeferred === true, then the reset is deferred.

I understand the motivation for this "deferred" feature. But it seems like something that should be avoided. And it's not robustly instrumented or adequately tested - only 1 unit test!

[samreid]

Should get() fail an assertion if it's called while this.isDeferred === true?

I don't think so--that would break its interface. You should be able to get a Property value at any time. When a property is deferred, you should still be able to read the old value. This behavior is also tested in the unit test:

  const property = new Property( 0 );
  //...
  property.setDeferred( true );
  property.value = 1;
  property.value = 2;
  assert.equal( property.value, 0, 'should have original value' );

Reset also makes sense to me. Resetting is equivalent to setting the original value. So it should behave the same as any defer.

But it seems like something that should be avoided.

Agreed, we have been using it sparingly. Particularly because it is not re-entrant.

And it's not robustly instrumented or adequately tested - only 1 unit test!

Yes, there is only 1 test, but 7 assertions are tested in it. What else do you recommend to test?

[pixelzoom]

You should be able to get a Property value at any time. When a property is deferred, you should still be able to read the old value.

I disagree. What is the use case for reading a Property's value while it's in a deferred state? Imo this encourages a certain type of code that should be avoided.

Reset also makes sense to me. Resetting is equivalent to setting the original value. So it should behave the same as any defer.

Again, I disagree. What is the use case?

And there are other things that don't seem to have been considered (or maybe just not documented?):

• notifyListenersStatic can be called while deferred. Why?
• Listeners can be added/remove while deferred? Why?
• After dispose, setDeferred(false) is forbidden, but get is allowed. Why?

Agreed, we have been using it sparingly. Particularly because it is not re-entrant.

If we're going to use it sparingly, what is the purpose of allowing anything other than set(value) and setDeferred(false) while deferred?

Yes, there is only 1 test, but 7 assertions are tested in it. What else do you recommend to test?

Recommended tests:

• reset while deferred
• dispose while deferred
• DerivedProperty tests where the DerivedProperty and/or dependencies are deferred

Robustness:

• document the "deferred" behavior for all relevant methods
• review which methods should be allowed while deferred
• review what is allowed after dispose has been called

[samreid]

setDeferred does not mean the property is "bad" and should not be read. It means that sometime soon it may take a new value. A case that @zepumph described in #281 (comment) follows this basic pattern:

• We want to set the state with the PhetioStateEngine
• The model Properties are deferred at the beginning of setState
• The PhetioGroup of ChargedParticles is cleared
• We create a new ChargedParticle
• During construction of the new ChargedParticle, it queries the value of a deferred Property, in this case it is chargesAndSensorsEnclosureBoundsProperty

I commented out that occurrence of Property.get() and ran the test again. It failed again with a similar issue on a different Property.

[samreid]

Could we prevent reads from deferred properties if we change the PhetioStateEngine sequence to hold off of creating new elements until the Properties have all taken their new values? I'm not sure whether that is technically feasible.

[zepumph]

to hold off of creating new elements until the Properties have all taken their new values

What about children Properties of dynamic elements, is that ok? How does the PhET-iO state engine know when it has done all it can without dynamic elements?

[pixelzoom]

I don't have anything else to add here, so unassigning.

[zepumph]

What about children Properties of dynamic elements, is that ok? How does the PhET-iO state engine know when it has done all it can without dynamic elements?

Perhaps we need to tweak our state setting algorithm:

• Run though setting all items that currently exist (don't create anything)
• When we are no longer changing anything (meannign everything else is dynamic), then try to create dynamic elements.

This way the order of "undeferring" (firing Property listeners) at the end of state set would first do static items, then it would do dynamic elements that may be more likely to try to access undeferred items.

Am I understanding the problem that we are trying to solve correctly?

[samreid]

This does not feel like an imminent problem. Maybe it would be appropriate to work on in the context of a practical problem when one arises. Re-labeling until then.