Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix presence messages ending an in-progress sync #319

Merged
merged 1 commit into from
Jul 18, 2016
Merged

Fix presence messages ending an in-progress sync #319

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions common/lib/client/realtimechannel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ var RealtimeChannel = (function() {
};

RealtimeChannel.prototype.onMessage = function(message) {
var syncChannelSerial;
var syncChannelSerial, isSync = false;
switch(message.action) {
case actions.ATTACHED:
this.setAttached(message);
Expand All @@ -277,6 +277,8 @@ var RealtimeChannel = (function() {
break;

case actions.SYNC:
/* syncs can have channelSerials, but might not if the sync is one page long */
isSync = true;
syncChannelSerial = this.syncChannelSerial = message.channelSerial;
/* syncs can happen on channels with no presence data as part of connection
* resuming, in which case protocol message has no presence property */
Expand All @@ -300,7 +302,7 @@ var RealtimeChannel = (function() {
if(!presenceMsg.timestamp) presenceMsg.timestamp = timestamp;
if(!presenceMsg.id) presenceMsg.id = id + ':' + i;
}
this.presence.setPresence(presence, true, syncChannelSerial);
this.presence.setPresence(presence, isSync, syncChannelSerial);
break;

case actions.MESSAGE:
Expand Down
13 changes: 9 additions & 4 deletions common/lib/client/realtimepresence.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,11 +212,16 @@ var RealtimePresence = (function() {
Presence.prototype._history.call(this, params, callback);
};

RealtimePresence.prototype.setPresence = function(presenceSet, broadcast, syncChannelSerial) {
RealtimePresence.prototype.setPresence = function(presenceSet, isSync, syncChannelSerial) {
Logger.logAction(Logger.LOG_MICRO, 'RealtimePresence.setPresence()', 'received presence for ' + presenceSet.length + ' participants; syncChannelSerial = ' + syncChannelSerial);
var syncCursor, match, members = this.members, broadcastMessages = [];
if(syncChannelSerial && (match = syncChannelSerial.match(/^\w+:(.*)$/)) && (syncCursor = match[1]))

if(isSync) {
this.members.startSync();
if(syncChannelSerial && (match = syncChannelSerial.match(/^\w+:(.*)$/))) {
syncCursor = match[1];
}
}

for(var i = 0; i < presenceSet.length; i++) {
var presence = PresenceMessage.fromValues(presenceSet[i]);
Expand All @@ -235,8 +240,8 @@ var RealtimePresence = (function() {
break;
}
}
/* if this is the last message in a sequence of sync updates, end the sync */
if(!syncCursor) {
/* if this is the last (or only) message in a sequence of sync updates, end the sync */
if(isSync && !syncCursor) {
members.endSync();
this.channel.setInProgress(RealtimeChannel.progressOps.sync, false);
}
Expand Down
62 changes: 62 additions & 0 deletions spec/realtime/presence.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1426,5 +1426,67 @@ define(['ably', 'shared_helper', 'async'], function(Ably, helper, async) {
});
};

/*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we maybe include tests where the mid-sync message is one of the sync'd members, including both the case that the sync for that member was received already, and the case that it is yet to be received?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, we should, and we do need more tests for sync. Not directly relevant to this change, so since we're about to change how syncing works for the 0.9 spec, I'll add some more sync tests then.

* Do a 110-member sync, so split into two sync messages. Inject a normal
* presence enter between the syncs. Check everything was entered correctly
*/
exports.presence_sync_interruptus = function(test) {
test.expect(1);
var channelName = "presence_sync_interruptus";
var interrupterClientId = "dark_horse";
var enterer = helper.AblyRealtime();
var syncer = helper.AblyRealtime();
var entererChannel = enterer.channels.get(channelName);
var syncerChannel = syncer.channels.get(channelName);

function waitForBothConnect(cb) {
async.parallel([
function(connectCb) { enterer.connection.on('connected', connectCb); },
function(connectCb) { syncer.connection.on('connected', connectCb); }
], function() { cb(); });
}

async.series([
waitForBothConnect,
function(cb) { entererChannel.attach(cb); },
function(cb) {
async.times(110, function(i, presCb) {
entererChannel.presence.enterClient(i.toString(), null, presCb);
}, cb);
},
function(cb) {
var originalOnMessage = syncerChannel.onMessage;
syncerChannel.onMessage = function(message) {
originalOnMessage.apply(this, arguments);
/* Inject an additional presence message after the first sync */
if(message.action === 16) {
syncerChannel.onMessage = originalOnMessage;
syncerChannel.onMessage({
"action": 14,
"id": "messageid-0",
"connectionId": "connid",
"timestamp": 2000000000000,
"presence": [{
"clientId": interrupterClientId,
"action": 'enter'
}]});
}
};
syncerChannel.attach(cb);
},
function(cb) {
syncerChannel.presence.get(function(err, presenceSet) {
test.equal(presenceSet && presenceSet.length, 111, 'Check everyone’s in presence set');
cb(err);
});
}
], function(err) {
if(err) {
test.ok(false, helper.displayError(err));
}
closeAndFinish(test, [enterer, syncer]);
});
};

return module.exports = helper.withTimeout(exports);
});