⚡️ Avoid committing no-ops to the database

This change checks for no-ops just before committing, and avoids writing
them to the database.

Motivation
----------
Sometimes we have situations where multiple clients try to make an
identical change to the same document at the same time (if multiple
clients are trying to keep two documents in sync, for example).

When this happens, this can result in a `O(n^2)` retry cascade, since
the requests will all keep retrying, with only a single request
succeeding at a time, even if those ops are all no-ops.

Solution
--------
This change adds a new special `ERR_NO_OP` error code. If the server
detects a no-op, it will skip committing, and early-return this error
code, which takes the request out of the retry queue.

The client then has some special handling for this case, where it will:

 1. Fetch from remote to get up-to-date with any potential conflicting
    ops
 2. Then acknowledge the in-flight op, **without** bumping the version
    (since the op wasn't committed)

Note that if `$fixup()` has been used, we **do not** early-return a
no-op error, even if the fixup results in a no-op, since the client
would be left in an inconsistent state without the fixup ops being
returned and applied.

Author: alecgibson

lib/backend.js

@@ -43,6 +43,7 @@ function Backend(options) {
       'new Backend({doNotForwardSendPresenceErrorsToClient: true})\n\n'
     );
   }
+  this.doNotCommitNoOps = !!options.doNotCommitNoOps;
 
   // Map from event name to a list of middleware
   this.middleware = Object.create(null);

lib/client/doc.js

@@ -328,6 +328,15 @@ Doc.prototype._handleSubscribe = function(error, snapshot) {
 
 Doc.prototype._handleOp = function(err, message) {
   if (err) {
+    if (err.code === ERROR_CODE.ERR_NO_OP && message.seq === this.inflightOp.seq) {
+      // Our op was a no-op, either because we submitted a no-op, or - more
+      // likely - because our op was transformed into a no-op by the server
+      // because of a similar remote op. In this case, the server has avoided
+      // committing the op to the database, and we should just clear the in-flight
+      // op and call the callbacks. However, let's first catch ourselves up to
+      // the remote, so that we're in a nice consistent state
+      return this.fetch(this._clearInflightOp.bind(this));
+    }
     if (this.inflightOp) {
       return this._rollback(err);
     }

lib/error.js

@@ -35,6 +35,7 @@ ShareDBError.CODES = {
   ERR_MAX_SUBMIT_RETRIES_EXCEEDED: 'ERR_MAX_SUBMIT_RETRIES_EXCEEDED',
   ERR_MESSAGE_BADLY_FORMED: 'ERR_MESSAGE_BADLY_FORMED',
   ERR_MILESTONE_ARGUMENT_INVALID: 'ERR_MILESTONE_ARGUMENT_INVALID',
+  ERR_NO_OP: 'ERR_NO_OP',
   ERR_OP_ALREADY_SUBMITTED: 'ERR_OP_ALREADY_SUBMITTED',
   ERR_OP_NOT_ALLOWED_IN_PROJECTION: 'ERR_OP_NOT_ALLOWED_IN_PROJECTION',
   ERR_OP_SUBMIT_REJECTED: 'ERR_OP_SUBMIT_REJECTED',

lib/protocol.js

@@ -1,6 +1,6 @@
 module.exports = {
   major: 1,
-  minor: 1,
+  minor: 2,
   checkAtLeast: checkAtLeast
 };
 

lib/submit-request.js

@@ -2,6 +2,7 @@ var ot = require('./ot');
 var projections = require('./projections');
 var ShareDBError = require('./error');
 var types = require('./types');
+const protocol = require('./protocol');
 
 var ERROR_CODE = ShareDBError.CODES;
 
@@ -204,6 +205,19 @@ SubmitRequest.prototype.commit = function(callback) {
       };
     }
 
+    var skipNoOp = backend.doNotCommitNoOps &&
+      protocol.checkAtLeast(request.agent.protocol, '1.2') &&
+      request.op.op &&
+      request.op.op.length === 0 &&
+      request._fixupOps.length === 0;
+
+    if (skipNoOp) {
+      // The op is a no-op, either because it was submitted as such, or - more
+      // likely - because it was transformed into one. Let's avoid committing it
+      // and tell the client.
+      return callback(request.noOpError());
+    }
+
     // Try committing the operation and snapshot to the database atomically
     backend.db.commit(
       request.collection,
@@ -361,3 +375,9 @@ SubmitRequest.prototype.maxRetriesError = function() {
     'Op submit failed. Exceeded max submit retries of ' + this.maxRetries
   );
 };
+SubmitRequest.prototype.noOpError = function() {
+  return new ShareDBError(
+    ERROR_CODE.ERR_NO_OP,
+    'Op is a no-op. Skipping commit.'
+  );
+};

test/client/submit.js

@@ -1290,6 +1290,92 @@ module.exports = function() {
       });
     });
 
