From 88a56ddccf697497a0f3edf5593df5569111156d Mon Sep 17 00:00:00 2001 From: Emanuele Peruffo Date: Sat, 7 Oct 2023 12:34:34 +0200 Subject: [PATCH 1/7] Fix ignored additional callback arguments --- src/util/actor.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/actor.ts b/src/util/actor.ts index 5bdc7d3291..aa43f3f5c6 100644 --- a/src/util/actor.ts +++ b/src/util/actor.ts @@ -213,13 +213,13 @@ export class Actor { if (task.error) { callback(deserialize(task.error)); } else { - callback(null, deserialize(task.data)); + callback(null, ...(deserialize(task.data) as any[])); } } } else { let completed = false; const buffers: Array = []; - const done = task.hasCallback ? (err: Error, data?: any) => { + const done = task.hasCallback ? (err: Error, ...data: any[]) => { completed = true; delete this.cancelCallbacks[id]; const responseMessage: MessageData = { From 81178e96e6d5b9266df4c2dfab812e229f85d034 Mon Sep 17 00:00:00 2001 From: Emanuele Peruffo Date: Tue, 10 Oct 2023 08:44:06 +0200 Subject: [PATCH 2/7] Add changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8c260176d8..214d3893b3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### 🐞 Bug fixes - _...Add new stuff here..._ +- Fix actor passing only the first 2 arguments to callbacks ## 3.5.2 From 1c083fb1e99abdfac2e30ad50f2b43753da1f293 Mon Sep 17 00:00:00 2001 From: Emanuele Peruffo Date: Tue, 10 Oct 2023 10:34:35 +0200 Subject: [PATCH 3/7] Add unit test --- src/util/actor.test.ts | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/src/util/actor.test.ts b/src/util/actor.test.ts index 06ad375df5..0f27e73b0d 100644 --- a/src/util/actor.test.ts +++ b/src/util/actor.test.ts @@ -63,6 +63,32 @@ describe('Actor', () => { }); }); + test('calls the callback with the correct number of arguments', done => { + setTestWorker(class MockWorker { + self: any; + actor: Actor; + constructor(self) { + this.self = self; + this.actor = new Actor(self, this); + } + test(mapId, params, callback) { + setTimeout(callback, 0, null, '1st param', '2nd param', '3rd param'); + } + }); + + const worker = workerFactory(); + + const m = new Actor(worker, {}, '1'); + + m.send('test', {value: 1729}, (err, param1, param2, param3) => { + expect(err).toBeFalsy(); + expect(param1).toBe('1st param'); + expect(param2).toBe('2nd param'); + expect(param3).toBe('3rd param'); + done(); + }); + }); + test('targets worker-initiated messages to correct map instance', done => { let workerActor; From 15148f5cc257cb97e2d416d3bdeda8f00546548f Mon Sep 17 00:00:00 2001 From: Emanuele Peruffo Date: Thu, 12 Oct 2023 09:41:27 +0200 Subject: [PATCH 4/7] Test reload of expired tiles --- test/integration/browser/browser.test.ts | 61 ++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/test/integration/browser/browser.test.ts b/test/integration/browser/browser.test.ts index b0cb7f5885..32c08d4caa 100644 --- a/test/integration/browser/browser.test.ts +++ b/test/integration/browser/browser.test.ts @@ -248,4 +248,65 @@ describe('Browser tests', () => { expect(markerScreenPosition.x).toBeCloseTo(386.5); expect(markerScreenPosition.y).toBeCloseTo(378.1); }, 20000); + + test('Expired Tile Reload', async() => { + const callCountByUrl = await page.evaluate(() => { + const CACHE_SECS = 1; + const TILES_URL = 'https://demotiles.maplibre.org/tiles/:z/:x/:y.pbf'; + + const callCountByUrl: Record = {}; + + maplibregl.addProtocol('cust', ({url}, callback) => { + const [x, y, z] = url.split('//')[1].split('-'); + + callCountByUrl[url] = (callCountByUrl[url] ?? 0) + 1; + + const cacheControl = `no-store,max-age=${CACHE_SECS}`; + const expires = new Date(Date.now() + CACHE_SECS * 1000).toUTCString(); + + const abortController = new AbortController(); + + const tileUrl = TILES_URL.replace(':z', z).replace(':x', x).replace(':y', y); + + fetch(tileUrl, { + signal: abortController.signal, + }) + .then((response) => response.arrayBuffer()) + .then((data) => { + callback(undefined, data, cacheControl, expires); + }) + .catch((err) => { + callback(err); + }); + + return { + cancel: () => { + abortController.abort(); + }, + }; + }); + + return new Promise>((resolve, reject) => { + map.addSource('cust', { + type: 'vector', + tiles: ['cust://{x}-{y}-{z}'], + }); + + map.addLayer({ + id: 'cust', + type: 'circle', + source: 'cust', + 'source-layer': 'centroids', + }); + + setTimeout(() => { + resolve(callCountByUrl); + }, 5000); + }); + }); + + for (const [, count] of Object.entries(callCountByUrl)) { + expect(count).toBeGreaterThan(1); + } + }, 20000); }); From 0a3f4f9cc05662c6543f7d68c71fab531c93be94 Mon Sep 17 00:00:00 2001 From: Emanuele Peruffo Date: Thu, 12 Oct 2023 09:47:33 +0200 Subject: [PATCH 5/7] Remove unused reject --- test/integration/browser/browser.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/browser/browser.test.ts b/test/integration/browser/browser.test.ts index 32c08d4caa..c2a6b0b1b4 100644 --- a/test/integration/browser/browser.test.ts +++ b/test/integration/browser/browser.test.ts @@ -286,7 +286,7 @@ describe('Browser tests', () => { }; }); - return new Promise>((resolve, reject) => { + return new Promise>((resolve) => { map.addSource('cust', { type: 'vector', tiles: ['cust://{x}-{y}-{z}'], From d07df515efe704da14895a8405b15d7d778d5d35 Mon Sep 17 00:00:00 2001 From: Emanuele Peruffo Date: Fri, 13 Oct 2023 16:28:31 +0200 Subject: [PATCH 6/7] Add PR link --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 214d3893b3..bb8525d03c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ ### 🐞 Bug fixes - _...Add new stuff here..._ -- Fix actor passing only the first 2 arguments to callbacks +- Fixed actor passing only the first 2 arguments to callbacks ([#3130](https://github.com/maplibre/maplibre-gl-js/pull/3196)) ## 3.5.2 From 6d7c9b6ef0a18c9e4733508978fa3ff82a1b8008 Mon Sep 17 00:00:00 2001 From: Emanuele Peruffo Date: Wed, 25 Oct 2023 12:57:10 +0000 Subject: [PATCH 7/7] Improve types for actor --- src/util/actor.ts | 77 ++++++++++++++++++++++++----------------------- 1 file changed, 40 insertions(+), 37 deletions(-) diff --git a/src/util/actor.ts b/src/util/actor.ts index aa43f3f5c6..e4e3beb2a7 100644 --- a/src/util/actor.ts +++ b/src/util/actor.ts @@ -31,26 +31,34 @@ export interface GlyphsProvider { ); } -export type MessageType = '' | '' | -'geojson.getClusterExpansionZoom' | 'geojson.getClusterChildren' | 'geojson.getClusterLeaves' | 'geojson.loadData' | +export type MessageType = 'geojson.getClusterExpansionZoom' | 'geojson.getClusterChildren' | +'geojson.getClusterLeaves' | 'geojson.loadData' | 'removeSource' | 'loadWorkerSource' | 'loadDEMTile' | 'removeDEMTile' | 'removeTile' | 'reloadTile' | 'abortTile' | 'loadTile' | 'getTile' | 'getGlyphs' | 'getImages' | 'setImages' | 'syncRTLPluginState' | 'setReferrer' | 'setLayers' | 'updateLayers'; -export type MessageData = { +type BasicMessage = { id: string; + targetMapId?: string | null; + sourceMapId: string | number | null; +} + +type RequestMessage = BasicMessage & { type: MessageType; - data?: Serialized; - targetMapId?: string | number | null; - mustQueue?: boolean; + hasCallback: boolean; + mustQueue: boolean; + params: Serialized; +} + +type ResponseMessage = BasicMessage & { + type: ''; error?: Serialized | null; - hasCallback?: boolean; - sourceMapId: string | number | null; + callbackArgs: Array; } -export type Message = { - data: MessageData; +type CancelMessage = BasicMessage & { + type: ''; } /** @@ -59,15 +67,16 @@ export type Message = { * that spin them off - in this case, tasks like parsing parts of styles, * owned by the styles */ + export class Actor { target: ActorTarget; parent: WorkerSourceProvider | GlyphsProvider; mapId: string | number | null; - callbacks: { [x: number]: Function}; + callbacks: Record = {}; name: string; - tasks: { [x: number]: MessageData }; - taskQueue: Array; - cancelCallbacks: { [x: number]: () => void }; + tasks: Record = {}; + taskQueue: Array = []; + cancelCallbacks: Record = {}; invoker: ThrottledInvoker; globalScope: ActorTarget; @@ -80,10 +89,6 @@ export class Actor { this.target = target; this.parent = parent; this.mapId = mapId; - this.callbacks = {}; - this.tasks = {}; - this.taskQueue = []; - this.cancelCallbacks = {}; this.invoker = new ThrottledInvoker(this.process); this.target.addEventListener('message', this.receive, false); this.globalScope = isWorker() ? target : window; @@ -98,7 +103,7 @@ export class Actor { */ send( type: MessageType, - data: unknown, + params: unknown, callback?: Function | null, targetMapId?: string | null, mustQueue: boolean = false @@ -112,35 +117,34 @@ export class Actor { this.callbacks[id] = callback; } const buffers: Array = []; - const message: MessageData = { + this.target.postMessage({ id, type, hasCallback: !!callback, targetMapId, mustQueue, sourceMapId: this.mapId, - data: serialize(data, buffers) - }; + params: serialize(params, buffers) + } satisfies RequestMessage, {transfer: buffers}); - this.target.postMessage(message, {transfer: buffers}); return { cancel: () => { if (callback) { // Set the callback to null so that it never fires after the request is aborted. delete this.callbacks[id]; } - const cancelMessage: MessageData = { + + this.target.postMessage({ id, type: '', targetMapId, sourceMapId: this.mapId - }; - this.target.postMessage(cancelMessage); + } satisfies CancelMessage); } }; } - receive = (message: Message) => { + receive = (message: { data: RequestMessage | ResponseMessage | CancelMessage }) => { const data = message.data; const id = data.id; @@ -163,7 +167,7 @@ export class Actor { cancel(); } } else { - if (isWorker() || data.mustQueue) { + if (isWorker() || data.type !== '' && data.mustQueue) { // In workers, store the tasks that we need to process before actually processing them. This // is necessary because we want to keep receiving messages, and in particular, // messages. Some tasks may take a while in the worker thread, so before @@ -202,7 +206,7 @@ export class Actor { this.processTask(id, task); }; - processTask(id: string, task: MessageData) { + processTask(id: string, task: RequestMessage | ResponseMessage) { if (task.type === '') { // The done() function in the counterpart has been called, and we are now // firing the callback in the originating actor, if there is one. @@ -213,29 +217,28 @@ export class Actor { if (task.error) { callback(deserialize(task.error)); } else { - callback(null, ...(deserialize(task.data) as any[])); + callback(null, ...(task.callbackArgs.map(deserialize))); } } } else { let completed = false; const buffers: Array = []; - const done = task.hasCallback ? (err: Error, ...data: any[]) => { + const done = task.hasCallback ? (err: Error, ...callbackArgs: unknown[]) => { completed = true; delete this.cancelCallbacks[id]; - const responseMessage: MessageData = { + this.target.postMessage({ id, type: '', sourceMapId: this.mapId, error: err ? serialize(err) : null, - data: serialize(data, buffers) - }; - this.target.postMessage(responseMessage, {transfer: buffers}); + callbackArgs: callbackArgs.map(data => serialize(data, buffers)) + } satisfies ResponseMessage, {transfer: buffers}); } : (_) => { completed = true; }; - let callback: Cancelable = null; - const params = deserialize(task.data); + let callback: Cancelable | null = null; + const params = deserialize(task.params); if (this.parent[task.type]) { // task.type == 'loadTile', 'removeTile', etc. callback = this.parent[task.type](task.sourceMapId, params, done);