Closed zepumph closed 5 years ago
I agree that this is good to discuss. But the examples above for aNewNode
and bNode
are cases where nothing needs to be done, and might cause misconceptions. If aNewNode
was a constructor parameter (a Node that we don't own), and bNode
had a tandem
option, then cleanup for those things would be legitimate.
In dispose order matters, ...
Misc thoughts about order...
(1) Endeavor to write code where order of disposal doesn't matter.
(2) In my experience, order of disposal rarely matters.
(3) Rules like "call super.dispose
last" do not generally apply, since it assumes that super
was called in the constructor before anything was done that will need undoing.
(4) Adding something like reverseCalling
to Emitter encounters the problem noted in (3). If you want to be strict about order, then some of the stuff handled by disposeEmitter
may need to be handled before super.dispose
and some after super.dispose
.
(4) Adding something like
reverseCalling
to Emitter encounters the problem noted in (3). If you want to be strict about order, then some of the stuff handled bydisposeEmitter
may need to be handled beforesuper.dispose
and some aftersuper.dispose
.
If the base type had a reverseCalling
Emitter
, then it could tear down in the opposite order of creation.
I totally agree with (1). Regarding (2)--I'm not sure how much code would break if I went through and scrambled the order of disposals, but it would be an interesting experiment.
(5) Using Emitter for disposal actions has the advantage that specifying an action (e.g. link
) and its dispose action (e.g. unlink
) can be co-located in the code. It was the disadvantage that you can't easily see the ultimate order of dispose actions, as you can with this.dispose{{Classnames}}
or this.dispose
.
If the base type had a reverseCalling Emitter, then it could tear down in the opposite order of creation.
Yes, I understand. Please read this point again. How do you propose to call super.dispose
somewhere in the middle of an Emitter's listener list.
(6) This relies on Emitter's listeners being called in the order that they were added. As I noted in https://github.com/phetsims/graphing-lines/issues/109#issuecomment-455900546:
In general... The Observer pattern does not specify order of notification. java.util.Observable in fact specifies that "the order in which notifications will be delivered is unspecified". And writing code that relies on order is generally a bad practice. Order of notifications for Property and Emitter was unspecified until you decided otherwise in phetsims/axon@1239bb6.
If order is really important, then relying on the order of Emitter listener's is an ordering dependency that you'll be leaning on heavily. phetsims/axon@1239bb6 put us on that trajectory, and it seems like you're willing to leverage that. I think that will prove to be unwise in the long run.
How do you propose to call super.dispose somewhere in the middle of an Emitter's listener list.
If everything is done through resetEmitter
then dispose
will not need to be overridden and super.dispose
will not need to be called.
Is this what you're proposing?
...
super();
this.disposeEmitter = new Emitter();
this.disposeEmitter.addListener( ()=> { super.dispose(); } );
...
dispose(){
this.disposeEmitter.emit();
}
@samreid said:
If everything is done through
resetEmitter
thendispose
will not need to be overridden andsuper.dispose
will not need to be called.
I don't understand this at all. What is resetEmitter
? How can cleanup of the superclass be accomplished if super.dispose
is not called? If dispose
is not overridden, how does disposeEmitter.emit
get called?
Oops, sorry I meant disposeEmitter
everywhere I said resetEmitter
.
For example:
class Supertype{
constructor(){
this.disposeEmitter = new Emitter({reverseOrder:true});
this.linkSomething();
this.disposeEmitter.addListener(()=>this.unlinkSomething());
}
dispose(){
this.disposeEmitter.emit();
}
}
class Subtype extends Supertype{
constructor(){
this.myComponent = new Component(...);
this.disposeEmitter.addListener(()=>this.myComponent.dispose());
this.linkSomethingElse();
this.disposeEmitter.addListener(()=>this.unlinkSomethingElse());
}
// No need to override dispose.
}
I'm trying to follow you here, but you're making pretty big unexplained leaps. When we started this example, this.disposeEmitter
was @private
and defined by the subclass. Now it looks like you're proposing that this.disposeEmitter
is provided by the superclass (the root superclass?), and is either @protected
or @public
-- all of which presents a whole new set of problems.
In https://github.com/phetsims/axon/issues/214#issuecomment-458259781, @samreid said:
If the base type had a
reverseCalling
Emitter
, then it could tear down in the opposite order of creation.
OK, so I missed the proposal to move this to "the base type".
Are you thinking that PhetioObject
might be "the base type" where disposeEmitter
is defined? If so, that would mean that (1) PhetioObject is required for anything that needs to be disposed, (2) subclasses that need to use a different dispose pattern would potentially be saddled with disposeEmitter
, and (3) PhetioObject has non-PhET-iO responsibilities and should be renamed.
Now it looks like you're proposing that
this.disposeEmitter
is provided by the superclass (the root superclass?), and is either@protected
or@public
-- all of which presents a whole new set of problems.
Right, the pattern identified in https://github.com/phetsims/axon/issues/214#issuecomment-458268028 requires either disposeEmitter
to be public or protector, or for there to be a public/protected way of adding listeners to it.
Are you thinking that
PhetioObject
might be "the base type" wheredisposeEmitter
is defined?
That doesn't seem like a great fit. Alternatives may be (a) creating a new base type Disposable
which PhetioObject
extends. Or just letting types use this pattern identified in https://github.com/phetsims/axon/issues/214#issuecomment-458268028 without trying to factor it out. I'm not sure what's best.
After this discussion does it seem like disposeEmitter
is a pattern worth having?
I currently see 8 usages of disposeEmitter.addListener
, and only one of it is called on this
. All the rest are treating it as a public method. It seems like the pattern discussed this morning is not actually a pattern we have adopted in PhET code yet.
The base type implementation seems like overkill, and like we are trying to solve a problem we don't have. I would likely feel differently if this was the general pattern that PhET preferred, but it isn't, its a third tier bench-warmer used for special, and already more complex, cases of disposal.
That said, and having read the discussion above, if we do go with this pattern, then I think that I prefer the disposeEmitter
pattern outlined as such:
this.disposeEmitter
that will have its listeners called in reverse orderaddListener
for each action, recognizing that the order of adding is the opposite order of callingdisposeEmitter
super.dispose
call as a listener to the emitter, and have the dispose
method singularly call the disposeEmitter
So to sum up, I outline two options:
What do people think?
@zepumph asked:
.... What do people think?
(1) If every class uses disposeEmitter
, then subclasses will blow away their parent class' disposeEmitter
. A naming convention like disposeEmitter{{Classname}}
is needed.
(2) Unless you require that super
is called in the constructor before doing anything related to memory management (which is not a constraint that we should be willing to live with), you cannot accomplish "remove in opposite order" using this approach. Example:
constructor( property1, property2 ) {
const disposeEmitter = new Emitter( { reverseOrder: true } );
const property1Listener = ...;
property1.link( property1Listener );
disposeEmitter.addListener( () => { property1.unlink( property1Listener ); } );
super();
const property2Listener = ...;
property2.link( property1Listener );
disposeEmitter.addListener( () => { property2.unlink( property2Listener ); } );
// @private
this.disposeEmitter = disposeEmitter;
}
dispose() {
this.disposeEmitter.emit();
super.dispose();
}
Order of setup is: property1, super, property2
Order of cleanup is: property2, property1, super
(1) If every class uses disposeEmitter, then subclasses will blow away their parent class' disposeEmitter. A naming convention like disposeEmitter{{Classname}} is needed.
I very much like that.
RE (2): I thought that this block would cover that, in the rare cases that we find it:
ATYPICAL CASE2 (rare and tricky, but possible): If you have to call super dispose in the middle of actions, then add the super.dispose call as a listener to the emitter, and have the dispose method singularly call the disposeEmitter
such that the above statement would look like:
constructor( property1, property2 ) {
const disposeEmitter = new Emitter( { reverseOrder: true } );
const property1Listener = ...;
property1.link( property1Listener );
disposeEmitter.addListener( () => { property1.unlink( property1Listener ); } );
super();
disposeEmitter.addListener(()=>{super.dispose()});
const property2Listener = ...;
property2.link( property1Listener );
disposeEmitter.addListener( () => { property2.unlink( property2Listener ); } );
// @private
this.disposeEmitter = disposeEmitter;
}
dispose() {
this.disposeEmitter.emit();
}
ATYPICAL CASE2 (rare and tricky, but possible): If you have to call super dispose in the middle of actions, then add the super.dispose call as a listener to the emitter, and have the dispose method singularly call the disposeEmitter
I missed that, thanks for pointing it out. But I think it's a really bad idea to override something and then chain to the super method elsewhere.
I missed that, thanks for pointing it out. But I think it's a really bad idea to override something and then chain to the super method elsewhere.
I think I agree that it is a code smell, and not preferred, in our branching decision tree we have found ourselves out on quite the leaf:
1st method doesn't work -> 2nd method doesn't work so use disposeEmitter{{TypeName}} -> normal supertype dispose
method calling doesn't work -> add listener with super.dispose()
in it.
And we still have a solution.
Before continuing to discuss this edge case it may be worth investigating how many times it occurs in the project.
There are 300 usages of .dispose.call
, and I glanced at 100 and only saw 1 place where the dispose call was straddled by other code, and I'm not really even sure that's problematic (BAAGameChallenge
)
@zepumph said:
... only saw 1 place where the dispose call was straddled by other code, and I'm not really even sure that's problematic
Here's the organization that I typically use (and prefer) for Node subclasses. As soon as we add PhET-iO instrumentation to this code, we will have straddling of super
.
class MyNode extends Node {
constructor() {
// Create the scenograph for MyNode
// Create Nodes for subcomponents, some of which need to be instrumented.
super( {
children: [ nodes created above ]
} );
// Now add observers (Property link, addInputListener, etc.)
}
}
And I most definitely do not want to be forced to change the above to require mutate
:
class MyNode extends Node {
constructor( ..., options ) {
super();
// Create the scenograph for MyNode
// Create Nodes for subcomponents, some of which need to be instrumented.
options.children = [ nodes created above ];
this.mutate( options );
// Now add observers (Property link, addInputListener, etc.)
}
}
and I'm not really even sure that's problematic
To be clear I meant the usage I found
Here's the organization that I typically use (and prefer) for Node subclasses. As soon as we add PhET-iO instrumentation to this code, we will have straddling of super.
Can you provide an example of when you have disposed the supertype in the middle of disposing code for the type. I can't find usages so it is hard for me to gauge how much a problem it is.
@pixelzoom, since the case outlined in ATYPICAL CASE2 is so offensive, and sounds like it won't work, would you please recommend the form you would like to see this pattern take?
Note the above commit is just a work in progress, and the section that relates to the dispose emitter pattern will update based on what we decide in this issue.
@zepumph asked:
Can you provide an example of when you have disposed the supertype in the middle of disposing code for the type. I can't find usages so it is hard for me to gauge how much a problem it is.
I have not had a need to do this. As I mentioned in https://github.com/phetsims/axon/issues/214#issuecomment-458258547:
(2) In my experience, order of disposal rarely matters.
@zepumph asked:
@pixelzoom, since the case outlined in ATYPICAL CASE2 is so offensive, and sounds like it won't work, would you please recommend the form you would like to see this pattern take?
I'm afraid that we haven't identified a form that I can get behind. I'm just not a fan of this pattern. It seems to be addressing a problem whose existence I question. We haven't identified a form that doesn't have introduce problems that seem worse than the problem we're trying to solve. And using an Emitter with listeners feels like overkill.
From discussion on Monday, we don't think that finding a total consensus on this issue is worth the cost.
In discussing this pattern, we are trying to nail down a "third tier" pattern, for use after our two more preferred ones.
In the design pattern doc it says:
Sometimes the above, preferred patterns won't work. For example sometimes components are conditionally created, and
therefore are only conditionally disposed. If there are these sorts of complex disposal constraints, then create an
AXON/Emitter
to manage disposal tasks, and add a listener to the Emitter for each disposal task.
disposeEmitter{{TypeName}}
dispose
method, emit the dispose emitter, and call the parent dispose (super.dispose()
) in the appropriate
order.See issue for details on the "dispose emitter" pattern.
This seems close enough to a pattern that is reasonable to use if absolutely needed. Closing
From design patterns conversation today during core hours (https://github.com/phetsims/phet-info/issues/88#issuecomment-458242942), we discussed using an emitter to perform disposal actions as a "catch all" choice when our more preferred options (
disposeMyType
function in constructor and putting everything on the prototypedispose
method) weren't ideal.Here is an example of the pattern:
Further discussion with @samreid has brought up a few ideas/concerns to discuss.
In dispose order matters, and in general we want to tear down in the opposite order we create, so
this.disposeEmitter
should call its listeners in reverse order. We could support that with an option likereverseCalling
.We could go one step further to generalize this pattern with a new type,
DisposeEmitter
, that does that for you.Without this feature, the disposeEmitter pattern seems brittle and unusable in the general case. Discussion of this is blocking further work in https://github.com/phetsims/phet-info/issues/88