Skip to content
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

sync with live: true triggers "change" events twice #4293

Closed
gr2m opened this issue Sep 9, 2015 · 8 comments
Closed

sync with live: true triggers "change" events twice #4293

gr2m opened this issue Sep 9, 2015 · 8 comments
Labels
bug Confirmed bug

Comments

@gr2m
Copy link
Contributor

gr2m commented Sep 9, 2015

var PouchDB = require('pouchdb').defaults({
  // memdown makes no difference, just added for sake of simplicity
  db: require('memdown')
})

var db1 = new PouchDB('db1')
var db2 = new PouchDB('db2')

db1.put({_id: 'a'})

.then(function () {
  return db2.put({_id: 'b'})
})

.then(function () {
  db1.sync(db2, {live: true}).on('change', function (change) {
    console.log(change.direction, change.change.docs)
  })
})

Output looks like this

$ node debug.js 
push [ { _id: 'a', _rev: '1-4c355ea809e3720366f3ce63e030039f' } ]
pull [ { _id: 'b', _rev: '1-30da76a9658982ad291add2de458cb7c' } ]
push [ { _id: 'a', _rev: '1-4c355ea809e3720366f3ce63e030039f' } ]
pull [ { _id: 'b', _rev: '1-30da76a9658982ad291add2de458cb7c' } ]

WIthout the live: true option, it correctly triggers the "change" events only once per document. Is this by design? Or some kind of race condition maybe?

@daleharvey daleharvey added the bug Confirmed bug label Sep 9, 2015
@daleharvey
Copy link
Member

Certainly not by design, just a straight up bug I think

@varjmes
Copy link

varjmes commented Sep 9, 2015

I poked around a bit: I'm unfamiliar with how everything fits together but I can say with confidence that your journey to fix this may start with this line: https://github.com/pouchdb/pouchdb/blob/master/lib/sync.js#L80

  if (opts.live) {
    this.push.on('complete', self.pull.cancel.bind(self.pull));
    this.pull.on('complete', self.push.cancel.bind(self.push));
  }

Not a great lot of help, but a start :)

@nolanlawson
Copy link
Member

Mmm yeah if this only occurs with sync() but not with replicate(), then it's definitely a bug in sync.js.

@daleharvey
Copy link
Member

Added a test here, its a bug in replicate - #4348, when we have a replication, when a document is written to a database via pull replication, we trigger a change on the push replication that is empty

daleharvey added a commit that referenced this issue Sep 17, 2015
@daleharvey
Copy link
Member

Fairly trivial fix - 764ba8a, however looks like the retry tests are broken somehow (they depend on extra events being fired) and now failing

@daleharvey
Copy link
Member

So the retry test depends on this behaviour, the retry test does rep.on('change', postNewDoc) to post docs in a loop, currently we only fire the change event once the checkpoint has successfully written, if that fails, the batch fails, replication gets retried but the next document is already written so it will fire the empty change event

@daleharvey
Copy link
Member

We can change the behaviour for replication to fire change events before the checkpointer, however that is going to have a knock on effect (the tests fail when I do that)

@daleharvey
Copy link
Member

Fixed in fe4cc24

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bug
Projects
None yet
Development

No branches or pull requests

4 participants