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 Auth instantiation - frameworkChangeObservers never cleared #11770

Closed
3 tasks done
mathbruyen opened this issue Aug 10, 2023 · 4 comments
Closed
3 tasks done
Assignees
Labels
Auth Related to Auth components/category bug Something isn't working

Comments

@mathbruyen
Copy link

mathbruyen commented Aug 10, 2023

Before opening, please confirm:

JavaScript Framework

Not applicable

Amplify APIs

Authentication

Amplify Categories

auth

Environment information

  System:
    OS: Linux 5.10 Debian GNU/Linux 11 (bullseye) 11 (bullseye)
    CPU: (8) x64 Intel(R) Core(TM) i5-8250U CPU @ 1.60GHz
    Memory: 3.04 GB / 15.36 GB
    Container: Yes
    Shell: 5.1.4 - /bin/bash
  Binaries:
    Node: 20.5.0 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 9.8.0 - /usr/local/bin/npm
  Browsers:
    Chrome: 115.0.5790.170
  npmPackages:
    @types/express: ^4.17.14 => 4.17.17 
    @types/mocha: ^10.0.0 => 10.0.1 
    @types/node: ^18.11.17 => 18.17.3 
    @typescript-eslint/eslint-plugin: ^6.0.0 => 6.3.0 
    @typescript-eslint/parser: ^6.0.0 => 6.3.0 
    aws-amplify: ^5.1.4 => 5.3.6 
    compression: ^1.7.4 => 1.7.4 
    cookie-parser: ^1.4.6 => 1.4.6 
    css-loader: ^6.7.3 => 6.8.1 
    csurf: ^1.11.0 => 1.11.0 
    eslint: ^8.33.0 => 8.46.0 
    eslint-config-google: ^0.14.0 => 0.14.0 
    eslint-plugin-react: ^7.32.2 => 7.33.1 
    express: ^4.18.2 => 4.18.2 
    express-graphql: ^0.12.0 => 0.12.0 
    glob: ^10.0.0 => 10.3.3 (7.2.3, 7.2.0)
    graphql: ^16.6.0 => 16.7.1 (15.8.0)
    intl-messageformat: ^10.3.0 => 10.5.0 
    js-cookie: ^3.0.1 => 3.0.5 (2.2.1)
    jszip: ^3.10.1 => 3.10.1 
    mini-css-extract-plugin: ^2.7.2 => 2.7.6 
    mocha: ^10.2.0 => 10.2.0 
    nodemon: ^3.0.0 => 3.0.1 
    puppeteer: ^21.0.0 => 21.0.1 
    react: ^18.2.0 => 18.2.0 
    react-dom: ^18.2.0 => 18.2.0 
    react-redux: ^8.0.5 => 8.1.2 
    react-router-dom: ^6.8.1 => 6.14.2 
    redux: ^4.2.1 => 4.2.1 
    sass: ^1.58.0 => 1.64.2 
    sass-loader: ^13.2.0 => 13.3.2 
    ts-loader: ^9.4.1 => 9.4.4 
    typescript: ^5.0.0 => 5.1.6 
    webpack: ^5.75.0 => 5.88.2 
    webpack-assets-manifest: ^5.1.0 => 5.1.0 
    webpack-cli: ^5.0.1 => 5.1.4 
    webpack-node-externals: ^3.0.0 => 3.0.0 
    zip-stream: ^4.1.0 => 4.1.0 
  npmGlobalPackages:
    corepack: 0.19.0
    npm: 9.8.0

Describe the bug

During its creation, Auth class is adding a callback for being notified about framework changes (observeFrameworkChanges). This callback is never cleared (there is no API to remove callbacks from the array) and causes a memory leak when using Auth with server side rendering as one instance is created for every request:

at observeFrameworkChanges (/portal/node_modules/@aws-amplify/core/lib/Platform/detectFramework.js:32:13)
at PlatformBuilder.observeFrameworkChanges (/portal/node_modules/@aws-amplify/core/lib/Platform/index.js:46:55)
at new AuthClass (/portal/node_modules/@aws-amplify/auth/lib/Auth.js:80:25)
at /portal/node_modules/aws-amplify/lib/withSSRContext.js:39:26
at Array.forEach (<anonymous>)
at withSSRContext (/portal/node_modules/aws-amplify/lib/withSSRContext.js:38:13)
at defineAmplify (webpack://webServer/./src/server/services/amplify.ts?:163:84)
at eval (webpack://webServer/./src/server/services/async.ts?:8:19)
at Layer.handle [as handle_request] (/portal/node_modules/express/lib/router/layer.js:95:5)
at trim_prefix (/portal/node_modules/express/lib/router/index.js:328:13)

This seem to have been introduced with 03aa356

Expected behavior

Server side rendering does not cause memory usage to continuously grow

Reproduction steps

We have a basic Express app that does the following on every request:

const Amplify = withSSRContext({req});

and then uses Auth to authenticate users against Cognito.

Code Snippet

No response

Log output

No response

aws-exports.js

No response

Manual configuration

No response

Additional configuration

No response

Mobile Device

No response

Mobile Operating System

No response

Mobile Browser

No response

Mobile Browser Version

No response

Additional information and screenshots

No response

@mathbruyen mathbruyen added the pending-triage Issue is pending triage label Aug 10, 2023
@cwomack cwomack added the Auth Related to Auth components/category label Aug 10, 2023
@cwomack
Copy link
Contributor

cwomack commented Aug 10, 2023

Hello, @mathbruyen 👋. We appreciate you not only opening this issue, but providing the suspected commit as well! We'll review this internally and report back on this issue soon.

@cwomack cwomack added investigating This issue is being investigated and removed pending-triage Issue is pending triage labels Aug 10, 2023
@cwomack cwomack self-assigned this Aug 10, 2023
@cwomack cwomack removed the investigating This issue is being investigated label Aug 11, 2023
@cwomack
Copy link
Contributor

cwomack commented Aug 21, 2023

@mathbruyen, this should now be fixed as of the most recent version of Amplify. Can you upgrade and verify that the memory leak no longer occurs?

The fix was actually merged in as of the v5.3.7 release, but the newer v5.3.8 has additional fixes to the Auth category that we'd recommend you have!

@cwomack cwomack added the pending-response Issue is pending response from the issue requestor label Aug 21, 2023
@mathbruyen
Copy link
Author

The change reduced memory increase speed, I found another leak which I am reporting under a different ticket but this one seems indeed fixed. Thank you!

@cwomack
Copy link
Contributor

cwomack commented Sep 5, 2023

@mathbruyen, that's great to hear that the one reported in this issue is resolved!

As for the new one, definitely open a new ticket and reference this issue in it please. We'll investigate the new one as soon as you do. Thank you for taking the time to report these!

@cwomack cwomack added bug Something isn't working and removed pending-response Issue is pending response from the issue requestor labels Sep 5, 2023
@cwomack cwomack closed this as completed Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Auth Related to Auth components/category bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants