change event with 2 way replication issue + bugfix #4627

Closed
mickael-kerjean opened this Issue Dec 2, 2015 · 3 comments

Comments

Projects
None yet
3 participants
@mickael-kerjean

Hello there,

First of all, thank you for having created such a cool and usefull project!

As I was having some fun with some code, I went into some issues with the live replication option.
The change event is quite powerfull to plug some application code that can refresh part of screen
depending on the type of document that is received.

With two way sync enable, Everytime some data is push, I receive a push then a pull.
The issue being the docs received while pulling has already been received before.

As a piece of code worth more than just plain text, here is an example. The more we insert data, the worst it becomes: see this other example when trying to insert way more documents.

As it was the perfect opportunity to browse some of pouchdb code, I
drilled down and found where my problem was coming: /lib/replicate/replicate.js:

  1. Here, we can see how the batch is processed. The interesting functions are writeDocs and finishBatch
  2. the finishBatch function trigger the actuel change event given a change object that has the docs field I was interested in, this field being populated with the changedDocs variable.
  3. writeDocs is currently resetting the changedDocs array but not in
    every case
    which is causing the issue I was experiencing

In other words, for my use case I had to update the original writeDocs
function to become: https://gist.github.com/mickael-kerjean/a6fdda69372ace248b55

That fairly basic but it is now working as expected by my application.

Long life to pouchdb!

@mickael-kerjean mickael-kerjean changed the title from change event with 2 way replication, just sharing a change I had to insert in pouch to make my application work to change event with 2 way replication Dec 2, 2015

@mickael-kerjean mickael-kerjean changed the title from change event with 2 way replication to change event with 2 way replication issue + bugfix that solve my problem Dec 2, 2015

@mickael-kerjean mickael-kerjean changed the title from change event with 2 way replication issue + bugfix that solve my problem to change event with 2 way replication issue + bugfix Dec 2, 2015

@nolanlawson

This comment has been minimized.

Show comment
Hide comment
@nolanlawson

nolanlawson Dec 13, 2015

Member

I'm sorry, but there are a few things about this issue I don't understand:

  1. What exactly is the problem? Performance? You don't specify.
  2. Can you please provide your fix as a pull request? It's much easier if we can see the diff between the old code and the new code, and plus Travis will automatically run our tests, so we can verify that your change doesn't break anything.

That said, if there are inefficiencies in the live replication code, I'm not surprised at all, and would gladly welcome a fix. :)

Member

nolanlawson commented Dec 13, 2015

I'm sorry, but there are a few things about this issue I don't understand:

  1. What exactly is the problem? Performance? You don't specify.
  2. Can you please provide your fix as a pull request? It's much easier if we can see the diff between the old code and the new code, and plus Travis will automatically run our tests, so we can verify that your change doesn't break anything.

That said, if there are inefficiencies in the live replication code, I'm not surprised at all, and would gladly welcome a fix. :)

@mickael-kerjean

This comment has been minimized.

Show comment
Hide comment
@mickael-kerjean

mickael-kerjean Dec 14, 2015

  1. Yes it is a performance issue. This example simulates 2 databases that synchronize together, both are initialize with a set of documents. If we push a batch of documents in one database, the change event is trigger a crazy amount of times (23 times in the example) and the data received from the change event is repeated over and over again. Performance was greatly improve by this small change in the code.
  2. sure, I'll grab a linux machine this week and will write a proper test.
  1. Yes it is a performance issue. This example simulates 2 databases that synchronize together, both are initialize with a set of documents. If we push a batch of documents in one database, the change event is trigger a crazy amount of times (23 times in the example) and the data received from the change event is repeated over and over again. Performance was greatly improve by this small change in the code.
  2. sure, I'll grab a linux machine this week and will write a proper test.

willholley added a commit that referenced this issue Jan 6, 2016

(#4627) - reset changedDocs in write_docs
This fix from @mickael-kerjean resets the changedDocs array
on every call to writeDocs to prevent duplicate "change" events
firing during 2 way replication.

willholley added a commit that referenced this issue Jan 6, 2016

(#4627) - reset changedDocs in write_docs
This fix from @mickael-kerjean resets the changedDocs array
on every call to writeDocs to prevent duplicate "change" events
firing during 2 way replication.

willholley added a commit that referenced this issue Jan 6, 2016

(#4627) - reset changedDocs in write_docs
This fix from @mickael-kerjean resets the changedDocs array
on every call to writeDocs to prevent duplicate "change" events
firing during 2 way replication.

willholley added a commit that referenced this issue Jan 7, 2016

(#4627) - reset changedDocs in write_docs
This fix from @mickael-kerjean resets the changedDocs array
on every call to writeDocs to prevent duplicate "change" events
firing during 2 way replication.
@daleharvey

This comment has been minimized.

Show comment
Hide comment
@daleharvey

daleharvey Jan 7, 2016

Member

Fixed in c905239, cheers @mickael-kerjean for the detailed report and @willholley for picking it up and fixing it

Member

daleharvey commented Jan 7, 2016

Fixed in c905239, cheers @mickael-kerjean for the detailed report and @willholley for picking it up and fixing it

@daleharvey daleharvey closed this Jan 7, 2016

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