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

changes() leaks memory #6502

Closed
cztomsik opened this Issue May 15, 2017 · 12 comments

Comments

Projects
None yet
7 participants
@cztomsik

cztomsik commented May 15, 2017

Any changes() call without return_docs: false leaks memory

It seems that return_docs is effective even for http connections unlike from what is mentioned in the docs:

Is available for non-http databases and defaults to true

Reproduce

const db = new PouchDB('http://127.0.0.1:5984/skimdb')

db.changes({
  include_docs: true,
  batch_size: 1000
}).on('change', (c) => {
  // doesnt matter
})

What happens is that it will "slow down" after first ~50 000 seqs - because of excessive memory usage (few seconds and it's hundreds MBs). I am not sure if it relates to any existing issue so I am creating new one.

This might be resolved by:

  1. updating docs (along with examples)
  2. making return_docs: false default for http databases
@vvo

This comment has been minimized.

Contributor

vvo commented May 15, 2017

Just stumbled on this too, we have been facing a memory leak on https://github.com/algolia/npm-search and have been searching for it a bit.

I will use this return_docs: false and see what happens. The docs says [return_docs] Is available for non-http databases and defaults to true.

We are using the http adapter, but strangely the code of the http adapter sets return_docs to true by default, see

Not sure this as anything to do with our leak but will try.

vvo added a commit to algolia/npm-search that referenced this issue May 15, 2017

fix(memleak): maybe fix it
see pouchdb/pouchdb#6502

Ultimately we should be able to fix this memleak by doing a serious
analysis of heap etc but last time I tried I failed at understanding
what was going on.

So for now let's try to fix "obvious" mistakes.
@Haroenv

This comment has been minimized.

Haroenv commented May 16, 2017

This did fix our memory leak, so maybe return_docs should always be false by default.

@vvo

This comment has been minimized.

Contributor

vvo commented May 16, 2017

maybe return_docs should always be false by default.

Yes in the http couch adapter, will wait for a more knowledgeable person (on pouch) to confirm this.

@vvo

This comment has been minimized.

Contributor

vvo commented May 22, 2017

@willholley If you need more insights let us know, for now this is mostly a documentation issue. The doc says that if you are only using a http layer or DB then you are fine, changes will not leak. But that's not the case because the actual http pouchdb layer sets returnDocs: true by default.

@willholley

This comment has been minimized.

Member

willholley commented May 22, 2017

thanks @vvo - will do

@daleharvey

This comment has been minimized.

Member

daleharvey commented Jun 24, 2017

So yup the documentation is wrong here, however we should not be setting different defaults for http or not, we should fix the documentation for whether this option is available for http adapters

Seperately we could change the default for all adapters to returnDocs = false, or more drastically remove the option for returnDocs at all and require any changes listeners to collect their own results

@cztomsik

This comment has been minimized.

cztomsik commented Jun 24, 2017

just fixing docs is fine :)

@vvo

This comment has been minimized.

Contributor

vvo commented Jun 24, 2017

The documentation is wrong on two aspects: we say that the http adapter is not using returnDocs and that's it's false by default.

You can passe returnDocs when using an http adapter and it's true by default (which causes memleaks).

Seperately we could change the default for all adapters to returnDocs = false

I am down for this, if you can avoid memleaks situations automatically and warn people it's a better default because debugging mem leaks is awful.

@jareware

This comment has been minimized.

Contributor

jareware commented Aug 3, 2017

FWIW, we just spent a long time wondering where our memory is being retained in a long-running process perusing changes() and think this is the cause. +1 for:

or more drastically remove the option for returnDocs at all and require any changes listeners to collect their own results

Anecdotally, I didn't even know you can use the complete event for collecting your results, and have always collected them myself in change. :)

@stale

This comment has been minimized.

stale bot commented Nov 29, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@alxndrsn

This comment has been minimized.

Member

alxndrsn commented Feb 26, 2018

Documentation still needs updating.

@alxndrsn alxndrsn reopened this Feb 26, 2018

@stale stale bot removed the wontfix label Feb 26, 2018

daleharvey added a commit that referenced this issue Mar 21, 2018

daleharvey added a commit that referenced this issue Apr 1, 2018

daleharvey added a commit that referenced this issue Apr 2, 2018

daleharvey added a commit that referenced this issue Apr 2, 2018

daleharvey added a commit that referenced this issue Apr 2, 2018

daleharvey added a commit that referenced this issue Apr 3, 2018

daleharvey added a commit that referenced this issue Apr 4, 2018

daleharvey added a commit that referenced this issue Apr 4, 2018

@daleharvey

This comment has been minimized.

Member

daleharvey commented Apr 4, 2018

ok got a fix for this up, return_docs defaults to true unless opts.live = true, then it default is false, updated the document to match reality as well - #7211

daleharvey added a commit that referenced this issue Apr 4, 2018

@daleharvey daleharvey closed this Apr 4, 2018

aeremin added a commit to sth-larp/deus-server that referenced this issue Apr 17, 2018

aeremin added a commit to alice-larp/alice-larp-sdk that referenced this issue Aug 22, 2018

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