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

(#398) - reuse PouchDB object between calls #426

Merged
merged 1 commit into from Feb 11, 2017

Conversation

Projects
None yet
1 participant
@nolanlawson
Copy link
Member

nolanlawson commented Feb 8, 2017

This partially (?) solves the memory leak (#398). Incidentally it also seems to make the server much faster.

@nolanlawson

This comment has been minimized.

Copy link
Member

nolanlawson commented Feb 8, 2017

Boo, tests fail. This needs more research first.

@nolanlawson nolanlawson force-pushed the 398-fix branch from 378ef57 to 7f45308 Feb 9, 2017

@nolanlawson

This comment has been minimized.

Copy link
Member

nolanlawson commented Feb 9, 2017

OK, this should pass now. Also, based on my tests using --inspect and the memory profiler, the memory leak is now fixed. The tradeoff is that I've created a single global lock to make all db creates/destroys serial.

@nolanlawson

This comment has been minimized.

Copy link
Member

nolanlawson commented Feb 9, 2017

Unfortunately there is still a leak if you delete and recreate databases between every call, but hopefully nobody is doing that except for unit tests...

@nolanlawson

This comment has been minimized.

Copy link
Member

nolanlawson commented Feb 10, 2017

This PR stops the bleeding, but I'd like to also identify the root memory leak.

@nolanlawson

This comment has been minimized.

Copy link
Member

nolanlawson commented Feb 11, 2017

Upon further inspection I believe this is the full, correct memory fix. In the current code we create a new PouchDB() object every time any endpoint is called, and we never explicitly destroy() that database, so the _destructionListeners keep piling up, because each one has to notify the db object if and when it gets destroyed. It just never gets destroyed.

This is consistent with our design for EventEmitters in PouchDB and I don't believe it constitutes a leak in PouchDB itself because we are upholding the contract, which is that db.on('destroyed') will get called for every db object. The problem is we are simply creating too many db objects in express-pouchdb.

I'm going to self-+1 this because it's been a few days and this is an important fix.

@nolanlawson nolanlawson merged commit 17cb28a into master Feb 11, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment