#1734 Fix race conditions in the pouchDBCheckpointsPlugin

[chapko]

Fixes #1734

---

This change is 

[ThaliTester]

PR is added to the queue for testing as 1. task. (8b10a27)

[ThaliTester]

Test 102446911 (8b10a27) build started.

[ThaliTester]

Test (Fail) 102446911 build is completed (8b10a27)

See https://github.com/ThaliTester/TestResults/tree/1024469118b10a27__1734_Fix_race_conditions_in_the_pouchDBCheckpointsPlugin_chapko/ for the logs

[ThaliTester]

PR is added to the queue for testing as 1. task. (cf31575)

[ThaliTester]

Test 102446911 (cf31575) build started.

[ThaliTester]

PR is added to the queue for testing as 2. task. (a6c1be7)

[ThaliTester]

Test 102446911 (a6c1be7) build started.

[ThaliTester]

Test (Fail) 102446911 build is completed (cf31575)

See https://github.com/ThaliTester/TestResults/tree/102446911cf31575__1734_Fix_race_conditions_in_the_pouchDBCheckpointsPlugin_chapko/ for the logs

[ThaliTester]

Test (Success) 102446911 build is completed (a6c1be7)

See https://github.com/ThaliTester/TestResults/tree/102446911a6c1be7__1734_Fix_race_conditions_in_the_pouchDBCheckpointsPlugin_chapko/ for the logs

[ThaliTester]

Test 102446911a6c1be7(a6c1be7) has failed

See https://github.com/ThaliTester/TestResults/tree/102446911a6c1be7__1734_Fix_race_conditions_in_the_pouchDBCheckpointsPlugin_chapko/ for the fail logs

[yaronyg]

Please see comments.

---

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

thali/NextGeneration/utils/pouchDBCheckpointsPlugin.js, line 70 at r3 (raw file):

    });

    var changes = db.changes({

How does this ever get cancelled? If we have stopped Thali isn't this going to just run forever? Seems like a memory leak.

---

thali/NextGeneration/utils/pouchDBCheckpointsPlugin.js, line 74 at r3 (raw file):

      since: 'now'
    })
      .on('error', function (error) {

Shouldn't we also grab the complete event? It is fired if a live change is cancelled which per my previous comment we should probably be doing. And when it happens shouldn't we call cancel on checkDBSizeThrottled?

---

thali/NextGeneration/utils/pouchDBCheckpointsPlugin.js, line 79 at r3 (raw file):

          String(error), error.stack
        );
        changes.cancel();

Shouldn't we call cancel on checkDBSizeThrottled here as well?

---

Comments from Reviewable

[czyzm]

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

Comments from Reviewable

[evabishchevich]

Failure:
https://gist.github.com/evabishchevich/0417304547546ba6c410a30c1f3aa7db

[chapko]

I think the best course for this PR is to update plugin API a bit to make it possible to unsubscribe from 'checkpoint' events. See comments for details.

---

Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

thali/NextGeneration/utils/pouchDBCheckpointsPlugin.js, line 70 at r3 (raw file):

Previously, yaronyg (Yaron Y Goland) wrote…

How does this ever get cancelled? If we have stopped Thali isn't this going to just run forever? Seems like a memory leak.

There is no public API to cancel it manually. All we have is onCheckpointReached method to subscribe to the checkpoint event.

Original spec (#319) suggests .on('...', handler) syntax. This way it would be possible to unsubscribe and if there are no 'checkpoint' event listeners we could just cancel changes feed. But it was implemented differently.

My suggestion is to change the API and use db.on() and db.removeListener() to subscribe and unsubscribe respectively.

---

thali/NextGeneration/utils/pouchDBCheckpointsPlugin.js, line 74 at r3 (raw file):

Previously, yaronyg (Yaron Y Goland) wrote…

Shouldn't we also grab the complete event? It is fired if a live change is cancelled which per my previous comment we should probably be doing. And when it happens shouldn't we call cancel on checkDBSizeThrottled?

Yes, it makes sense. The only thing that bothers me is that 'complete' event might be emitted asynchronously after calling cancel. I think we should call checkDBSizeThrottled.cancel() together with changes.cancel(). Or we can also listen to 'cancel' event, but it is not documented, so I wouldn't recommend it if there are other solutions.

I suggest to manually cancel timer synchronously with changes.cancel() and cancel it in the complete event handler.

---

thali/NextGeneration/utils/pouchDBCheckpointsPlugin.js, line 79 at r3 (raw file):

Previously, yaronyg (Yaron Y Goland) wrote…

Shouldn't we call cancel on checkDBSizeThrottled here as well?

Yes, everything from previous comment applies here.

---

Comments from Reviewable

[yaronyg]

Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

thali/NextGeneration/utils/pouchDBCheckpointsPlugin.js, line 70 at r3 (raw file):

Previously, chapko (Eugene Chapko) wrote…

There is no public API to cancel it manually. All we have is onCheckpointReached method to subscribe to the checkpoint event.

Original spec (#319) suggests .on('...', handler) syntax. This way it would be possible to unsubscribe and if there are no 'checkpoint' event listeners we could just cancel changes feed. But it was implemented differently.

My suggestion is to change the API and use db.on() and db.removeListener() to subscribe and unsubscribe respectively.

It's just weird that this wasn't ever caught by the PouchDB folks. But I guess as long as you don't check changes very often you would probably never even notice the leak. How big a deal do we think this will be in practice for us?

---

thali/NextGeneration/utils/pouchDBCheckpointsPlugin.js, line 74 at r3 (raw file):

Previously, chapko (Eugene Chapko) wrote…

Yes, it makes sense. The only thing that bothers me is that 'complete' event might be emitted asynchronously after calling cancel. I think we should call checkDBSizeThrottled.cancel() together with changes.cancel(). Or we can also listen to 'cancel' event, but it is not documented, so I wouldn't recommend it if there are other solutions.

I suggest to manually cancel timer synchronously with changes.cancel() and cancel it in the complete event handler.

Nuke it from orbit, it's the only way to be sure. Agreed, cancel everything.

---

thali/NextGeneration/utils/pouchDBCheckpointsPlugin.js, line 79 at r3 (raw file):

Previously, chapko (Eugene Chapko) wrote…

Yes, everything from previous comment applies here.

Ditto

---

Comments from Reviewable

[chapko]

pouchDBCheckpointsPlugin is not used anywhere in Thali, thats why this PR was put on hold for such a long time and we just disabled failing tests. See comments below.

---

Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

thali/NextGeneration/utils/pouchDBCheckpointsPlugin.js, line 70 at r3 (raw file):
I'm not sure what PouchDB folks has to do with it. It is our own plugin.

As far as I know this plugin should fix #499. The only problem is that it is not used anywhere in the Thali. I think it makes more sense to make it a separate module.

How big a deal do we think this will be in practice for us?

Right now the only impact it has is 3 KB wasted to store its source code :) If someone is going to use it with default config, then it most likely will leak one change listener per database.

---

Comments from Reviewable

[yaronyg]

Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

thali/NextGeneration/utils/pouchDBCheckpointsPlugin.js, line 70 at r3 (raw file):

Previously, chapko (Eugene Chapko) wrote…

I'm not sure what PouchDB folks has to do with it. It is our own plugin.

As far as I know this plugin should fix #499. The only problem is that it is not used anywhere in the Thali. I think it makes more sense to make it a separate module.

How big a deal do we think this will be in practice for us?

Right now the only impact it has is 3 KB wasted to store its source code :) If someone is going to use it with default config, then it most likely will leak one change listener per database.

Sorry, as usual, I was confused. :(

---

Comments from Reviewable