From 18beec009a7fac24d7c2e461683fc8f01a99407c Mon Sep 17 00:00:00 2001 From: Kamil Sienkiewicz <138137613+kamil-sienkiewicz-asi@users.noreply.github.com> Date: Fri, 28 Feb 2025 16:15:49 +0100 Subject: [PATCH] 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 --- CHANGELOG.md | 3 +- src/util/actor.test.ts | 73 +++++++++++++++++++ src/util/actor.ts | 42 +++++++---- src/util/browser.test.ts | 154 ++++++++++++++++++++++++++++++++++++--- src/util/browser.ts | 14 +++- 5 files changed, 254 insertions(+), 32 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5590c1f16df..ae04019a1c2 100644 --- 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 index e2c3b916cf6..9cf1e1d6ecf 100644 --- 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 index ba437c68978..99babeefe84 100644 --- a/src/util/actor.ts +++ b/src/util/actor.ts @@ -45,6 +45,8 @@ export interface IActor { export type MessageHandler = (mapId: string | number, params: RequestResponseMessageMap[T][0], abortController?: AbortController) => Promise; +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: '', + 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: '', - 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 = []; const messageToPost: MessageData = { ...message, diff --git a/src/util/browser.test.ts b/src/util/browser.test.ts index dcef4c6472b..bd739ab7c87 100644 --- 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 index 393d36364c5..927b94736b4 100755 --- 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 {