Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow EntityCollection events to be reentrant #3739

Merged
merged 5 commits into from
Mar 21, 2016
Merged

Conversation

mramato
Copy link
Contributor

@mramato mramato commented Mar 21, 2016

The EntityCollection.collectionChanged event would have odd behavior if you modified an entity inside the collection from within a collectionChanged callback. The added/removed/changed entity would get appended to the current arrays being passed around and a new event would get raised immedediately which included duplicates of all current event parameters plus the new entity. At the same time, remaining handlers would ultimately end up seeing the second event but not the first.

This change makes the code rentrant safe and causes all reentrant events to be queued and then properly raised at the end of the current event cycle.

The `EntityCollection.collectionChanged` event would have odd behavior if
you modified an entity inside the collection from within a
`collectionChanged` callback.  The added/removed/changed entity would get
appended to the current arrays being passed around and a
new event would get raised immedediately which included duplicates of all
current event parameters plus the new entity.  At the same time, remaining
handlers would ultimately end up seeing the second event but not the first.

This change makes the code rentrant safe and causes all reentrant events
to be queued and then properly raised at the end of the current event
cycle.
@@ -30,15 +30,27 @@ define([
};

function fireChangedEvent(collection) {
if (collection._firing) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_firing is never set to true, as far as I can tell. I think you must be missing some tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops. What's weird is the test I wrote fails in master and passes in this branch (doing the right thing as far as I can tell). But what I'm not catching is checking that the second event is not fired until the first even is finished, which might be hard to test. I'll look into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, this should be fixed. I could add a more elaborate test if you think it's worth it that uses multiple spies to make sure things are called in the expected order.

collection._firing = true;
do {
collection._refire = false;
var addedArray = collection._addedEntities.values.slice(0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these should use the local variables already present above, added, removed, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

shunter added a commit that referenced this pull request Mar 21, 2016
Allow EntityCollection events to be reentrant
@shunter shunter merged commit 78db12e into master Mar 21, 2016
@shunter shunter deleted the reentrant-entity branch March 21, 2016 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants