From bd456fc69667b0cf2df73f6ed8815d3e52c1382c Mon Sep 17 00:00:00 2001 From: Matthew Amato Date: Mon, 21 Mar 2016 16:05:41 -0400 Subject: [PATCH 1/5] Allow EntityCollection events to be reentrant 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. --- Source/DataSources/EntityCollection.js | 22 +++++++++++++++++---- Specs/DataSources/EntityCollectionSpec.js | 24 +++++++++++++++++++++++ 2 files changed, 42 insertions(+), 4 deletions(-) diff --git a/Source/DataSources/EntityCollection.js b/Source/DataSources/EntityCollection.js index e4bb2beaec60..7873bb53fcb0 100644 --- a/Source/DataSources/EntityCollection.js +++ b/Source/DataSources/EntityCollection.js @@ -30,15 +30,27 @@ define([ }; function fireChangedEvent(collection) { + if (collection._firing) { + collection._refire = true; + return; + } + if (collection._suspendCount === 0) { var added = collection._addedEntities; var removed = collection._removedEntities; var changed = collection._changedEntities; if (changed.length !== 0 || added.length !== 0 || removed.length !== 0) { - collection._collectionChanged.raiseEvent(collection, added.values, removed.values, changed.values); - added.removeAll(); - removed.removeAll(); - changed.removeAll(); + do { + collection._refire = false; + var addedArray = collection._addedEntities.values.slice(0); + var removedArray = collection._removedEntities.values.slice(0); + var changedArray = collection._changedEntities.values.slice(0); + + added.removeAll(); + removed.removeAll(); + changed.removeAll(); + collection._collectionChanged.raiseEvent(collection, addedArray, removedArray, changedArray); + } while (collection._refire); } } } @@ -60,6 +72,8 @@ define([ this._collectionChanged = new Event(); this._id = createGuid(); this._show = true; + this._firing = false; + this._refire = false; } /** diff --git a/Specs/DataSources/EntityCollectionSpec.js b/Specs/DataSources/EntityCollectionSpec.js index 60ef44eda3c5..5375b4c7a8a8 100644 --- a/Specs/DataSources/EntityCollectionSpec.js +++ b/Specs/DataSources/EntityCollectionSpec.js @@ -135,6 +135,30 @@ defineSuite([ entityCollection.collectionChanged.removeEventListener(listener.onCollectionChanged, listener); }); + it('raises expected events when reentrant', function() { + var entityCollection = new EntityCollection(); + + var entity = new Entity(); + var entity2 = new Entity(); + entityCollection.add(entity); + entityCollection.add(entity2); + + var listener = jasmine.createSpy('listener').and.callFake(function(collection, added, removed, changed) { + //When we set the name to `newName` below, this code will modify entity2's name, thus triggering + //another event firing that occurs after all current subscribers have been notified of the + //event we are inside of. + if (entity2.name !== 'Bob') { + entity2.name = 'Bob'; + } + }); + entityCollection.collectionChanged.addEventListener(listener); + + entity.name = 'newName'; + expect(listener.calls.count()).toBe(2); + expect(listener.calls.argsFor(0)).toEqual([entityCollection, [], [], [entity]]); + expect(listener.calls.argsFor(1)).toEqual([entityCollection, [], [], [entity2]]); + }); + it('suspended add/remove raises expected events', function() { var entity = new Entity(); var entity2 = new Entity(); From 1800ecb2a51c1478bed59f625c46fe744dd1810c Mon Sep 17 00:00:00 2001 From: Matthew Amato Date: Mon, 21 Mar 2016 16:18:33 -0400 Subject: [PATCH 2/5] Update CHANGES. --- CHANGES.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGES.md b/CHANGES.md index c6c3ca0931d3..c4ccb9895b03 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -19,6 +19,7 @@ Change Log * Fix issue where the `GroundPrimitive` volume was being clipped by the far plane. [#3706](https://github.com/AnalyticalGraphicsInc/cesium/issues/3706) * Fixed issue where `Camera.computeViewRectangle` was incorrect when crossing the international date line [#3717](https://github.com/AnalyticalGraphicsInc/cesium/issues/3717) * Added `Rectangle` result parameter to `Camera.computeViewRectangle` +* Fixed a reentrancy bug in `EntityCollection.collectionChanged`. [#3739](https://github.com/AnalyticalGraphicsInc/cesium/pull/3739) * Fix bug when upsampling exaggerated terrain where the terrain heights were exaggerated at twice the value. [#3607](https://github.com/AnalyticalGraphicsInc/cesium/issues/3607) ### 1.19 - 2016-03-01 From a970507bbb05224a1cb420c743ee74389b47c1c1 Mon Sep 17 00:00:00 2001 From: Matthew Amato Date: Mon, 21 Mar 2016 16:35:35 -0400 Subject: [PATCH 3/5] Changes after review --- Source/DataSources/EntityCollection.js | 2 ++ Specs/DataSources/EntityCollectionSpec.js | 7 +++++++ 2 files changed, 9 insertions(+) diff --git a/Source/DataSources/EntityCollection.js b/Source/DataSources/EntityCollection.js index 7873bb53fcb0..5e45c29e3a76 100644 --- a/Source/DataSources/EntityCollection.js +++ b/Source/DataSources/EntityCollection.js @@ -40,6 +40,7 @@ define([ var removed = collection._removedEntities; var changed = collection._changedEntities; if (changed.length !== 0 || added.length !== 0 || removed.length !== 0) { + collection._firing = true; do { collection._refire = false; var addedArray = collection._addedEntities.values.slice(0); @@ -51,6 +52,7 @@ define([ changed.removeAll(); collection._collectionChanged.raiseEvent(collection, addedArray, removedArray, changedArray); } while (collection._refire); + collection._firing = false; } } } diff --git a/Specs/DataSources/EntityCollectionSpec.js b/Specs/DataSources/EntityCollectionSpec.js index 5375b4c7a8a8..b7cedd07c2f0 100644 --- a/Specs/DataSources/EntityCollectionSpec.js +++ b/Specs/DataSources/EntityCollectionSpec.js @@ -143,13 +143,20 @@ defineSuite([ entityCollection.add(entity); entityCollection.add(entity2); + var inCallback = false; var listener = jasmine.createSpy('listener').and.callFake(function(collection, added, removed, changed) { //When we set the name to `newName` below, this code will modify entity2's name, thus triggering //another event firing that occurs after all current subscribers have been notified of the //event we are inside of. + + //By checking that inCallback is false, we are making sure the entity2.name assignment + //is delayed until after the first round of events is fired. + expect(inCallback).toBe(false); + inCallback = true; if (entity2.name !== 'Bob') { entity2.name = 'Bob'; } + inCallback = false; }); entityCollection.collectionChanged.addEventListener(listener); From ed8cdddc70996e2cb676d8614ac20364080f3872 Mon Sep 17 00:00:00 2001 From: Matthew Amato Date: Mon, 21 Mar 2016 17:16:32 -0400 Subject: [PATCH 4/5] Re-use existing variables. --- Source/DataSources/EntityCollection.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Source/DataSources/EntityCollection.js b/Source/DataSources/EntityCollection.js index 5e45c29e3a76..d25b41c5fc1e 100644 --- a/Source/DataSources/EntityCollection.js +++ b/Source/DataSources/EntityCollection.js @@ -43,9 +43,9 @@ define([ collection._firing = true; do { collection._refire = false; - var addedArray = collection._addedEntities.values.slice(0); - var removedArray = collection._removedEntities.values.slice(0); - var changedArray = collection._changedEntities.values.slice(0); + var addedArray = added.values.slice(0); + var removedArray = removed.values.slice(0); + var changedArray = changed.values.slice(0); added.removeAll(); removed.removeAll(); From 5b57d9dae87e4f70c9ef275592b18147451e7573 Mon Sep 17 00:00:00 2001 From: Scott Hunter Date: Mon, 21 Mar 2016 17:42:41 -0400 Subject: [PATCH 5/5] Add additional tests for added/removed --- Specs/DataSources/EntityCollectionSpec.js | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/Specs/DataSources/EntityCollectionSpec.js b/Specs/DataSources/EntityCollectionSpec.js index b7cedd07c2f0..85ca583b8cb0 100644 --- a/Specs/DataSources/EntityCollectionSpec.js +++ b/Specs/DataSources/EntityCollectionSpec.js @@ -143,6 +143,11 @@ defineSuite([ entityCollection.add(entity); entityCollection.add(entity2); + var entityToDelete = new Entity(); + entityCollection.add(entityToDelete); + + var entityToAdd = new Entity(); + var inCallback = false; var listener = jasmine.createSpy('listener').and.callFake(function(collection, added, removed, changed) { //When we set the name to `newName` below, this code will modify entity2's name, thus triggering @@ -156,6 +161,12 @@ defineSuite([ if (entity2.name !== 'Bob') { entity2.name = 'Bob'; } + if (entityCollection.contains(entityToDelete)) { + entityCollection.removeById(entityToDelete.id); + } + if (!entityCollection.contains(entityToAdd)) { + entityCollection.add(entityToAdd); + } inCallback = false; }); entityCollection.collectionChanged.addEventListener(listener); @@ -163,7 +174,12 @@ defineSuite([ entity.name = 'newName'; expect(listener.calls.count()).toBe(2); expect(listener.calls.argsFor(0)).toEqual([entityCollection, [], [], [entity]]); - expect(listener.calls.argsFor(1)).toEqual([entityCollection, [], [], [entity2]]); + expect(listener.calls.argsFor(1)).toEqual([entityCollection, [entityToAdd], [entityToDelete], [entity2]]); + + expect(entity.name).toEqual('newName'); + expect(entity2.name).toEqual('Bob'); + expect(entityCollection.contains(entityToDelete)).toEqual(false); + expect(entityCollection.contains(entityToAdd)).toEqual(true); }); it('suspended add/remove raises expected events', function() {