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

sendDeltasToPeer sends old vector clock #9

Open
choxi opened this issue Jun 28, 2017 · 1 comment
Open

sendDeltasToPeer sends old vector clock #9

choxi opened this issue Jun 28, 2017 · 1 comment

Comments

@choxi
Copy link
Collaborator

choxi commented Jun 28, 2017

I noticed a potential bug in sendDeltasToPeer:

diff --git a/src/ampl/network/delta-router.js b/src/ampl/network/delta-router.js
index cc6fb18..c4f9e37 100644
--- a/src/ampl/network/delta-router.js
+++ b/src/ampl/network/delta-router.js
@@ -86,7 +86,7 @@ export default class DeltaRouter {

         if (this.isAheadOf(myClock, theirEstimatedClock)) {
           console.log("We are ahead - send deltas",peer.id)
-          this.sendDeltasToPeer(peer)
+          this.sendDeltasToPeer(peer, theirEstimatedClock)
         }

         // it should be safe to use the estimated clock but for this purpose m.vectorClock would work too
@@ -114,15 +114,14 @@ export default class DeltaRouter {
   broadcastState() {
     console.log("broadcast state")
     this.peergroup.peers().forEach((peer) => {
-      this.sendDeltasToPeer(peer)
+      this.sendDeltasToPeer(peer, this.clocks[peer.id])
     })
   }

-  sendDeltasToPeer(peer) {
+  sendDeltasToPeer(peer, theirClock) {
     console.log("maybe send deltas")
     let state = this.getTesseractCB()
     let myClock = Tesseract.getVClock(state)
-    let theirClock = this.clocks[peer.id];

     if (theirClock) {

When it's called from the peer message handler, we calculate which deltas to send based on our peer's clock in this.clocks[peer.id], but we don't update this.clocks[peer.id] to the newest clock until after we send the deltas:

        // update the clock after sending to prevent exceptions above from falsely moving our
        // estimated peer clock forward
        this.clocks[peer.id] = theirEstimatedClock

That means we're probably sending deltas based on a stale clock value. Maybe we're just sending redundant deltas then, but it could be causing other problems as well.

@pvh
Copy link
Member

pvh commented Jun 29, 2017

Ah, yea, that's a good call. We had a different bug which was if something went wrong with delta sending it would still update the clock and then refuse to send those deltas again even if the other client requested them. This is wrong in that it will send too many deltas but at least it will send them...

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

No branches or pull requests

2 participants