Partially fix domain memory leak #499
Conversation
|
👍 |
After the response is finished, remove the request and response objects from the domain and prevent them from remaining in the captured scope of the domains error listener.
|
This is all set (I fixed the lint issues). |
|
Looks like linting issues in the build.. ETA (must have been looking at a cached version of the page) .. Thanks @aheckmann |
|
you are correct, it was failing before but i forced pushed the fix :) |
|
👍 Neat... let me know when this gets published out... interested in getting this pulled into a future release. |
|
After a little more poking around, I found that the domains themselves are still being leaked (they hang around in the domain module's internal |
|
@aheckmann Excited to learn more about how you fixed this. We've been seeing a bad memory leak lately that we attributed to the Also, can you verify that your commit has your paypal email address and name properly set? It's not showing your icon. |
|
Hmm definitely worth some due diligence. I know that domains are pending deprecation and the path forward isn't straight forward in core. There are zones, experimental async-hooks and other help with async error handling approaches. All have trade-offs and issues. It does look like this was added as a minor bump, but unsure who relies on it now for their error handling. And interested to know how shutdown was triggered before. Will need to take a peek at the code again. |
|
The code currently uses the deprecated domain module to attempt a graceful
shutdown of the server. We could remove domains and use an
uncaughtException handler to do the same in it's place.
…On Tue, Dec 5, 2017, 8:04 AM Shaun Warman ***@***.***> wrote:
Hmm definitely worth some due diligence. I know that domains are pending
deprecation and the path forward isn't straight forward in core
<nodejs/node#10843>.
There are zones <nodejs/CTC#4>, experimental
async-hooks <https://nodejs.org/api/async_hooks.html> and other help with
async error handling approaches. All have trade-offs and issues.
It does look like this was added as a minor bump, but unsure who relies on
it now for their error handling. And interested to know how shutdown was
triggered before. Will need to take a peek at the code again.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#499 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAKLslq3tlp5uDPKb_QkqgINiMXnIliUks5s9WlogaJpZM4QzAZa>
.
|
Long story short; it took about a week of reading / re-reading these fine articles:
Then a lot of experimentation + the heapdump module + finding a route which reproduced the memory leak (most of them did) + googling for domain related memory leaks. The comparison view in Chrome Dev Tools told me that ServerResponses were being created but not gc'd. Once I had that, I checked to see if IncomingMessages were being leaked. When they were and Dev Tools showed domains involved, I used |
|
Domains are nice here because we keep the crash associated with the CAL
record for the request. We already have a non-domain handler and it can't
tell us what routes triggered the failure.
…On Dec 5, 2017 10:06 AM, "Aaron Heckmann" ***@***.***> wrote:
The code currently uses the deprecated domain module to attempt a graceful
shutdown of the server. We could remove domains and use an
uncaughtException handler to do the same in it's place.
On Tue, Dec 5, 2017, 8:04 AM Shaun Warman ***@***.***>
wrote:
> Hmm definitely worth some due diligence. I know that domains are pending
> deprecation and the path forward isn't straight forward in core
> <nodejs/node#10843>.
>
> There are zones <nodejs/CTC#4>, experimental
> async-hooks <https://nodejs.org/api/async_hooks.html> and other help
with
> async error handling approaches. All have trade-offs and issues.
>
> It does look like this was added as a minor bump, but unsure who relies
on
> it now for their error handling. And interested to know how shutdown was
> triggered before. Will need to take a peek at the code again.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#499 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAKLslq3tlp5uDPKb_
QkqgINiMXnIliUks5s9WlogaJpZM4QzAZa>
> .
>
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#499 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAPBf7DGno-PjHo4NtJk98J4KffsDW6Nks5s9YYrgaJpZM4QzAZa>
.
|
|
To keep things focused, I'll open an issue to continue the discussion for considering removal of domains. This PR at least reduces the memory leak significantly and could be released as semver patch. |
|
A basic kraken app does not exhibit the leak. Therefore, I'm closing this PR because it isn't fixing the root cause (it just helps reduce the impact). The cause of the leaking domains is due to an issue in another module, which once I patch, I'll link back here for future reference. |
After the response is finished, remove the request and
response objects from the domain and prevent them from
remaining in the captured scope of the domains error
listener.