From 9318db885c8b98b3cf3232639156efe213338faa Mon Sep 17 00:00:00 2001 From: Alec Gibson <12036746+alecgibson@users.noreply.github.com> Date: Thu, 3 Oct 2024 13:05:44 +0100 Subject: [PATCH] =?UTF-8?q?=E2=9A=A1=EF=B8=8F=20Avoid=20committing=20no-op?= =?UTF-8?q?s=20to=20the=20database?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- lib/client/doc.js | 9 ++++++++ lib/error.js | 1 + lib/submit-request.js | 13 +++++++++++ test/client/submit.js | 53 +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 76 insertions(+) diff --git a/lib/client/doc.js b/lib/client/doc.js index dd702eace..acb457374 100644 --- a/lib/client/doc.js +++ b/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); } diff --git a/lib/error.js b/lib/error.js index 0715faf38..cde95bdbe 100644 --- a/lib/error.js +++ b/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', diff --git a/lib/submit-request.js b/lib/submit-request.js index ff379801c..efde23f38 100644 --- a/lib/submit-request.js +++ b/lib/submit-request.js @@ -204,6 +204,13 @@ SubmitRequest.prototype.commit = function(callback) { }; } + if (request.op.op && request.op.op.length === 0 && request._fixupOps.length === 0) { + // 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 +368,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.' + ); +}; diff --git a/test/client/submit.js b/test/client/submit.js index 8fa098930..3ba32eb05 100644 --- a/test/client/submit.js +++ b/test/client/submit.js @@ -1290,6 +1290,59 @@ module.exports = function() { }); }); + it('only commits one of two identical ops submitted from different clients', function(done) { + var backend = this.backend; + 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']}); + expect(doc1.version).to.equal(2); + expect(doc2.version).to.equal(2); + 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; + 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);