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 CLS hooked memory leak #5801

Merged
merged 2 commits into from
May 11, 2022
Merged

Fix CLS hooked memory leak #5801

merged 2 commits into from
May 11, 2022

Conversation

mike12345567
Copy link
Collaborator

@mike12345567 mike12345567 commented May 10, 2022

Description

After fixing the memory leak we found in our PouchDB usage we were still experiencing a slow leak, I found issues:

  1. [Memory leak]The destroyNamespace method doesn't seem to work Jeff-Lewis/cls-hooked#67
  2. [Memory leak] Context is not cleared if it references a Promise Jeff-Lewis/cls-hooked#66

These two issues demonstrate a memory leak that is related to the async hooks not being cleared up when the context is finished. We also had an issue with using a single static and global namespace which could not be destroyed. First I changed how our context functions work, so that there are two contexts, a single global one which stores the ID of a namespace which is created per request, then the request namespace which is created and destroyed as part of the run. I tested this by taking periodic snapshots of the heap state and comparing them while the app service was under heavy load, before these changes I would see a few structures grow in count in a linear fashion compared to the number of requests, e.g. PouchDB objects left over after being closed.

Once these changes were implemented all of these structures were no longer present, the memory usage would remain constant across all of the snapshots.

…emory leak (no async hooks disable call) fixed as well as changing how we use the CLS namespaces to allow us to destroy the namespace we use per request.
@mike12345567 mike12345567 self-assigned this May 10, 2022
Copy link
Contributor

@Rory-Powell Rory-Powell left a comment

Choose a reason for hiding this comment

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

Great work diagnosing and fixing this issue 💯 / 💯

@mike12345567 mike12345567 merged commit a1d6228 into master May 11, 2022
@mike12345567 mike12345567 deleted the fix/cls-hooked-memory-leak branch May 11, 2022 09:34
@github-actions github-actions bot locked and limited conversation to collaborators May 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants