Cancel pending GeoJSON requests when GeoJSONSource.setData() is called (#1102)

vanilla-lake · web-flow · commit 1ab71b15bbf0 · 2022-05-12T16:55:10.000+03:00
* Cancel pending GeoJSON requests when GeoJSONSource.setData() is called

* Add changelog entry

* Improve unit tests

* Add link to PR in changelog entry
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,7 @@
 
 ### ✨ Features and improvements
 
+- Cancel pending GeoJSON requests when `GeoJSONSource.setData()` is called instead of waiting for any pending request to complete before issuing the request for the new URL (#1102)
 - *...Add new stuff here...*
 
 ### 🐞 Bug fixes
diff --git a/src/source/geojson_source.test.ts b/src/source/geojson_source.test.ts
@@ -90,6 +90,18 @@ describe('GeoJSONSource#setData', () => {
         source.load();
     });
 
+    test('fires "dataabort" event', done => {
+        const source = new GeoJSONSource('id', {} as any, wrapDispatcher({
+            send(type, data, callback) {
+                setTimeout(() => callback(null, {abandoned: true}));
+            }
+        }), undefined);
+        source.on('dataabort', () => {
+            done();
+        });
+        source.load();
+    });
+
     test('respects collectResourceTiming parameter on source', done => {
         const source = createSource({collectResourceTiming: true});
         source.map = {
@@ -137,6 +149,19 @@ describe('GeoJSONSource#setData', () => {
         });
         source.setData({} as GeoJSON.GeoJSON);
     });
+
+    test('marks source as loaded before firing "dataabort" event', done => {
+        const source = new GeoJSONSource('id', {} as any, wrapDispatcher({
+            send(type, data, callback) {
+                setTimeout(() => callback(null, {abandoned: true}));
+            }
+        }), undefined);
+        source.on('dataabort', () => {
+            expect(source.loaded()).toBeTruthy();
+            done();
+        });
+        source.setData({} as GeoJSON.GeoJSON);
+    });
 });
 
 describe('GeoJSONSource#onRemove', () => {
diff --git a/src/source/geojson_source.ts b/src/source/geojson_source.ts
@@ -256,20 +256,13 @@ class GeoJSONSource extends Evented implements Source {
             this._pendingLoads--;
 
             if (this._removed || (result && result.abandoned)) {
+                this.fire(new Event('dataabort', {dataType: 'source', sourceDataType}));
                 return;
             }
 
             let resourceTiming = null;
             if (result && result.resourceTiming && result.resourceTiming[this.id])
                 resourceTiming = result.resourceTiming[this.id].slice(0);
-            // Any `loadData` calls that piled up while we were processing
-            // this one will get coalesced into a single call when this
-            // 'coalesce' message is processed.
-            // We would self-send from the worker if we had access to its
-            // message queue. Waiting instead for the 'coalesce' to round-trip
-            // through the foreground just means we're throttling the worker
-            // to run at a little less than full-throttle.
-            this.actor.send(`${this.type}.coalesce`, {source: options.source}, null);
 
             if (err) {
                 this.fire(new ErrorEvent(err));
diff --git a/src/source/geojson_worker_source.test.ts b/src/source/geojson_worker_source.test.ts
@@ -46,7 +46,6 @@ describe('reloadTile', () => {
 
         function addData(callback) {
             source.loadData({source: 'sourceId', data: JSON.stringify(geoJson)} as LoadGeoJSONParameters, (err) => {
-                source.coalesce();
                 expect(err).toBeNull();
                 callback();
             });
@@ -134,7 +133,10 @@ describe('resourceTiming', () => {
         window.performance.getEntriesByName = jest.fn().mockReturnValue([ exampleResourceTiming ]);
 
         const layerIndex = new StyleLayerIndex(layers);
-        const source = new GeoJSONWorkerSource(actor, layerIndex, [], (params, callback) => { return callback(null, geoJson); });
+        const source = new GeoJSONWorkerSource(actor, layerIndex, [], (params, callback) => {
+            callback(null, geoJson);
+            return {cancel: () => {}};
+        });
 
         source.loadData({source: 'testSource', request: {url: 'http://localhost/nonexistent', collectResourceTiming: true}} as LoadGeoJSONParameters, (err, result) => {
             expect(err).toBeNull();
@@ -166,7 +168,10 @@ describe('resourceTiming', () => {
         jest.spyOn(perf, 'clearMeasures').mockImplementation(() => { return null; });
 
         const layerIndex = new StyleLayerIndex(layers);
-        const source = new GeoJSONWorkerSource(actor, layerIndex, [], (params, callback) => { return callback(null, geoJson); });
+        const source = new GeoJSONWorkerSource(actor, layerIndex, [], (params, callback) => {
+            callback(null, geoJson);
+            return {cancel: () => {}};
+        });
 
         source.loadData({source: 'testSource', request: {url: 'http://localhost/nonexistent', collectResourceTiming: true}} as LoadGeoJSONParameters, (err, result) => {
             expect(err).toBeNull();
@@ -221,55 +226,46 @@ describe('loadData', () => {
         // (regardless of timing)
         const originalLoadGeoJSON = worker.loadGeoJSON;
         worker.loadGeoJSON = function(params, callback) {
-            setTimeout(() => {
+            const timeout = setTimeout(() => {
                 originalLoadGeoJSON(params, callback);
             }, 0);
+
+            return {cancel: () => clearTimeout(timeout)};
         };
         return worker;
     }
 
-    test('abandons coalesced callbacks', done => {
-        // Expect first call to run, second to be abandoned,
-        // and third to run in response to coalesce
+    test('abandons previous callbacks', done => {
         const worker = createWorker();
-        worker.loadData({source: 'source1', data: JSON.stringify(geoJson)} as LoadGeoJSONParameters, (err, result) => {
-            expect(err).toBeNull();
-            expect(result && result.abandoned).toBeFalsy();
-            worker.coalesce();
-        });
+        let firstCallbackHasRun = false;
 
         worker.loadData({source: 'source1', data: JSON.stringify(geoJson)} as LoadGeoJSONParameters, (err, result) => {
             expect(err).toBeNull();
             expect(result && result.abandoned).toBeTruthy();
+            firstCallbackHasRun = true;
         });
 
         worker.loadData({source: 'source1', data: JSON.stringify(geoJson)} as LoadGeoJSONParameters, (err, result) => {
             expect(err).toBeNull();
             expect(result && result.abandoned).toBeFalsy();
+            expect(firstCallbackHasRun).toBeTruthy();
             done();
         });
     });
 
     test('removeSource aborts callbacks', done => {
-        // Expect:
-        // First loadData starts running before removeSource arrives
-        // Second loadData is pending when removeSource arrives, gets cancelled
-        // removeSource is executed immediately
-        // First loadData finishes running, sends results back to foreground
         const worker = createWorker();
-        worker.loadData({source: 'source1', data: JSON.stringify(geoJson)} as LoadGeoJSONParameters, (err, result) => {
-            expect(err).toBeNull();
-            expect(result && result.abandoned).toBeFalsy();
-            done();
-        });
-
+        let loadDataCallbackHasRun = false;
         worker.loadData({source: 'source1', data: JSON.stringify(geoJson)} as LoadGeoJSONParameters, (err, result) => {
             expect(err).toBeNull();
             expect(result && result.abandoned).toBeTruthy();
+            loadDataCallbackHasRun = true;
         });
 
         worker.removeSource({source: 'source1'}, (err) => {
             expect(err).toBeFalsy();
+            expect(loadDataCallbackHasRun).toBeTruthy();
+            done();
         });
 
     });
diff --git a/src/source/geojson_worker_source.ts b/src/source/geojson_worker_source.ts
@@ -21,6 +21,7 @@ import type StyleLayerIndex from '../style/style_layer_index';
 import type {LoadVectorDataCallback} from './vector_tile_worker_source';
 import type {RequestParameters, ResponseCallback} from '../util/ajax';
 import type {Callback} from '../types/callback';
+import type {Cancelable} from '../types/cancelable';
 
 export type LoadGeoJSONParameters = {
     request?: RequestParameters;
@@ -33,7 +34,7 @@ export type LoadGeoJSONParameters = {
     filter?: Array<unknown>;
 };
 
-export type LoadGeoJSON = (params: LoadGeoJSONParameters, callback: ResponseCallback<any>) => void;
+export type LoadGeoJSON = (params: LoadGeoJSONParameters, callback: ResponseCallback<any>) => Cancelable;
 
 export interface GeoJSONIndex {
     getTile(z: number, x: number, y: number): any;
@@ -72,10 +73,6 @@ function loadGeoJSONTile(params: WorkerTileParameters, callback: LoadVectorDataC
     });
 }
 
-export type SourceState = // Source empty or data loaded
-'Idle' | // Data finished loading, but discard 'loadData' messages until receiving 'coalesced'
-'Coalescing' | 'NeedsLoadData';  // 'loadData' received while coalescing, trigger one more 'loadData' on receiving 'coalesced'
-
 /**
  * The {@link WorkerSource} implementation that supports {@link GeoJSONSource}.
  * This class is designed to be easily reused to support custom source types
@@ -87,12 +84,11 @@ export type SourceState = // Source empty or data loaded
  * @private
  */
 class GeoJSONWorkerSource extends VectorTileWorkerSource {
-    _state: SourceState;
     _pendingCallback: Callback<{
         resourceTiming?: {[_: string]: Array<PerformanceResourceTiming>};
         abandoned?: boolean;
     }>;
-    _pendingLoadDataParams: LoadGeoJSONParameters;
+    _pendingRequest: Cancelable;
     _geoJSONIndex: GeoJSONIndex;
 
     /**
@@ -117,9 +113,8 @@ class GeoJSONWorkerSource extends VectorTileWorkerSource {
      * expecting `callback(error, data)` to be called with either an error or a
      * parsed GeoJSON object.
      *
-     * When `loadData` requests come in faster than they can be processed,
-     * they are coalesced into a single request using the latest data.
-     * See {@link GeoJSONWorkerSource#coalesce}
+     * When a `loadData` request comes in while a previous one is being processed,
+     * the previous one is aborted.
      *
      * @param params
      * @param callback
@@ -129,40 +124,20 @@ class GeoJSONWorkerSource extends VectorTileWorkerSource {
         resourceTiming?: {[_: string]: Array<PerformanceResourceTiming>};
         abandoned?: boolean;
     }>) {
+        this._pendingRequest?.cancel();
         if (this._pendingCallback) {
             // Tell the foreground the previous call has been abandoned
             this._pendingCallback(null, {abandoned: true});
         }
-        this._pendingCallback = callback;
-        this._pendingLoadDataParams = params;
-
-        if (this._state &&
-            this._state !== 'Idle') {
-            this._state = 'NeedsLoadData';
-        } else {
-            this._state = 'Coalescing';
-            this._loadData();
-        }
-    }
-
-    /**
-     * Internal implementation: called directly by `loadData`
-     * or by `coalesce` using stored parameters.
-     */
-    _loadData() {
-        if (!this._pendingCallback || !this._pendingLoadDataParams) {
-            assert(false);
-            return;
-        }
-        const callback = this._pendingCallback;
-        const params = this._pendingLoadDataParams;
-        delete this._pendingCallback;
-        delete this._pendingLoadDataParams;
 
         const perf = (params && params.request && params.request.collectResourceTiming) ?
             new RequestPerformance(params.request) : false;
 
-        this.loadGeoJSON(params, (err?: Error | null, data?: any | null) => {
+        this._pendingCallback = callback;
+        this._pendingRequest = this.loadGeoJSON(params, (err?: Error | null, data?: any | null) => {
+            delete this._pendingCallback;
+            delete this._pendingRequest;
+
             if (err || !data) {
                 return callback(err);
             } else if (typeof data !== 'object') {
@@ -204,35 +179,6 @@ class GeoJSONWorkerSource extends VectorTileWorkerSource {
         });
     }
 
-    /**
-     * While processing `loadData`, we coalesce all further
-     * `loadData` messages into a single call to _loadData
-     * that will happen once we've finished processing the
-     * first message. {@link GeoJSONSource#_updateWorkerData}
-     * is responsible for sending us the `coalesce` message
-     * at the time it receives a response from `loadData`
-     *
-     *          State: Idle
-     *          ↑          |
-     *     'coalesce'   'loadData'
-     *          |     (triggers load)
-     *          |          ↓
-     *        State: Coalescing
-     *          ↑          |
-     *   (triggers load)   |
-     *     'coalesce'   'loadData'
-     *          |          ↓
-     *        State: NeedsLoadData
-     */
-    coalesce() {
-        if (this._state === 'Coalescing') {
-            this._state = 'Idle';
-        } else if (this._state === 'NeedsLoadData') {
-            this._state = 'Coalescing';
-            this._loadData();
-        }
-    }
-
     /**
     * Implements {@link WorkerSource#reloadTile}.
     *
@@ -264,24 +210,27 @@ class GeoJSONWorkerSource extends VectorTileWorkerSource {
      * @param params
      * @param [params.url] A URL to the remote GeoJSON data.
      * @param [params.data] Literal GeoJSON data. Must be provided if `params.url` is not.
+     * @returns {Cancelable} A Cancelable object.
      * @private
      */
-    loadGeoJSON(params: LoadGeoJSONParameters, callback: ResponseCallback<any>) {
+    loadGeoJSON(params: LoadGeoJSONParameters, callback: ResponseCallback<any>): Cancelable {
         // Because of same origin issues, urls must either include an explicit
         // origin or absolute path.
         // ie: /foo/bar.json or http://example.com/bar.json
         // but not ../foo/bar.json
         if (params.request) {
-            getJSON(params.request, callback);
+            return getJSON(params.request, callback);
         } else if (typeof params.data === 'string') {
             try {
-                return callback(null, JSON.parse(params.data));
+                callback(null, JSON.parse(params.data));
             } catch (e) {
-                return callback(new Error(`Input data given to '${params.source}' is not a valid GeoJSON object.`));
+                callback(new Error(`Input data given to '${params.source}' is not a valid GeoJSON object.`));
             }
         } else {
-            return callback(new Error(`Input data given to '${params.source}' is not a valid GeoJSON object.`));
+            callback(new Error(`Input data given to '${params.source}' is not a valid GeoJSON object.`));
         }
+
+        return {cancel: () => {}};
     }
 
     removeSource(params: {
diff --git a/src/ui/events.ts b/src/ui/events.ts
@@ -1395,7 +1395,7 @@ export type MapEvent =
     | 'style.load'
 
     /**
-     * Fired when a request for one of the map's sources' tiles is aborted.
+     * Fired when a request for one of the map's sources' data is aborted.
      * See {@link MapDataEvent} for more information.
      *
      * @event dataabort
@@ -1406,15 +1406,15 @@ export type MapEvent =
      * // Initialize the map
      * var map = new maplibregl.Map({ // map options });
      * // Set an event listener that fires
-     * // when a request for one of the map's sources' tiles is aborted.
+     * // when a request for one of the map's sources' data is aborted.
      * map.on('dataabort', function() {
      *   console.log('A dataabort event occurred.');
      * });
      */
     | 'dataabort'
 
     /**
-     * Fired when a request for one of the map's sources' tiles is aborted.
+     * Fired when a request for one of the map's sources' data is aborted.
      * See {@link MapDataEvent} for more information.
      *
      * @event sourcedataabort
@@ -1425,7 +1425,7 @@ export type MapEvent =
      * // Initialize the map
      * var map = new maplibregl.Map({ // map options });
      * // Set an event listener that fires
-     * // when a request for one of the map's sources' tiles is aborted.
+     * // when a request for one of the map's sources' data is aborted.
      * map.on('sourcedataabort', function() {
      *   console.log('A sourcedataabort event occurred.');
      * });
