From c740eb196492df6444ad5a4590b5a60a15f115fd Mon Sep 17 00:00:00 2001 From: Doug Ayers Date: Fri, 27 Jun 2025 15:17:42 -0500 Subject: [PATCH 1/9] Refactor onAbortPromise to use addEventListener --- packages/common/src/utils/async.ts | 2 +- packages/common/tests/utils/async.test.ts | 72 +++++++++++++++++++++++ 2 files changed, 73 insertions(+), 1 deletion(-) create mode 100644 packages/common/tests/utils/async.test.ts diff --git a/packages/common/src/utils/async.ts b/packages/common/src/utils/async.ts index c6fe822d8..6be283f08 100644 --- a/packages/common/src/utils/async.ts +++ b/packages/common/src/utils/async.ts @@ -54,7 +54,7 @@ export function onAbortPromise(signal: AbortSignal): Promise { if (signal.aborted) { resolve(); } else { - signal.onabort = () => resolve(); + signal.addEventListener('abort', () => resolve(), { once: true }); } }); } diff --git a/packages/common/tests/utils/async.test.ts b/packages/common/tests/utils/async.test.ts new file mode 100644 index 000000000..05a84d821 --- /dev/null +++ b/packages/common/tests/utils/async.test.ts @@ -0,0 +1,72 @@ +import { describe, expect, it, vi } from 'vitest'; +import { onAbortPromise } from '../../src/utils/async'; + +describe('onAbortPromise', () => { + it('should resolve when signal is aborted', async () => { + const controller = new AbortController(); + const promise = onAbortPromise(controller.signal); + + // Abort the signal after a short delay + setTimeout(() => controller.abort(), 10); + + // The promise should resolve + await expect(promise).resolves.toBeUndefined(); + }); + + it('should resolve immediately if signal is already aborted', async () => { + const controller = new AbortController(); + controller.abort(); // Abort before creating promise + + const promise = onAbortPromise(controller.signal); + + // Should resolve immediately + await expect(promise).resolves.toBeUndefined(); + }); + + // This test emphasizes why not to use the 'onabort' property. + it('should demonstrate that overwriting onabort property supersedes previous onabort logic', async () => { + const controller = new AbortController(); + const signal = controller.signal; + + // Set up first onabort handler + const firstHandler = vi.fn(); + signal.onabort = firstHandler; + + // Overwrite onabort property with a second handler + const secondHandler = vi.fn(); + signal.onabort = secondHandler; + + // Abort the signal + controller.abort(); + + // Only the second handler should be called (first one was overwritten) + expect(firstHandler).not.toHaveBeenCalled(); + expect(secondHandler).toHaveBeenCalledTimes(1); + }); + + it('should show that onAbortPromise does not interfere with onabort property or other event listeners', async () => { + const controller = new AbortController(); + const signal = controller.signal; + + // Set up onabort property handler + const onabortHandler = vi.fn(); + signal.onabort = onabortHandler; + + // Set up another event listener + const eventListenerHandler = vi.fn(); + signal.addEventListener('abort', eventListenerHandler); + + // Create onAbortPromise (which uses addEventListener internally) + const promise = onAbortPromise(signal); + + // Abort the signal + controller.abort(); + + // All handlers should be called + expect(onabortHandler).toHaveBeenCalledTimes(1); + expect(eventListenerHandler).toHaveBeenCalledTimes(1); + + // The promise should also resolve + await expect(promise).resolves.toBeUndefined(); + }); +}); From 599a60390da95cca5c919bcfe780ac2c4b6af568 Mon Sep 17 00:00:00 2001 From: Doug Ayers Date: Fri, 27 Jun 2025 15:28:00 -0500 Subject: [PATCH 2/9] Add changeset entry --- .changeset/moody-hats-sleep.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/moody-hats-sleep.md diff --git a/.changeset/moody-hats-sleep.md b/.changeset/moody-hats-sleep.md new file mode 100644 index 000000000..92486e4ff --- /dev/null +++ b/.changeset/moody-hats-sleep.md @@ -0,0 +1,5 @@ +--- +'@powersync/common': patch +--- + +Use addEventListener instead of overwriting the onabort property, which can lead to bugs and race conditions. From a0b8b6165d4e176bf0469b8998c380aafc61f1ac Mon Sep 17 00:00:00 2001 From: Doug Ayers <4142577+douglascayers@users.noreply.github.com> Date: Sat, 28 Jun 2025 20:54:08 -0500 Subject: [PATCH 3/9] Update .changeset/moody-hats-sleep.md Co-authored-by: Simon Binder --- .changeset/moody-hats-sleep.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/moody-hats-sleep.md b/.changeset/moody-hats-sleep.md index 92486e4ff..c2ba32f2a 100644 --- a/.changeset/moody-hats-sleep.md +++ b/.changeset/moody-hats-sleep.md @@ -2,4 +2,4 @@ '@powersync/common': patch --- -Use addEventListener instead of overwriting the onabort property, which can lead to bugs and race conditions. +Use addEventListener instead of overwriting the onabort property, preventing interference with outside users also setting the property on the same signal. From 380847799c92c6f8d48b0d0a33c767e4cb0237cf Mon Sep 17 00:00:00 2001 From: Doug Ayers Date: Sat, 28 Jun 2025 21:06:18 -0500 Subject: [PATCH 4/9] Remove test that doesn't test the SDK. The logic is explained in the change set. --- packages/common/tests/utils/async.test.ts | 21 --------------------- 1 file changed, 21 deletions(-) diff --git a/packages/common/tests/utils/async.test.ts b/packages/common/tests/utils/async.test.ts index 05a84d821..1e50798eb 100644 --- a/packages/common/tests/utils/async.test.ts +++ b/packages/common/tests/utils/async.test.ts @@ -23,27 +23,6 @@ describe('onAbortPromise', () => { await expect(promise).resolves.toBeUndefined(); }); - // This test emphasizes why not to use the 'onabort' property. - it('should demonstrate that overwriting onabort property supersedes previous onabort logic', async () => { - const controller = new AbortController(); - const signal = controller.signal; - - // Set up first onabort handler - const firstHandler = vi.fn(); - signal.onabort = firstHandler; - - // Overwrite onabort property with a second handler - const secondHandler = vi.fn(); - signal.onabort = secondHandler; - - // Abort the signal - controller.abort(); - - // Only the second handler should be called (first one was overwritten) - expect(firstHandler).not.toHaveBeenCalled(); - expect(secondHandler).toHaveBeenCalledTimes(1); - }); - it('should show that onAbortPromise does not interfere with onabort property or other event listeners', async () => { const controller = new AbortController(); const signal = controller.signal; From 01e3403fff08ac6161340494dedb6f2a529ec497 Mon Sep 17 00:00:00 2001 From: Doug Ayers Date: Sat, 28 Jun 2025 21:49:26 -0500 Subject: [PATCH 5/9] Avoid memory leaks by removing event listener after race. --- .../AbstractStreamingSyncImplementation.ts | 8 +- packages/common/src/utils/async.ts | 36 +++++++-- packages/common/tests/utils/async.test.ts | 79 ++++++++++++------- 3 files changed, 86 insertions(+), 37 deletions(-) diff --git a/packages/common/src/client/sync/stream/AbstractStreamingSyncImplementation.ts b/packages/common/src/client/sync/stream/AbstractStreamingSyncImplementation.ts index f24e72e7a..2649df105 100644 --- a/packages/common/src/client/sync/stream/AbstractStreamingSyncImplementation.ts +++ b/packages/common/src/client/sync/stream/AbstractStreamingSyncImplementation.ts @@ -6,7 +6,7 @@ import { FULL_SYNC_PRIORITY, InternalProgressInformation } from '../../../db/cru import * as sync_status from '../../../db/crud/SyncStatus.js'; import { AbortOperation } from '../../../utils/AbortOperation.js'; import { BaseListener, BaseObserver, Disposable } from '../../../utils/BaseObserver.js'; -import { onAbortPromise, throttleLeadingTrailing } from '../../../utils/async.js'; +import { resolveEarlyOnAbort, throttleLeadingTrailing } from '../../../utils/async.js'; import { BucketChecksum, BucketDescription, @@ -1045,7 +1045,7 @@ The next upload iteration will be delayed.`); }); } - private async applyCheckpoint(checkpoint: Checkpoint, abort: AbortSignal) { + private async applyCheckpoint(checkpoint: Checkpoint, signal: AbortSignal) { let result = await this.options.adapter.syncLocalDatabase(checkpoint); const pending = this.pendingCrudUpload; @@ -1062,9 +1062,9 @@ The next upload iteration will be delayed.`); this.logger.debug( 'Could not apply checkpoint due to local data. Waiting for in-progress upload before retrying.' ); - await Promise.race([pending, onAbortPromise(abort)]); + await resolveEarlyOnAbort(pending, signal); - if (abort.aborted) { + if (signal.aborted) { return { applied: false, endIteration: true }; } diff --git a/packages/common/src/utils/async.ts b/packages/common/src/utils/async.ts index 6be283f08..8cc35de61 100644 --- a/packages/common/src/utils/async.ts +++ b/packages/common/src/utils/async.ts @@ -49,12 +49,38 @@ export function throttleLeadingTrailing(func: () => void, wait: number) { }; } -export function onAbortPromise(signal: AbortSignal): Promise { - return new Promise((resolve) => { +/** + * Race a promise against an abort signal. + * Returns a promise that resolves early if the signal is aborted before the + * original promise resolves. + * + * Note: The signal does not cancel the promise. To cancel the promise then + * its logic needs to explicitly check the signal. + */ +export function resolveEarlyOnAbort( + promise: Promise, + signal: AbortSignal +): Promise<{ result: T; aborted: false } | { aborted: true }> { + return new Promise((resolve, reject) => { if (signal.aborted) { - resolve(); - } else { - signal.addEventListener('abort', () => resolve(), { once: true }); + resolve({ aborted: true }); + return; } + + const abortHandler = () => { + resolve({ aborted: true }); + }; + + // Use an event listener to avoid interfering with the onabort + // property where other code may have registered a handler. + signal.addEventListener('abort', abortHandler); + + promise + .then((result) => resolve({ result, aborted: false })) + .catch((error) => reject(error)) + .finally(() => { + // Remove the abort handler to avoid memory leaks. + signal.removeEventListener('abort', abortHandler); + }); }); } diff --git a/packages/common/tests/utils/async.test.ts b/packages/common/tests/utils/async.test.ts index 1e50798eb..4df8b9868 100644 --- a/packages/common/tests/utils/async.test.ts +++ b/packages/common/tests/utils/async.test.ts @@ -1,51 +1,74 @@ import { describe, expect, it, vi } from 'vitest'; -import { onAbortPromise } from '../../src/utils/async'; +import { resolveEarlyOnAbort } from '../../src/utils/async'; -describe('onAbortPromise', () => { - it('should resolve when signal is aborted', async () => { +describe('resolveEarlyOnAbort', () => { + it('should resolve early when signal is aborted', async () => { const controller = new AbortController(); - const promise = onAbortPromise(controller.signal); - // Abort the signal after a short delay + const slowPromise = new Promise((resolve) => { + setTimeout(() => resolve('completed'), 100); + }); + + const racePromise = resolveEarlyOnAbort(slowPromise, controller.signal); + + // Abort after a short delay setTimeout(() => controller.abort(), 10); - // The promise should resolve - await expect(promise).resolves.toBeUndefined(); + const result = await racePromise; + expect(result).toEqual({ aborted: true }); }); it('should resolve immediately if signal is already aborted', async () => { const controller = new AbortController(); - controller.abort(); // Abort before creating promise + controller.abort(); // Abort before creating the race - const promise = onAbortPromise(controller.signal); + const slowPromise = new Promise((resolve) => { + setTimeout(() => resolve('completed'), 100); + }); - // Should resolve immediately - await expect(promise).resolves.toBeUndefined(); + const result = await resolveEarlyOnAbort(slowPromise, controller.signal); + expect(result).toEqual({ aborted: true }); }); - it('should show that onAbortPromise does not interfere with onabort property or other event listeners', async () => { + it('should resolve with the result if the promise resolves before the signal is aborted', async () => { const controller = new AbortController(); - const signal = controller.signal; - // Set up onabort property handler - const onabortHandler = vi.fn(); - signal.onabort = onabortHandler; + const slowPromise = new Promise((resolve) => { + setTimeout(() => resolve('completed'), 100); + }); - // Set up another event listener - const eventListenerHandler = vi.fn(); - signal.addEventListener('abort', eventListenerHandler); + const result = await resolveEarlyOnAbort(slowPromise, controller.signal); + expect(result).toEqual({ result: 'completed', aborted: false }); + }); + + it('should show that resolveEarlyOnAbort does not interfere with onabort property or other event listeners', async () => { + const controller = new AbortController(); + let onabortCalled = false; + let eventListenerCalled = false; - // Create onAbortPromise (which uses addEventListener internally) - const promise = onAbortPromise(signal); + // Set onabort property + controller.signal.onabort = () => { + onabortCalled = true; + }; - // Abort the signal - controller.abort(); + // Add another event listener + controller.signal.addEventListener('abort', () => { + eventListenerCalled = true; + }); + + const slowPromise = new Promise((resolve) => { + setTimeout(() => resolve('completed'), 100); + }); + + const racePromise = resolveEarlyOnAbort(slowPromise, controller.signal); + + // Abort after a short delay + setTimeout(() => controller.abort(), 10); - // All handlers should be called - expect(onabortHandler).toHaveBeenCalledTimes(1); - expect(eventListenerHandler).toHaveBeenCalledTimes(1); + const result = await racePromise; - // The promise should also resolve - await expect(promise).resolves.toBeUndefined(); + expect(result).toEqual({ aborted: true }); + expect(onabortCalled).toBe(true); + expect(eventListenerCalled).toBe(true); }); }); From fcd53c9480ca71f633017746bda968a4c29a608b Mon Sep 17 00:00:00 2001 From: Doug Ayers Date: Sat, 28 Jun 2025 21:50:24 -0500 Subject: [PATCH 6/9] Update changeset entry --- .changeset/moody-hats-sleep.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/moody-hats-sleep.md b/.changeset/moody-hats-sleep.md index c2ba32f2a..269fe0731 100644 --- a/.changeset/moody-hats-sleep.md +++ b/.changeset/moody-hats-sleep.md @@ -2,4 +2,4 @@ '@powersync/common': patch --- -Use addEventListener instead of overwriting the onabort property, preventing interference with outside users also setting the property on the same signal. +Use addEventListener instead of overwriting the onabort property, preventing interference with outside users also setting the property on the same signal. Remove event listener when race settles to avoid memory leak. From 7a6407d69ec5741903f245bbaeb17755fd9da347 Mon Sep 17 00:00:00 2001 From: Doug Ayers Date: Sun, 29 Jun 2025 10:48:21 -0500 Subject: [PATCH 7/9] Avoid memory leak by ensuring abort handler is removed --- packages/common/src/utils/async.ts | 47 ++++++++++++++++++++---------- 1 file changed, 31 insertions(+), 16 deletions(-) diff --git a/packages/common/src/utils/async.ts b/packages/common/src/utils/async.ts index 8cc35de61..bc8356d88 100644 --- a/packages/common/src/utils/async.ts +++ b/packages/common/src/utils/async.ts @@ -60,27 +60,42 @@ export function throttleLeadingTrailing(func: () => void, wait: number) { export function resolveEarlyOnAbort( promise: Promise, signal: AbortSignal -): Promise<{ result: T; aborted: false } | { aborted: true }> { +): Promise> { return new Promise((resolve, reject) => { - if (signal.aborted) { - resolve({ aborted: true }); - return; - } + const resolveWith = (result: ResolveEarlyOnAbortResult) => { + removeAbortHandler(); + resolve(result); + }; + + const rejectWith = (error: Error) => { + removeAbortHandler(); + reject(error); + }; const abortHandler = () => { - resolve({ aborted: true }); + resolveWith({ aborted: true }); }; - // Use an event listener to avoid interfering with the onabort - // property where other code may have registered a handler. - signal.addEventListener('abort', abortHandler); + const addAbortHandler = () => { + // Use an event listener to avoid interfering with the onabort + // property where other code may have registered a handler. + signal.addEventListener('abort', abortHandler); + }; - promise - .then((result) => resolve({ result, aborted: false })) - .catch((error) => reject(error)) - .finally(() => { - // Remove the abort handler to avoid memory leaks. - signal.removeEventListener('abort', abortHandler); - }); + const removeAbortHandler = () => { + // Remove the abort handler to avoid memory leaks. + signal.removeEventListener('abort', abortHandler); + }; + + addAbortHandler(); + + if (signal.aborted) { + resolveWith({ aborted: true }); + return; + } + + promise.then((result) => resolveWith({ result, aborted: false })).catch((error) => rejectWith(error)); }); } + +type ResolveEarlyOnAbortResult = { result: T; aborted: false } | { aborted: true }; From d08cc0863f441cd3bb1e5ded724582bdeb3fac78 Mon Sep 17 00:00:00 2001 From: Doug Ayers Date: Sun, 29 Jun 2025 10:52:39 -0500 Subject: [PATCH 8/9] Consolidate logic by calling abortHandler() if signal is already aborted --- packages/common/src/utils/async.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/common/src/utils/async.ts b/packages/common/src/utils/async.ts index bc8356d88..3f7fdb194 100644 --- a/packages/common/src/utils/async.ts +++ b/packages/common/src/utils/async.ts @@ -1,3 +1,5 @@ +import { abort } from 'process'; + /** * Throttle a function to be called at most once every "wait" milliseconds, * on the trailing edge. @@ -90,7 +92,7 @@ export function resolveEarlyOnAbort( addAbortHandler(); if (signal.aborted) { - resolveWith({ aborted: true }); + abortHandler(); return; } From f9561d20ceea70e070e472642044f75b747d9f6b Mon Sep 17 00:00:00 2001 From: Doug Ayers Date: Sun, 29 Jun 2025 10:55:18 -0500 Subject: [PATCH 9/9] Remove unused import of 'abort' --- packages/common/src/utils/async.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/common/src/utils/async.ts b/packages/common/src/utils/async.ts index 3f7fdb194..72e1ced20 100644 --- a/packages/common/src/utils/async.ts +++ b/packages/common/src/utils/async.ts @@ -1,5 +1,3 @@ -import { abort } from 'process'; - /** * Throttle a function to be called at most once every "wait" milliseconds, * on the trailing edge.