Fix presence messages ending an in-progress sync #319
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
LGTM |
| @@ -1426,5 +1426,67 @@ define(['ably', 'shared_helper', 'async'], function(Ably, helper, async) { | |||
| }); | |||
| }; | |||
|
|
|||
| /* | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
LGTM with a comment |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue reported by Larry where a large room would occasionally show far too few presence members (within a region - separate from inter-region sync issues). Turns out that a presence message arriving in the middle of a sync would interrupt the sync.
(I reused the
broadcastparameter fromsetPresence, since it wasn't actually used anywhere in the function. I'm assuming it's leftover from some previous design for presence and is now defunct)