TinyEmitter.emit should assert that it isn't disposed #242
Comments
|
This can be quickly triggered in CCK by dragging a wire from the carousel, then selecting the wire and pressing the trash can button at the bottom of the screen. I would like to understand what is happening better before commenting on whether we can proceed with this assertion. |
|
@pixelzoom will investigate his sims and report back |
|
To reproduce manually in unit-rates:
Stack trace: |
|
To reproduce manually in equality-explorer:
Stack trace: |
|
To reproduce manually in fractions-intro:
Stack trace: |
|
I could not manually reproduce with CCK:DCVL. After 5+ minutes with |
|
All of the above stack traces involve a sun button and SCENERY/PressListener (see 👉 annotations in stack traces). So I don't think we have a sim-specific problem here. @ariel-phet I think it would be most efficient/effective to have either @jbphet or @jonathanolson pick up the investigation from here. |
|
5/16/19 dev meeting: @samreid pointed out that this seems to happen when clicking a button causes it to dispose itself. We'll wait for @ariel-phet to assign. |
This is true in some cases, not in others. I dug into it, and found that for two of the cases, the problem is that the But...this only fixed things for CCK and Unit Rates. The other sims had different issues. I investigated for Equality Explorer, and the problem in this case is that I looked at the fractions sims, and I wasn't sure what exactly was happening, but it seemed pretty specific to that sim, and not a general issue with the radio buttons. I think we should decide whether this assertion is something that we really really want and, if so, each of the responsible developers should be assigned (including me) to do their part before adding the assertion, then we turn it on. In the mean time, I will run a local aqua test to see if reversing the disposal call order can be safely committed and, if so, I'll do it. |
|
Marking for dev meeting, assigning to
|
|
In #242 (comment), @jbphet said:
In #242 (comment), @jbphet said:
It sounds like you are referring to the code below. Please provide details on why setting removeTermFromPlate: function( term ) {
assert && assert( this.allTerms.contains( term ), 'term not found: ' + term );
assert && assert( this.termsOnPlate.contains( term ), 'term not on plate: ' + term );
// ORDER IS VERY IMPORTANT HERE!
var cell = this.plate.removeTerm( term );
this.termsOnPlate.remove( term );
403 term.onPlateProperty.value = false;
return cell;
}, |
|
@jbphet explained equality-explorer to me in person. 403 if ( !term.onPlateProperty.isDisposed ) {
term.onPlateProperty.value = false;
} |
|
Discussed in dev meeting today. We will change the usages stated above like @jbphet recommended, assigning to the appropriate humans. The last person can either add the assertion in @samreid can't help but to think that we have pondered this before, and run into an edge case where we don't want a strict assertion like this. For now we will continue, with the caveat that we may bail if we remember a deal breaking edge case. |
|
equality-explorer and equality-explorer-two-variables are done. |
|
I committed the changes to the order of disposal actions in the button views, and so far continuous testing seems to be okay. This should fix Unit Rates and CCK. Up to @jonathanolson to track things down for the fractions suite. |
|
Fixed up fractions (and as I was the last, I added the assertion). @zepumph anything else to do here? |
|
Looking at CT like the assertion was commited... equality-explorer-two-variables is still having problems. @jbphet gene-expression-essential is also having problems. (@pixelzoom edit: created issue phetsims/gene-expression-essentials#127) |
|
@zepumph I've created sim-specific issue for the sims noted in #242 (comment), so if you want to close this issue, that's fine with me. |
|
The last piece of this is to actually add the assertion in once everything is done. I'll keep this open to make sure that happens. |
|
@jonathanolson said in #242 (comment) that he did add the assertion, and @pixelzoom confirmed in #242 (comment). Looking at master, I can see the assertion is still there. @zepumph anything else for this issue? |
|
I'm sorry! I found myself glancing at the dispose method instead of the emit method. Ready to close. Thanks your patience. |
From @pixelzoom review in #222, it would be nice to add this assertion to
TinyEmitter.emit().assert && assert( !this.isDisposed, 'should not be called if disposed' );When I do this, a number of sims fail:
These don't always appear, so there may be more. For example CCK took a few minutes of fuzzing to fail, but then it failed with:
I'm pretty sure that this is the sort of assertion that we would want to have in TinyEmitter, but I don't want to add this until these usages are fixed, and it's not obvious to me how these should be fixed. Marking for dev meeting to see if we want to fix these, and how to go about doing it.
UPDATE:
Adding on to this issue, it seems like we should also add something like the following in
dispose:assert && assert( this.activeListenersStack.length === 0, 'should not dispose an emitter while firing its listeners' );@jonathanolson could you review the two proposed lines of code and let me know what you think.
The text was updated successfully, but these errors were encountered: