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

Memory Leak in getAuth(...).listUsers(...) #2235

Closed
adrianjost opened this issue Jul 6, 2023 · 3 comments · Fixed by #2236
Closed

Memory Leak in getAuth(...).listUsers(...) #2235

adrianjost opened this issue Jul 6, 2023 · 3 comments · Fixed by #2236

Comments

@adrianjost
Copy link
Contributor

adrianjost commented Jul 6, 2023

Environment

  • Operating System version: macOS 13.4.1 (but also in within a Google Cloud Run NodeJS environment)
  • Firebase SDK version:
    • firebase: 9.23.0 (also 9.21.0)
    • firebase-admin: 11.9.0 (also 11.7.0)
    • firebase-functions: 4.4.1 (also 4.3.1)
  • Firebase Product: auth
  • Node.js version: 16, 18, 20
  • NPM version: 9.5.1

Problem

Steps to reproduce:

We discovered a Memory Leak when running the list_all_users example code from https://firebase.google.com/docs/auth/admin/manage-users#list_all_users.
The data loaded from getAuth(app).listUsers(1000, nextPageToken) is not garbage collected and causes the memory footprint of our authentication backup to grow and eventually run out of memory.

node20-plain-new-dep

Every blue bar is memory that is still used with is ~1MB/1000 Users making it almost impossible to use this function to backup multiple GB of user data.
The problematic path seems to be within: node_modules/firebase-admin/lib/utils/api-request.js.

Side-note: We only started noticing this issue with node v20, with node v18 and below we did not have the issue when using our production bundle that is generated with typescript and webpack. We could go back to previous versions and can see that the memory is correctly cleaned up in older node versions. Unfortunately we couldn't figure out what exactly the difference to the simple issue demo script I've added below is, which has the issue even on older versions.
It just shows us, that there is now an issue that didn't exist before.

We could also see the same when not using a loop and just calling auth.listUsers(1000) once, so the issue is not in the boilerplate around. Also the issue goes away when we mock this function call.

Here you can see a Memory Profile of the issue with our webpack output. You can clearly see, that in node20 the blue bars that are kept are way larger. In node18 and before the data was immediately cleaned up.

node 20 node 18 (16 looks similar)
node20-webpack-new-dep node18-webpack-new-dep

Relevant Code:

const { initializeApp } = require("firebase-admin/app");
const { getAuth } = require("firebase-admin/auth");

const app = initializeApp({
  projectId: /* TODO */,
});

const auth = getAuth(app);

const listAllUsers = (nextPageToken) => {
  auth
    .listUsers(1000, nextPageToken)
    .then((listUsersResult) => {
      listUsersResult.users.forEach((userRecord) => {
        console.log("user", userRecord.toJSON());
      });
      if (listUsersResult.pageToken) {
        listAllUsers(listUsersResult.pageToken);
      }
    })
    .catch((error) => {
      console.log("Error listing users:", error);
    });
};

listAllUsers();
@adrianjost
Copy link
Contributor Author

We have narrowed down the issue to https://github.com/firebase/firebase-admin-node/blob/master/src/utils/api-request.ts#L507-514
which was introduced to prevent a bug in Node12 but seems to be not necessary anymore.
When we just remove the req.on('socket', (socket) => { code entirely the memory leak is gone.

We figured out, that in newer node versions, socket.setTimeout(timeout, timeoutCallback); restarts the timeout after it added a new event listener causing socket.on('end', () => { to never be triggered. This results in the timeoutCallback never be released from memory and that one holds a reference to the entire req object.

@adrianjost
Copy link
Contributor Author

BTW here is an even simpler example with the memory leak:

const { initializeApp } = require("firebase-admin/app");
const { getAuth } = require("firebase-admin/auth");

const app = initializeApp({
  projectId: "TODO",
});

const waitSeconds = (seconds) =>
  new Promise((resolve) => setTimeout(resolve, seconds * 1000));

async function main() {
  while (true) {
    await getAuth(app).listUsers(1000);
    waitSeconds(2);
  }
}
main();

@lahirumaramba
Copy link
Member

Hi @adrianjost Thank you for the highly detailed issue report!
We really appreciate the time you spent on investigating this issue!

The fix you proposed in #2236 looks good to me. We can merge the fix once we complete the review. Thanks!

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

Successfully merging a pull request may close this issue.

3 participants