From f9ef1ecfc470fd5337bfc15ce62f002df81b616c Mon Sep 17 00:00:00 2001 From: Alec Gibson <12036746+alecgibson@users.noreply.github.com> Date: Mon, 17 Jan 2022 17:14:54 +0000 Subject: [PATCH] =?UTF-8?q?=E2=9A=A1=EF=B8=8F=20Remove=20server-side=20Pre?= =?UTF-8?q?sence=20transformations?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This change addresses a performance issue with Presence, whereby every single Presence update will cause a Database call to fetch the latest ops, and transform Presence up. This can be quite costly if there are a lot of Presence updates, and adversely impact the "normal" operation of ShareDB. This is particularly strange for the "happy case", where two clients are exchanging Presence on the same version of a document (where there should be no need to query the database at all for Presence). We already have mechanisms in the client to transform presence, as well as re-request stale presence, so we can remove the server-side Presence transformations all together. This alleviates pressure on the server, at the cost of decreased performance in edge cases, where clients need to re-request Presence from other clients, or catch up on cached ops. However, this should be a sensible change to make, since we're trying to optimise for the happy path, rather than edge cases. Note that this changes a test case: we have to resubscribe `doc1` in this test case, otherwise it's never up-to-date, and `presence2` continues to throw out its presence updates. This could happen in a real-world situation (where `doc1` stays offline), but it's arguable that this is uninteresting presence anyway, since that client is not "live". --- lib/agent.js | 9 +++----- lib/backend.js | 16 ------------- test/client/presence/doc-presence.js | 34 ++++++++++------------------ test/ot.js | 15 ++++++++++++ 4 files changed, 30 insertions(+), 44 deletions(-) diff --git a/lib/agent.js b/lib/agent.js index ebb3bc7d9..0f3c33891 100644 --- a/lib/agent.js +++ b/lib/agent.js @@ -748,13 +748,10 @@ Agent.prototype._broadcastPresence = function(presence, callback) { }; backend.trigger(backend.MIDDLEWARE_ACTIONS.receivePresence, this, context, function(error) { if (error) return callback(error); - backend.transformPresenceToLatestVersion(agent, presence, function(error, presence) { + var channel = agent._getPresenceChannel(presence.ch); + agent.backend.pubsub.publish([channel], presence, function(error) { if (error) return callback(error); - var channel = agent._getPresenceChannel(presence.ch); - agent.backend.pubsub.publish([channel], presence, function(error) { - if (error) return callback(error); - callback(null, presence); - }); + callback(null, presence); }); }); }; diff --git a/lib/backend.js b/lib/backend.js index 017d49de2..c21aa6642 100644 --- a/lib/backend.js +++ b/lib/backend.js @@ -858,22 +858,6 @@ Backend.prototype._buildSnapshotFromOps = function(id, startingSnapshot, ops, ca callback(error, snapshot); }; -Backend.prototype.transformPresenceToLatestVersion = function(agent, presence, callback) { - if (!presence.c || !presence.d) return callback(null, presence); - this.getOps(agent, presence.c, presence.d, presence.v, null, function(error, ops) { - if (error) return callback(error); - for (var i = 0; i < ops.length; i++) { - var op = ops[i]; - var isOwnOp = op.src === presence.src; - var transformError = ot.transformPresence(presence, op, isOwnOp); - if (transformError) { - return callback(transformError); - } - } - callback(null, presence); - }); -}; - function pluckIds(snapshots) { var ids = []; for (var i = 0; i < snapshots.length; i++) { diff --git a/test/client/presence/doc-presence.js b/test/client/presence/doc-presence.js index ea0bdfe71..4cb7714e8 100644 --- a/test/client/presence/doc-presence.js +++ b/test/client/presence/doc-presence.js @@ -232,38 +232,28 @@ describe('DocPresence', function() { var localPresence1 = presence1.create('presence-1'); async.series([ + presence1.subscribe.bind(presence1), presence2.subscribe.bind(presence2), doc1.unsubscribe.bind(doc1), doc2.submitOp.bind(doc2, {index: 5, value: 'ern'}), function(next) { expect(doc1.version).to.eql(1); expect(doc2.version).to.eql(2); - + next(); + }, + localPresence1.submit.bind(localPresence1, {index: 12}), + function(next) { localPresence1.submit({index: 12}, errorHandler(done)); - presence2.once('receive', function(id, presence) { - expect(doc2.version).to.eql(2); - expect(presence).to.eql({index: 15}); - next(); - }); - } - ], done); - }); - it('returns errors when failing to transform old presence to the latest doc', function(done) { - var localPresence1 = presence1.create('presence-1'); + var handlePresence = function(id, presence) { + if (presence.index !== 15) return; + presence2.off('receive', handlePresence); + next(); + }; - async.series([ - presence2.subscribe.bind(presence2), - doc1.unsubscribe.bind(doc1), - doc2.submitOp.bind(doc2, {index: 5, value: 'ern'}), - function(next) { - expect(doc1.version).to.eql(1); - expect(doc2.version).to.eql(2); + presence2.on('receive', handlePresence); - localPresence1.submit({badProp: 'foo'}, function(error) { - expect(error.code).to.equal('ERR_PRESENCE_TRANSFORM_FAILED'); - next(); - }); + doc1.subscribe(errorHandler(next)); } ], done); }); diff --git a/test/ot.js b/test/ot.js index 090ea0365..e8cfa05b8 100644 --- a/test/ot.js +++ b/test/ot.js @@ -316,6 +316,21 @@ describe('ot', function() { expect(error.code).to.eql('ERR_OT_OP_BADLY_FORMED'); }); + it('returns a transformation error if the presence cannot be transformed', function () { + var presence = { + p: {index: 5}, + t: presenceType.uri, + v: 1 + }; + + var op = { + op: {badProp: 'foo'} + }; + var error = ot.transformPresence(presence, op); + + expect(error.code).to.eql('ERR_PRESENCE_TRANSFORM_FAILED'); + }); + it('considers isOwnOp', function() { var presence = { p: {index: 5},