+    it('commits both of two identical ops submitted from different clients by default', function(done) {
+      var backend = this.backend;
+      backend.doNotCommitNoOps = false;
+      var doc1 = backend.connect().get('dogs', id);
+      var doc2 = backend.connect().get('dogs', id);
+      var op = [{p: ['tricks', 0], ld: 'fetch'}];
+
+      async.series([
+        doc1.create.bind(doc1, {tricks: ['fetch', 'sit']}),
+        doc2.fetch.bind(doc2),
+        async.parallel.bind(async.parallel, [
+          doc1.submitOp.bind(doc1, op),
+          doc2.submitOp.bind(doc2, op)
+        ]),
+        function(next) {
+          expect(doc1.data).to.eql({tricks: ['sit']});
+          expect(doc2.data).to.eql({tricks: ['sit']});
+          next();
+        },
+        function(next) {
+          backend.db.getOps('dogs', id, 0, null, {}, function(error, ops) {
+            if (error) return next(error);
+            // Expect:
+            // v0: create
+            // v1: update
+            // v2: duplicate update
+            expect(ops).to.have.length(3);
+            next();
+          });
+        }
+      ], done);
+    });
+
+    it('only commits one of two identical ops submitted from different clients', function(done) {
+      var backend = this.backend;
+      backend.doNotCommitNoOps = true;
+      var doc1 = backend.connect().get('dogs', id);
+      var doc2 = backend.connect().get('dogs', id);
+      var op = [{p: ['tricks', 0], ld: 'fetch'}];
+
+      async.series([
+        doc1.create.bind(doc1, {tricks: ['fetch', 'sit']}),
+        doc2.fetch.bind(doc2),
+        async.parallel.bind(async.parallel, [
+          doc1.submitOp.bind(doc1, op),
+          doc2.submitOp.bind(doc2, op)
+        ]),
+        function(next) {
+          expect(doc1.data).to.eql({tricks: ['sit']});
+          expect(doc2.data).to.eql({tricks: ['sit']});
+          next();
+        },
+        function(next) {
+          backend.db.getOps('dogs', id, 0, null, {}, function(error, ops) {
+            if (error) return next(error);
+            // Expect:
+            // v0: create
+            // v1: update
+            // no duplicate update
+            expect(ops).to.have.length(2);
+            next();
+          });
+        }
+      ], done);
+    });
+
+    it('fixes up even if an op is fixed up to become a no-op', function(done) {
+      var backend = this.backend;
+      backend.doNotCommitNoOps = true;
+      var doc = backend.connect().get('dogs', id);
+
+      backend.use('apply', function(req, next) {
+        req.$fixup([{p: ['fixme'], od: true}]);
+        next();
+      });
+
+      async.series([
+        doc.create.bind(doc, {}),
+        doc.submitOp.bind(doc, [{p: ['fixme'], oi: true}]),
+        function(next) {
+          expect(doc.data).to.eql({});
+          next();
+        }
+      ], done);
+    });
+
     describe('type.deserialize', function() {
       it('can create a new doc', function(done) {
         var doc = this.backend.connect().get('dogs', id);