pouchdb-server
pouchdb-server copied to clipboard
pouchdb
Long polling fails to clean up and goes on forever
From @yaronyg on June 3, 2016 15:29
Please see https://github.com/pouchdb/pouchdb/issues/5271 for the test code I'm using and check out my final comment there. What I can see happening is that once we set up a long polling connection it keeps polling, forever. Even though the socket associated with the req is destroyed!
Copied from original issue: pouchdb/express-pouchdb#326
From @yaronyg on June 3, 2016 15:30
Note that this matters because:
- It's a memory leak on every single replication and in something like Thali we will do a lot of them
- I keeps node from exiting which make running our test framework impossible and is just bad form :)
From @colinskow on June 3, 2016 16:9
It doesn't appear that CouchDB (and thus PouchDB-Express) actually has an API to instantly cancel continuous replication short of letting the long-poll feed time out.
You could try sending your server a SIGINT event in order to make the tests pass. The connections will eventually automatically drop depending on the timeout and KEEPALIVE settings.
From @yaronyg on June 3, 2016 16:19
In the case I'm looking at the scenario is that both the client AND the server have closed their sockets. So the socket is fully destroyed. However the changes code doesn't realize this and continues writing to its now dead res object endlessly.
From @colinskow on June 3, 2016 17:6
Adding a req.on('close') handler to PouchDB-Express could be a possible solution.
From @yaronyg on June 3, 2016 17:46
So per an old stack overflow article I found it claimed that one should listen to the close event on req.connection but I tested that out and it never triggered, even when the socket was destroyed. So just for fun I tried, well, everything:
req.on = listened for abort and socket req.connection.on = listened for close, end res.on = listened for close and finish res.connection.on = listened for close and finish
Nothing fired. Literally nothing.
So I have a trick that seems to work (although really slowly) that I'll submit as a PR for lack of a better idea.
I record the changes object for both of the continuous changes (e.g. for continuous or the inner changes in long poll).
Then I extended cleanup to call cancel on the changes object as well as to call res.end().
Then I put a check into heartbeatInterval to see if res.connection.destroyed is true and if so I call cleanup.
Not pretty but it seems to work.
Yeah this doesn't surprise me; I think I wrote that code and didn't test it heavily
From @yaronyg on June 3, 2016 18:21
@nolanlawson you and @marten-de-vries both get 'blame' :) It's a minor bug. I'll go fix it now. Mostly I have to figure out what kind of testing I'm supposed to do. Do I just run the normal PouchDB main repo tests but against my forked version of Express-PouchDB?
From @yaronyg on June 3, 2016 18:46
Ahh yes, 'npm run test' Well duh. :)
From @yaronyg on June 3, 2016 19:19
Would it be fair to say that nobody has run the tests in this project in a really long time? Because I'm pretty sure it's loading pouchdb incorrectly.
This appears to have a test case in https://github.com/pouchdb/pouchdb/issues/5271 but needs serious investigation, marking as "hard"
@nolanlawson did you find a solution or workaround for this issue? I'm seeing a leak that amounts to 2GB node res mem in 48 hours (shows up in the heap as an accumulation of zlib and related objects), I'm surprised more people aren't bothered by this issue, given the 5k per week downloads of this package
