From febb54f1926c7f9bc5110c4cce7f258ccb1d4ad2 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 ebb3bc7d..0f3c3389 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 017d49de..c21aa664 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 ea0bdfe7..4cb7714e 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 090ea036..323a6306 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},