Skip to content
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

fix(auth): Memory Leak in AsyncHttpCall affecting auth.listUsers #2236

Merged
merged 2 commits into from
Jul 10, 2023

Conversation

adrianjost
Copy link
Contributor

@adrianjost adrianjost commented Jul 7, 2023

fixes #2235

The socket code was only introduced to fix a bug in NodeJS 12 (db8be26) and since then has even needed a patch (1a34bc4). However the code does not seem necessary at all anymore. Removing this code resolved the described memory leak.

Hey there! So you want to contribute to a Firebase SDK?
Before you file this pull request, please read these guidelines:

Discussion

  • Read the contribution guidelines (CONTRIBUTING.md).
  • If this has been discussed in an issue, make sure to link to the issue here.
    If not, go file an issue about this before creating a pull request to discuss.

Testing

  • Make sure all existing tests in the repository pass after your change. - CI not started yet, but we are running it in production without issues 🤷‍♂️
  • If you fixed a bug or added a feature, add a new test to cover your code. Not sure how this could be tested

API Changes

  • At this time we cannot accept changes that affect the public API. If you'd like to help
    us make Firebase APIs better, please propose your change in an issue so that we
    can discuss it together.

fixes 2235
The socket code was only introduced to fix a bug in NodeJS 12 (firebase@db8be26) and since then has even needed a patch (firebase@1a34bc4). However the code does not seem necessary at all anymore.
Removing this code resolved the described memory leak.
@adrianjost adrianjost marked this pull request as ready for review July 7, 2023 09:23
@adrianjost adrianjost changed the title Fix Memory Leak in AsyncHttpCall affecting auth.listUsers fix(auth) Memory Leak in AsyncHttpCall affecting auth.listUsers Jul 10, 2023
@adrianjost adrianjost changed the title fix(auth) Memory Leak in AsyncHttpCall affecting auth.listUsers fix(auth): Memory Leak in AsyncHttpCall affecting auth.listUsers Jul 10, 2023
@lahirumaramba lahirumaramba self-requested a review July 10, 2023 17:44
@lahirumaramba lahirumaramba self-assigned this Jul 10, 2023
@lahirumaramba lahirumaramba added release:stage Stage a release candidate release-note labels Jul 10, 2023
Copy link
Member

@lahirumaramba lahirumaramba left a comment

Choose a reason for hiding this comment

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

Thank you for the fix! LGTM!

@lahirumaramba lahirumaramba merged commit c21de1e into firebase:master Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:stage Stage a release candidate release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Memory Leak in getAuth(...).listUsers(...)
2 participants