From 3eec5a06df818f3ae116fc8402c54ba90e452d27 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. --- docs/api/backend.md | 13 ++++++ docs/api/sharedb-error.md | 6 +++ lib/backend.js | 1 + lib/client/doc.js | 9 ++++ lib/error.js | 1 + lib/protocol.js | 2 +- lib/submit-request.js | 20 +++++++++ test/client/submit.js | 86 +++++++++++++++++++++++++++++++++++++++ 8 files changed, 137 insertions(+), 1 deletion(-) diff --git a/docs/api/backend.md b/docs/api/backend.md index b1db01078..b3efa8b04 100644 --- a/docs/api/backend.md +++ b/docs/api/backend.md @@ -113,6 +113,19 @@ Optional {: .d-inline-block } +`doNotCommitNoOps` -- boolean + +Optional +{: .label .label-grey } + +> Default: `false` + +> If set to `true`, will avoid committing no-ops to the database. Clients that submit no-ops +> (or ops that are transformed to no-ops) will have their ops acknowledged as if they were +> committed, but the doc version will not increment. + +{: .d-inline-block } + `errorHandler` -- Function Optional diff --git a/docs/api/sharedb-error.md b/docs/api/sharedb-error.md index 4dc89fcd0..7b5e90587 100644 --- a/docs/api/sharedb-error.md +++ b/docs/api/sharedb-error.md @@ -100,3 +100,9 @@ Representation of an error, with a machine-parsable [code](#error-codes). ### `ERR_TYPE_CANNOT_BE_PROJECTED` > The document's type cannot be projected. [`json0`]({{ site.baseurl }}{% link types/json0.md %}) is currently the only type that supports projections. + +### `ERR_NO_OP` + +> The submitted op resulted in a no-op, possibly after transformation by a remote op. + +> This is normal behavior and the client should swallow this error without bumping doc version. \ No newline at end of file diff --git a/lib/backend.js b/lib/backend.js index 7a3cd41cc..dcc95aa33 100644 --- a/lib/backend.js +++ b/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); 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/protocol.js b/lib/protocol.js index 208af5b6b..723478490 100644 --- a/lib/protocol.js +++ b/lib/protocol.js @@ -1,6 +1,6 @@ module.exports = { major: 1, - minor: 1, + minor: 2, checkAtLeast: checkAtLeast }; diff --git a/lib/submit-request.js b/lib/submit-request.js index ff379801c..e7e106c77 100644 --- a/lib/submit-request.js +++ b/lib/submit-request.js @@ -2,6 +2,7 @@ var ot = require('./ot'); var projections = require('./projections'); var ShareDBError = require('./error'); var types = require('./types'); +var 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.' + ); +}; diff --git a/test/client/submit.js b/test/client/submit.js index 8fa098930..cc829778b 100644 --- a/test/client/submit.js +++ b/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);