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 with v6 #419

Closed
denchiklut opened this issue Nov 23, 2023 · 10 comments
Closed

Memory leak with v6 #419

denchiklut opened this issue Nov 23, 2023 · 10 comments

Comments

@denchiklut
Copy link

denchiklut commented Nov 23, 2023

After updating to v6, I started having a memory leak issue.
After comparing Heap snapshots, I noticed lots of links in retained objects of changeListeners in Cookie

Screenshot 2023-11-23 at 11 56 55 PM

Downgrading react-cookie & universal-cookie-express to 5.0.0 solved the memory leak issue.

 npx envinfo --system --binaries --npmPackages universal-cookie-express,react-cookie,universal-cookie                                ✔ 
  System:
    OS: macOS 13.6
    CPU: (8) arm64 Apple M1 Pro
    Memory: 445.11 MB / 32.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 18.14.0 - ~/.nvm/versions/node/v18.14.0/bin/node
    Yarn: 1.22.19 - ~/.nvm/versions/node/v18.14.0/bin/yarn
    npm: 9.3.1 - ~/.nvm/versions/node/v18.14.0/bin/npm
    pnpm: 8.3.1 - ~/.nvm/versions/node/v18.14.0/bin/pnpm
  npmPackages:
    react-cookie: ^6.1.1 => 6.1.1 
    universal-cookie-express: ^6.1.1 => 6.1.1 

@meros
Copy link

meros commented Dec 15, 2023

I've seen this as well.

@meros
Copy link

meros commented Dec 15, 2023

After some investigation I think this is the culprit: 7816526

The interval keeps an instance to this, that causes the whole cookie class to be retained.

@Reeywhaar
Copy link

Can confirm. After adding removeChangeListener we've fixed our app's memory leak

@meros
Copy link

meros commented Dec 20, 2023

My problem is that I am using express middleware, that adds a change listener. It's not possible for me to remove this as I don't have access to the listener function itself.

@Reeywhaar
Copy link

Then you should address it to the middleware author I guess? Is it available on github?

@meros
Copy link

meros commented Dec 20, 2023

It's in this repo as well:

req.universalCookies.addChangeListener((change: CookieChangeOptions) => {

@dawecode
Copy link

Memory leak is happening on our app as well, downgrading has solved it for now.

@cbazureau
Copy link

Same here, already revert universal-cookie-express update on the end of september due to memory leak just after an upgrade to 6.1.1.

eXon added a commit that referenced this issue Dec 30, 2023
* Clean up test setup

* Migrate deprecated jest function

* Add tests

* Improve test
@eXon
Copy link
Collaborator

eXon commented Dec 30, 2023

Will be fixed in next release

@eXon eXon closed this as completed Dec 30, 2023
@eXon
Copy link
Collaborator

eXon commented Dec 30, 2023

Released in v6.1.2

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 a pull request may close this issue.

6 participants