Merge pull request share#259 from share/otapply-rollback

ericyhwang · web-flow · commit 70b881509da1 · 2018-11-28T11:16:39.000-06:00
Catch synchronous errors in `Doc._otApply`
diff --git a/lib/client/doc.js b/lib/client/doc.js
@@ -327,8 +327,11 @@ Doc.prototype._handleOp = function(err, message) {
   }
 
   this.version++;
-  this._otApply(message, false);
-  return;
+  try {
+    this._otApply(message, false);
+  } catch (error) {
+    return this._hardRollback(error);
+  }
 };
 
 // Called whenever (you guessed it!) the connection state changes. This will
@@ -512,8 +515,8 @@ function transformX(client, server) {
 Doc.prototype._otApply = function(op, source) {
   if (op.op) {
     if (!this.type) {
-      var err = new ShareDBError(4015, 'Cannot apply op to uncreated document. ' + this.collection + '.' + this.id);
-      return this.emit('error', err);
+      // Throw here, because all usage of _otApply should be wrapped with a try/catch
+      throw new ShareDBError(4015, 'Cannot apply op to uncreated document. ' + this.collection + '.' + this.id);
     }
 
     // Iteratively apply multi-component remote operations and rollback ops
@@ -655,8 +658,12 @@ Doc.prototype._submit = function(op, source, callback) {
     if (this.type.normalize) op.op = this.type.normalize(op.op);
   }
 
-  this._pushOp(op, callback);
-  this._otApply(op, source);
+  try {
+    this._pushOp(op, callback);
+    this._otApply(op, source);
+  } catch (error) {
+    return this._hardRollback(error);
+  }
 
   // The call to flush is delayed so if submit() is called multiple times
   // synchronously, all the ops are combined before being sent to the server.
@@ -867,7 +874,11 @@ Doc.prototype._rollback = function(err) {
     // I'm still not 100% sure about this functionality, because its really a
     // local op. Basically, the problem is that if the client's op is rejected
     // by the server, the editor window should update to reflect the undo.
-    this._otApply(op, false);
+    try {
+      this._otApply(op, false);
+    } catch (error) {
+      return this._hardRollback(error);
+    }
 
     this._clearInflightOp(err);
     return;
@@ -877,22 +888,34 @@ Doc.prototype._rollback = function(err) {
 };
 
 Doc.prototype._hardRollback = function(err) {
+  // Store pending ops so that we can notify their callbacks of the error.
+  // We combine the inflight op and the pending ops, because it's possible
+  // to hit a condition where we have no inflight op, but we do have pending
+  // ops. This can happen when an invalid op is submitted, which causes us
+  // to hard rollback before the pending op was flushed.
+  var pendingOps = [];
+  if (this.inflightOp) pendingOps.push(this.inflightOp);
+  pendingOps = pendingOps.concat(this.pendingOps);
+
   // Cancel all pending ops and reset if we can't invert
-  var op = this.inflightOp;
-  var pending = this.pendingOps;
   this._setType(null);
   this.version = null;
   this.inflightOp = null;
   this.pendingOps = [];
 
-  // Fetch the latest from the server to get us back into a working state
+  // Fetch the latest version from the server to get us back into a working state
   var doc = this;
   this.fetch(function() {
-    var called = op && callEach(op.callbacks, err);
-    for (var i = 0; i < pending.length; i++) {
-      callEach(pending[i].callbacks, err);
+    // We want to check that no errors are swallowed, so we check that:
+    // - there are callbacks to call, and
+    // - that every single pending op called a callback
+    // If there are no ops queued, or one of them didn't handle the error,
+    // then we emit the error.
+    var allOpsHadCallbacks = !!pendingOps.length;
+    for (var i = 0; i < pendingOps.length; i++) {
+      allOpsHadCallbacks = callEach(pendingOps[i].callbacks, err) && allOpsHadCallbacks;
     }
-    if (err && !called) return doc.emit('error', err);
+    if (err && !allOpsHadCallbacks) return doc.emit('error', err);
   });
 };
 
diff --git a/test/client/doc.js b/test/client/doc.js
@@ -1,5 +1,6 @@
 var Backend = require('../../lib/backend');
 var expect = require('expect.js');
+var util = require('../util')
 
 describe('client query subscribe', function() {
 
@@ -212,4 +213,133 @@ describe('client query subscribe', function() {
 
   });
 
+  describe('submitting an invalid op', function () {
+    var doc;
+    var invalidOp;
+    var validOp;
+
+    beforeEach(function (done) {
+      // This op is invalid because we try to perform a list deletion
+      // on something that isn't a list
+      invalidOp = {p: ['name'], ld: 'Scooby'};
+
+      validOp = {p:['snacks'], oi: true};
+
+      doc = this.connection.get('dogs', 'scooby');
+      doc.create({ name: 'Scooby' }, function (error) {
+        if (error) return done(error);
+        doc.whenNothingPending(done);
+      });
+    });
+
+    it('returns an error to the submitOp callback', function (done) {
+      doc.submitOp(invalidOp, function (error) {
+        expect(error.message).to.equal('Referenced element not a list');
+        done();
+      });
+    });
+
+    it('rolls the doc back to a usable state', function (done) {
+      util.callInSeries([
+        function (next) {
+          doc.submitOp(invalidOp, function (error) {
+            expect(error).to.be.ok();
+            next();
+          });
+        },
+        function (next) {
+          doc.whenNothingPending(next);
+        },
+        function (next) {
+          expect(doc.data).to.eql({name: 'Scooby'});
+          doc.submitOp(validOp, next);
+        },
+        function (next) {
+          expect(doc.data).to.eql({name: 'Scooby', snacks: true});
+          next();
+        },
+        done
+      ]);
+    });
+
+    it('rescues an irreversible op collision', function (done) {
+      // This test case attempts to reconstruct the following corner case, with
+      // two independent references to the same document. We submit two simultaneous, but
+      // incompatible operations (eg one of them changes the data structure the other op is
+      // attempting to manipulate).
+      //
+      // The second document to attempt to submit should have its op rejected, and its
+      // state successfully rolled back to a usable state.
+      var doc1 = this.backend.connect().get('dogs', 'snoopy');
+      var doc2 = this.backend.connect().get('dogs', 'snoopy');
+
+      var pauseSubmit = false;
+      var fireSubmit;
+      this.backend.use('submit', function (request, callback) {
+        if (pauseSubmit) {
+          fireSubmit = function () {
+            pauseSubmit = false;
+            callback();
+          };
+        } else {
+          fireSubmit = null;
+          callback();
+        }
+      });
+
+      util.callInSeries([
+        function (next) {
+          doc1.create({colours: ['white']}, next);
+        },
+        function (next) {
+          doc1.whenNothingPending(next);
+        },
+        function (next) {
+          doc2.fetch(next);
+        },
+        function (next) {
+          doc2.whenNothingPending(next);
+        },
+        // Both documents start off at the same v1 state, with colours as a list
+        function (next) {
+          expect(doc1.data).to.eql({colours: ['white']});
+          expect(doc2.data).to.eql({colours: ['white']});
+          next();
+        },
+        // doc1 successfully submits an op which changes our list into a string in v2
+        function (next) {
+          doc1.submitOp({p: ['colours'], oi: 'white,black'}, next);
+        },
+        // This next step is a little fiddly. We abuse the middleware to pause the op submission and
+        // ensure that we get this repeatable sequence of events:
+        // 1. doc2 is still on v1, where 'colours' is a list (but it's a string in v2)
+        // 2. doc2 submits an op that assumes 'colours' is still a list
+        // 3. doc2 fetches v2 before the op submission completes - 'colours' is no longer a list locally
+        // 4. doc2's op is rejected by the server, because 'colours' is not a list on the server
+        // 5. doc2 attempts to roll back the inflight op by turning a list insertion into a list deletion
+        // 6. doc2 applies this list deletion to a field that is no longer a list
+        // 7. type.apply throws, because this is an invalid op
+        function (next) {
+          pauseSubmit = true;
+          doc2.submitOp({p: ['colours', '0'], li: 'black'}, function (error) {
+            expect(error.message).to.equal('Referenced element not a list');
+            next();
+          });
+
+          doc2.fetch(function (error) {
+            if (error) return next(error);
+            fireSubmit();
+          });
+        },
+        // Validate that - despite the error in doc2.submitOp - doc2 has been returned to a
+        // workable state in v2
+        function (next) {
+          expect(doc1.data).to.eql({colours: 'white,black'});
+          expect(doc2.data).to.eql(doc1.data);
+          doc2.submitOp({p: ['colours'], oi: 'white,black,red'}, next);
+        },
+        done
+      ]);
+    });
+  });
 });
diff --git a/test/client/submit.js b/test/client/submit.js
@@ -629,16 +629,11 @@ describe('client submit', function() {
         doc2.del(function(err) {
           if (err) return done(err);
           doc.pause();
-          var calledBack = false;
-          doc.on('error', function() {
-            expect(calledBack).equal(true);
-            done();
-          });
           doc.submitOp({p: ['age'], na: 1}, function(err) {
-            expect(err).ok();
+            expect(err.code).to.equal(4017);
             expect(doc.version).equal(2);
             expect(doc.data).eql(undefined);
-            calledBack = true;
+            done();
           });
           doc.fetch();
         });
@@ -658,16 +653,11 @@ describe('client submit', function() {
           doc2.create({age: 5}, function(err) {
             if (err) return done(err);
             doc.pause();
-            var calledBack = false;
-            doc.on('error', function() {
-              expect(calledBack).equal(true);
-              done();
-            });
             doc.create({age: 9}, function(err) {
-              expect(err).ok();
+              expect(err.code).to.equal(4018);
               expect(doc.version).equal(3);
               expect(doc.data).eql({age: 5});
-              calledBack = true;
+              done();
             });
             doc.fetch();
           });
