Kamil/fix abort controllers memory leaks (#5561)

* chore: fix memory leaks * chore: add changelog * chore: add tests * chore: cleanup, pr review fixes * chore: fix code hygiene * chore: pr review --------- Co-authored-by: Harel M <[email protected]>
maplibre · Feb 28, 2025 · 18beec0 · 18beec0
1 parent 67b7f03
commit 18beec0
Show file tree

Hide file tree

Showing 5 changed files with 254 additions and 32 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,7 +7,8 @@
 
 ### 🐞 Bug fixes
 
-- Remove closeButton event listener on popup.remove().
+- Fix AbortController signal listener memory leak in frameAsync and sendAsync. ([#5561](https://github.com/maplibre/maplibre-gl-js/pull/5561))
+- Remove closeButton event listener on popup.remove(). ([#5564](https://github.com/maplibre/maplibre-gl-js/pull/5564))
 
 ## 5.1.1
 

diff --git a/src/util/actor.test.ts b/src/util/actor.test.ts
@@ -6,6 +6,79 @@ import {ABORT_ERROR, createAbortError} from './abort_error';
 import {MessageType} from './actor_messages';
 
 describe('Actor', () => {
+    test('removes "abort" event listener from signal on reject', async () => {
+        const worker = workerFactory() as any as WorkerGlobalScopeInterface & ActorTarget;
+
+        worker.worker.actor.registerMessageHandler(MessageType.getClusterExpansionZoom, () => {
+            return Promise.reject(new Error('some error'));
+        });
+
+        const actor = new Actor(worker, 'test-map-id');
+
+        const abortController = new AbortController();
+        const addSpy = vi.spyOn(abortController.signal, 'addEventListener');
+        const removeSpy = vi.spyOn(abortController.signal, 'removeEventListener');
+
+        await expect(
+            actor.sendAsync({
+                type: MessageType.getClusterExpansionZoom,
+                data: {type: 'geojson', source: '', clusterId: 1729}
+            }, abortController)
+        ).rejects.toThrow('some error');
+
+        expect(addSpy).toHaveBeenCalledTimes(1);
+        expect(removeSpy).toHaveBeenCalledTimes(1);
+        expect(removeSpy).toHaveBeenCalledWith('abort', expect.any(Function), expect.anything());
+    });
+
+    test('removes "abort" event listener from signal on abort', async () => {
+        const worker = workerFactory() as any as WorkerGlobalScopeInterface & ActorTarget;
+
+        worker.worker.actor.registerMessageHandler(MessageType.getClusterExpansionZoom, () => {
+            return new Promise(() => {});
+        });
+
+        const actor = new Actor(worker, 'test-map-id');
+
+        const abortController = new AbortController();
+        const addSpy = vi.spyOn(abortController.signal, 'addEventListener');
+        const removeSpy = vi.spyOn(abortController.signal, 'removeEventListener');
+
+        actor.sendAsync({
+            type: MessageType.getClusterExpansionZoom,
+            data: {type: 'geojson', source: '', clusterId: 1729}
+        }, abortController);
+
+        expect(addSpy).toHaveBeenCalledTimes(1);
+
+        abortController.abort();
+
+        await sleep(0);
+
+        expect(removeSpy).toHaveBeenCalledTimes(1);
+        expect(removeSpy).toHaveBeenCalledWith('abort', expect.any(Function), expect.anything());
+    });
+
+    test('removes "abort" event listener from signal after request completes', async () => {
+        const worker = workerFactory() as any as WorkerGlobalScopeInterface & ActorTarget;
+        worker.worker.actor.registerMessageHandler(MessageType.getClusterExpansionZoom, () => Promise.resolve(0));
+
+        const actor = new Actor(worker, 'test-map-id');
+
+        const abortController = new AbortController();
+        const addSpy = vi.spyOn(abortController.signal, 'addEventListener');
+        const removeSpy = vi.spyOn(abortController.signal, 'removeEventListener');
+
+        const result = await actor.sendAsync({
+            type: MessageType.getClusterExpansionZoom,
+            data: {type: 'geojson', source: '', clusterId: 1729}
+        }, abortController);
+
+        expect(result).toBe(0);
+
+        expect(addSpy).toHaveBeenCalledTimes(1);
+        expect(removeSpy).toHaveBeenCalledTimes(1);
+    });
 
     test('forwards responses to correct handler', async () => {
         const worker = workerFactory() as any as WorkerGlobalScopeInterface & ActorTarget;

diff --git a/src/util/actor.ts b/src/util/actor.ts
@@ -45,6 +45,8 @@ export interface IActor {
 
 export type MessageHandler<T extends MessageType> = (mapId: string | number, params: RequestResponseMessageMap[T][0], abortController?: AbortController) => Promise<RequestResponseMessageMap[T][1]>;
 
+const addEventDefaultOptions: AddEventListenerOptions = {once: true};
+
 /**
  * An implementation of the [Actor design pattern](https://en.wikipedia.org/wiki/Actor_model)
  * that maintains the relationship between asynchronous tasks and the objects
@@ -99,24 +101,32 @@ export class Actor implements IActor {
             // message from multiple other actors which could run in different execution context. A
             // linearly increasing ID could produce collisions.
             const id = Math.round((Math.random() * 1e18)).toString(36).substring(0, 10);
+
+            const subscription =  abortController ? subscribe(abortController.signal, 'abort', () => {
+                subscription?.unsubscribe();
+                delete this.resolveRejects[id];
+                const cancelMessage: MessageData = {
+                    id,
+                    type: '<cancel>',
+                    origin: location.origin,
+                    targetMapId: message.targetMapId,
+                    sourceMapId: this.mapId
+                };
+                this.target.postMessage(cancelMessage);
+                // In case of abort the current behavior is to keep the promise pending.
+            }, addEventDefaultOptions) : null;
+
             this.resolveRejects[id] = {
-                resolve,
-                reject
+                resolve: (value) => {
+                    subscription?.unsubscribe();
+                    resolve(value);
+                },
+                reject: (reason)=> {
+                    subscription?.unsubscribe();
+                    reject(reason);
+                }
             };
-            if (abortController) {
-                abortController.signal.addEventListener('abort', () => {
-                    delete this.resolveRejects[id];
-                    const cancelMessage: MessageData = {
-                        id,
-                        type: '<cancel>',
-                        origin: location.origin,
-                        targetMapId: message.targetMapId,
-                        sourceMapId: this.mapId
-                    };
-                    this.target.postMessage(cancelMessage);
-                    // In case of abort the current behavior is to keep the promise pending.
-                }, {once: true});
-            }
+
             const buffers: Array<Transferable> = [];
             const messageToPost: MessageData = {
                 ...message,

diff --git a/src/util/browser.test.ts b/src/util/browser.test.ts
@@ -1,21 +1,153 @@
-import {describe, test, expect} from 'vitest';
+import {describe, test, expect, beforeEach, vi, afterEach, type Mock} from 'vitest';
 import {browser} from './browser';
 
 describe('browser', () => {
-    test('frameAsync', async () => {
-        const id = await browser.frameAsync(new AbortController());
-        expect(id).toBeTruthy();
+    describe('frame',() => {
+        let originalRAF: typeof window.requestAnimationFrame;
+        let originalCAF: typeof window.cancelAnimationFrame;
+        let rafCallbacks: Array<{id: number; callback: FrameRequestCallback}> = [];
+        let rafIdCounter = 0;
+
+        /** Mimic scheduling RAFs for later */
+        function flushAllRAFs() {
+            const pending = [...rafCallbacks];
+            rafCallbacks = [];
+            pending.forEach(({callback}) => callback(performance.now()));
+        }
+
+        beforeEach(() => {
+            originalRAF = window.requestAnimationFrame;
+            originalCAF = window.cancelAnimationFrame;
+            rafCallbacks = [];
+            rafIdCounter = 0;
+            vi.spyOn(window, 'requestAnimationFrame').mockImplementation(cb => {
+                rafIdCounter++;
+                const id = rafIdCounter;
+                rafCallbacks.push({id, callback: cb});
+                return id;
+            });
+            vi.spyOn(window, 'cancelAnimationFrame').mockImplementation(id => {
+                rafCallbacks = rafCallbacks.filter(entry => entry.id !== id);
+            });
+        });
+
+        afterEach(() => {
+            window.requestAnimationFrame = originalRAF;
+            window.cancelAnimationFrame = originalCAF;
+            vi.restoreAllMocks();
+        });
+
+        test('calls requestAnimationFrame and invokes fn callback with timestamp', () => {
+            const abortController = new AbortController();
+            const addListenerSpy = vi.spyOn(abortController.signal, 'addEventListener');
+            const removeListenerSpy = vi.spyOn(abortController.signal, 'removeEventListener');
+
+            const fn = vi.fn();
+            const reject = vi.fn();
+
+            browser.frame(abortController, fn, reject);
+
+            expect(window.requestAnimationFrame).toHaveBeenCalledTimes(1);
+
+            flushAllRAFs();
+
+            expect(fn).toHaveBeenCalledTimes(1);
+            const callArg = fn.mock.calls[0][0];
+            expect(typeof callArg).toBe('number');
+
+            expect(window.cancelAnimationFrame).not.toHaveBeenCalled();
+            expect(reject).not.toHaveBeenCalled();
+
+            // cleanup leftover listeners
+            expect(addListenerSpy).toHaveBeenCalledWith('abort', expect.any(Function), false);
+            expect(removeListenerSpy).toHaveBeenCalledWith('abort', expect.any(Function), false);
+        });
+
+        test('when AbortController is aborted before frame fires, calls cancelAnimationFrame and reject', () => {
+            // We override the default mock so that the callback is NOT called immediately
+            // giving us time to abort.
+            (window.requestAnimationFrame as Mock).mockImplementation(
+                () => {
+                    // Return ID but do not invoke cb
+                    return 42;
+                }
+            );
+
+            const abortController = new AbortController();
+            const addListenerSpy = vi.spyOn(abortController.signal, 'addEventListener');
+            const removeListenerSpy = vi.spyOn(abortController.signal, 'removeEventListener');
+
+            const fn = vi.fn();
+            const reject = vi.fn();
+
+            browser.frame(abortController, fn, reject);
+
+            abortController.abort();
+
+            // Now we expect cancelAnimationFrame to be called with the ID 42
+            expect(window.cancelAnimationFrame).toHaveBeenCalledTimes(1);
+            expect(window.cancelAnimationFrame).toHaveBeenCalledWith(42);
+
+            // Expect reject to be called
+            expect(reject).toHaveBeenCalledTimes(1);
+            const errorArg = reject.mock.calls[0][0];
+            expect(errorArg).toBeInstanceOf(Error);
+            expect(errorArg.message).toMatch(/abort/i);
+
+            // fn should never have been called because we never triggered the RAF callback
+            expect(fn).not.toHaveBeenCalled();
+
+            // cleanup leftover listeners
+            expect(addListenerSpy).toHaveBeenCalledWith('abort', expect.any(Function), false);
+            expect(removeListenerSpy).toHaveBeenCalledWith('abort', expect.any(Function), false);
+        });
+
+        test('when AbortController is aborted after frame fires, fn is invoked anyway', () => {
+            const abortController = new AbortController();
+            const addListenerSpy = vi.spyOn(abortController.signal, 'addEventListener');
+            const removeListenerSpy = vi.spyOn(abortController.signal, 'removeEventListener');
+
+            const fn = vi.fn();
+            const reject = vi.fn();
+
+            browser.frame(abortController, fn, reject);
+
+            flushAllRAFs();
+
+            // The callback should have already been called
+            expect(fn).toHaveBeenCalledTimes(1);
+
+            // The callback runs immediately in our default mock
+            // so if we abort now, it's too late to cancel the frame
+            abortController.abort();
+
+            // Because callback already fired, there's no need to cancel
+            expect(window.cancelAnimationFrame).not.toHaveBeenCalled();
+            // And reject shouldn't be called either
+            expect(reject).not.toHaveBeenCalled();
+
+            // cleanup leftover listeners
+            expect(addListenerSpy).toHaveBeenCalledWith('abort', expect.any(Function), false);
+            expect(removeListenerSpy).toHaveBeenCalledWith('abort', expect.any(Function), false);
+        });
     });
 
-    test('now', () => {
-        expect(typeof browser.now()).toBe('number');
+    describe('frameAsync',()=>{
+        test('expect RAF to be called and receive RAF id', async () => {
+            const id = await browser.frameAsync(new AbortController());
+            expect(id).toBeTruthy();
+        });
+
+        test('throw error when abort is called', async () => {
+            const abortController = new AbortController();
+            const promise = browser.frameAsync(abortController);
+            abortController.abort();
+            await expect(promise).rejects.toThrow();
+        });
     });
 
-    test('frameAsync', async () => {
-        const abortController = new AbortController();
-        const promise = browser.frameAsync(abortController);
-        abortController.abort();
-        await expect(promise).rejects.toThrow();
+    test('now', () => {
+        expect(typeof browser.now()).toBe('number');
     });
 
     test('hardwareConcurrency', () => {

diff --git a/src/util/browser.ts b/src/util/browser.ts
@@ -1,4 +1,5 @@
 import {createAbortError} from './abort_error';
+import {subscribe} from './util';
 
 const now = typeof performance !== 'undefined' && performance && performance.now ?
     performance.now.bind(performance) :
@@ -17,11 +18,16 @@ export const browser = {
     now,
 
     frame(abortController: AbortController, fn: (paintStartTimestamp: number) => void, reject: (error: Error) => void): void {
-        const frame = requestAnimationFrame(fn);
-        abortController.signal.addEventListener('abort', () => {
-            cancelAnimationFrame(frame);
-            reject(createAbortError());
+        const frameId = requestAnimationFrame((paintStartTimestamp)=>{
+            unsubscribe();
+            fn(paintStartTimestamp);
         });
+
+        const {unsubscribe} = subscribe(abortController.signal, 'abort', () => {
+            unsubscribe();
+            cancelAnimationFrame(frameId);
+            reject(createAbortError());
+        }, false);
     },
 
     frameAsync(abortController: AbortController): Promise<number> {