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.

Sign up for GitHub

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

Jump to bottom

Clean up store error event listener to prevent memory leak #82

Merged
merged 2 commits into from Jun 7, 2019
Merged

Clean up store error event listener to prevent memory leak #82

merged 2 commits into from Jun 7, 2019

Conversation

clement-buchart
Copy link

@clement-buchart clement-buchart commented May 17, 2019 •
edited

Fixes #30

@clement-buchart clement-buchart mentioned this pull request May 17, 2019
Closed
@coveralls
Copy link

coveralls commented May 17, 2019 •
edited

Coverage Status

Coverage remained the same at 100.0% when pulling aaa7dab on clement-buchart:fix/eventlistener-leak into 237b219 on lukechilds:master.

@lukechilds
Copy link
Collaborator

Thanks so much for submitting this @clement-buchart.

This is technically a breaking change. The error event can be emitted multiple times, and could even be emitted when a request isn't currently taking place.

It is for both request errors and underlying DB errors. See: https://github.com/lukechilds/cacheable-request#onerror-error

So removing the event after each request means DB errors will not get passed through.

However the memory leak is obviously a big issue. I am definitely ok with making breaking changes to get it resolved.

lukechilds
lukechilds requested changes May 18, 2019
View reviewed changes
src/index.js Outdated
@@ -186,7 +186,10 @@ class CacheableRequest {
}
};

this.cache.on('error', error => ee.emit('error', new CacheableRequest.CacheError(error)));
const storeErrorHandler = error => ee.emit('error', new CacheableRequest.CacheError(error));
this.cache.on('error', storeErrorHandler);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be changed to this.cache.once. Then you don't need to manually remove it after it's fired. (Still needs to be manually removed after response though)

src/index.js Outdated
@@ -186,7 +186,10 @@ class CacheableRequest {
}
};

this.cache.on('error', error => ee.emit('error', new CacheableRequest.CacheError(error)));
const storeErrorHandler = error => ee.emit('error', new CacheableRequest.CacheError(error));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we just call this errorHandler as opposed to storeErrorHandler?

Copy link
Author

Choose a reason for hiding this comment

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

Done

@clement-buchart clement-buchart force-pushed the fix/eventlistener-leak branch 3 times, most recently from d41ff8a to 7df14e0 Compare May 21, 2019 03:01
@clement-buchart
Copy link
Author

This is technically a breaking change. The error event can be emitted multiple times, and could even be emitted when a request isn't currently taking place.

It is for both request errors and underlying DB errors. See: https://github.com/lukechilds/cacheable-request#onerror-error

So removing the event after each request means DB errors will not get passed through.

However the memory leak is obviously a big issue. I am definitely ok with making breaking changes to get it resolved.

I'm not sure I fully understand.
Isn't the ee EventEmitter variable scoped to a single request ?
Your concern is that if an 'error' event is sent on ee because of a request (non-DB) error, I will remove the DB error event listener ?
I feel like that's what I want though. My reasoning would be that regardless of the outcome of a request wrapped by cacheableRequest, I want to clean up the DB Error event listener attached to it, to prevent leak.

Sorry if I'm missing something obvious.

@lukechilds
Copy link
Collaborator

lukechilds commented Jun 3, 2019 •
edited

@clement-buchart Sorry, you are correct. I was thinking ee was scoped to the cacheable request instance not the request function scope.

Apologies for the delay on this, it's taken me a while to get round to review it due to a few crazy things in my personal life.

You mentioned in the related issue this cleared up your memory leak issues. Do you have any stats you can share for that?

@clement-buchart
Copy link
Author

Hello,

Here is what I can easily share (the project being a commercial one) :
I added https://www.npmjs.com/package/node-memwatch and https://www.npmjs.com/package/heapdump and made it so the app shutdowns on a leak event from node-memwatch

I simply generated load via ApacheBenchmark.

Sample A (before fix) : The app quickly grows to use more than 1Gi RAM. Then a memory leak is detected by node-memwatch, and the app shuts down.

When taking a Heapdump, a single Keyv Instance keep a trace of all request in memory :
image
You can see the retainer is the error events listener of Keyv :
image

Sample B (after fix) :
We simply added this to package.json (where 6.0.1 is my Pull Request pushed to our corporate repo)

"resolutions": {
    "got/cacheable-request": "6.0.1"
},

The app's memory footprint quickly stabilize (under same load), and Keyv holds almost no memory
image

Here are two dump after similar load generated, before and after fix :

image

Cheers.

@lukechilds
Copy link
Collaborator

@clement-buchart wow, thank you so much, this is great.

@lukechilds lukechilds changed the title fix: Clean up store error event listener to prevent memory leak Clean up store error event listener to prevent memory leak Jun 7, 2019
@lukechilds lukechilds merged commit cd683a2 into jaredwray:master Jun 7, 2019
@lukechilds
Copy link
Collaborator

Published as cacheable-request@6.1.0.

@lukechilds
Copy link
Collaborator

@clement-buchart You can submit your PR here to claim a bounty: https://issuehunt.io/r/lukechilds/cacheable-request/issues/30

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukechilds lukechilds lukechilds approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Possible event listener memory leak
3 participants
@clement-buchart @coveralls @lukechilds