Skip to content

Commit

Permalink
⚡️ Remove server-side Presence transformations
Browse files Browse the repository at this point in the history
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".
  • Loading branch information
alecgibson committed Jan 17, 2022
1 parent aa720e9 commit febb54f
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 44 deletions.
9 changes: 3 additions & 6 deletions lib/agent.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});
};
Expand Down
16 changes: 0 additions & 16 deletions lib/backend.js
Original file line number Diff line number Diff line change
Expand Up @@ -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++) {
Expand Down
34 changes: 12 additions & 22 deletions test/client/presence/doc-presence.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
Expand Down
15 changes: 15 additions & 0 deletions test/ot.js
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down

0 comments on commit febb54f

Please sign in to comment.