Skip to content

feat(common): use addEventListener instead of onabort in async utils onAbortPromise #645

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

douglascayers
Copy link

@douglascayers douglascayers commented Jun 27, 2025

Background

Identified a potential bug with overwriting an AbortSignal.onabort property.

Problem

The onabort property is being reassigned by the onAbortPromise function, which if a value had already been set then that logic may now not run because it's been overwritten. This can lead to unexpected behavior.

Changes

  • Use the addEventListener method which allows multiple listeners to be registered.
  • Replace onAbortPromise with resolveEarlyOnAbort to centralize memory management of the abort event listener with the promise race semantics of simply resolving early on abort

Copy link

changeset-bot bot commented Jun 27, 2025

🦋 Changeset detected

Latest commit: 74a9a95

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
@powersync/common Patch
@powersync/node Patch
@powersync/op-sqlite Patch
@powersync/react-native Patch
@powersync/tanstack-react-query Patch
@powersync/web Patch
@powersync/diagnostics-app Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

@simolus3 simolus3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks - I agree with this fix.

(sidenode: I think even an extremely short manual description is better than the one generated by copilot - the main thing I needed to know was that external users might want to set onabort on signals passed to onAbortPromise too. "addressing potential bugs and race conditions" isn't really what's going on here)

@douglascayers
Copy link
Author

I think even an extremely short manual description is better than the one generated by copilot

Thank you for the feedback. I'll rewrite the descriptions and skip letting AI write them.

@douglascayers douglascayers requested a review from simolus3 June 29, 2025 02:51
@douglascayers
Copy link
Author

douglascayers commented Jun 29, 2025

@simolus3 I've refactored the approach to also mitigate memory leaks, would appreciate your follow up review at your convenience

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants