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

Node domains causing a memory leak #5255

Closed
garethbowen opened this issue Jan 18, 2019 · 9 comments
Closed

Node domains causing a memory leak #5255

garethbowen opened this issue Jan 18, 2019 · 9 comments
Assignees
Labels
Priority: 1 - High Blocks the next release. Type: Performance Make something faster

Comments

@garethbowen
Copy link
Member

garethbowen commented Jan 18, 2019

Domains in Node are deprecated because they're bad. In particular, we call Domain.dispose which has been removed in Node v10 so uncaught exceptions cause even more exceptions to be thrown.

Domains can be used to limit errors to one specific request, but currently we're exiting the process which clearly isn't limiting it at all. Replace the use of domains with an unhandled exception handler.

@garethbowen garethbowen added Type: Technical issue Improve something that users won't notice Priority: 1 - High Blocks the next release. labels Jan 18, 2019
@garethbowen garethbowen self-assigned this Jan 18, 2019
@garethbowen garethbowen removed their assignment Jan 23, 2019
@ngaruko ngaruko self-assigned this Jan 23, 2019
@ngaruko
Copy link
Contributor

ngaruko commented Jan 23, 2019

Any clue how to AT this @garethbowen ?

@garethbowen
Copy link
Member Author

No AT needed. Sorry, I should have just closed it!

@garethbowen
Copy link
Member Author

garethbowen commented Jan 28, 2019

@dianabarsan found this memory leak in node v10+ which is solved by this fix.

@garethbowen
Copy link
Member Author

Due to the above memory leak Diana and I decide to backport this for 3.3.0 release.

@garethbowen garethbowen changed the title Don't use Node domains Node domains causing a memory leak Jan 28, 2019
@garethbowen garethbowen added Type: Performance Make something faster and removed Type: Technical issue Improve something that users won't notice labels Jan 28, 2019
@dianabarsan
Copy link
Member

dianabarsan commented Jan 29, 2019

Unfortunately, domain also leaks on Node 8.11.4 (which is the version that we use in production), albeit the leak appears to be not as severe as for Node 10.

Offline user calls to _changes, _bulk_docs, _all_docs & _bulk_get (~5k requests):
image

admin calls to _session (~50k requests):
image

Above tests were done on 3.2.1 api code running on Node 8.11.4 in production mode.

@dianabarsan
Copy link
Member

Same as above but after removing domain from api router:

Offline user calls to _changes, _bulk_docs, _all_docs & _bulk_get (~5k requests) 35MB vs 612MB:
image

admin calls to _session (~50k requests) 28MB vs 899MB:
image

@garethbowen
Copy link
Member Author

Thanks for the additional info @dianabarsan .

As discussed I'm proposing to leave this in 3.3.0 for now and monitor running instances and restart as needed. If it becomes too problematic we can backport to a new 3.2.2 release.

@derickl
Copy link
Member

derickl commented Feb 11, 2019

Probably worth back porting given MUSO's now frequent incidences of 502s.

@garethbowen
Copy link
Member Author

@derickl Have those crashes been investigated? How confident are we that it's this issue causing them?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: 1 - High Blocks the next release. Type: Performance Make something faster
Projects
None yet
Development

No branches or pull requests

4 participants