-
Notifications
You must be signed in to change notification settings - Fork 4
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.
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
Refactor realtime presence spec #166
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -723,7 +723,7 @@ h3(#realtime-presence). RealtimePresence | |
** @(RTP2f)@ If a @SYNC@ is in progress, then when a presence message with an action of @LEAVE@ arrives, it should be stored in the presence map with the action set to @ABSENT@. When the @SYNC@ completes, any @ABSENT@ members should be deleted from the presence map. (This is because in a @SYNC@, we might receive a @LEAVE@ before the corresponding @ENTER@). | ||
** @(RTP2g)@ Any incoming presence message that passes the newness check should be emitted on the @RealtimePresence@ object, with an event name set to its original action. Note: this action may not be the same one that it will have when stored in the presence map. For example: an incoming presence message with an @ENTER@ action will be emitted as an @enter@ event, and the emitted presence message will have its action set to @ENTER@. However, it will be stored in the presence map with a @PRESENT@ action. | ||
* @(RTP18)@ The realtime system reserves the right to initiate a sync of the presence members at any point once a channel is attached. A server initiated sync provides Ably with a means to send a complete list of members present on the channel at any point | ||
** @(RTP18a)@ The client library determines that a new sync has started whenever a @SYNC@ @ProtocolMessage@ is received with a @channel@ attribute and a new sync sequence identifier in the @channelSerial@ attribute. The @channelSerial@ is used as the sync cursor and is a two-part identifier @<sync sequence id>:<cursor value>@. If a new sequence identifier is sent from Ably, then the client library must consider that to be the start of a new sync sequence and any previous in-flight sync should be discarded | ||
** @(RTP18a)@ The client library determines that a sync has started whenever a @SYNC@ @ProtocolMessage@ is received with a @non-empty@ @channelSerial@ attribute. The @channelSerial@ is a two-part identifier @<sync sequence id>:<cursor value>@. If there is an ongoing sync with different @sync sequence id@, it should be ended while discarding related changes and a new sync should be started. | ||
** @(RTP18b)@ The sync operation for that sequence identifier has completed once the cursor is empty; that is, when the @channelSerial@ looks like @<sync sequence id>:@ | ||
** @(RTP18c)@ a @SYNC@ may also be sent with no @channelSerial@ attribute. In this case, the sync data is entirely contained within that @ProtocolMessage@ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @SimonWoolf I have not encountered this particular case [RTP18c] yet. Is this still valid or do we need to remove it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure feel free to remove RTP18c There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we sure about it? This means we have to remove check at
it will become
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, this shouldn't break, since I will be doing this change across dotnet, go and java codebase : ) |
||
* @(RTP19)@ If the @PresenceMap@ has existing members when a @SYNC@ is started, the client library must ensure that members no longer present on the channel are removed from the local @PresenceMap@ once the sync is complete. In order to do this, the client library must keep track of any members that have not been added or updated in the @PresenceMap@ during the sync process. Note that a member can be added or updated when received in a @SYNC@ message or when received in a @PRESENCE@ message during the sync process. Once the sync is complete, the members in the @PresenceMap@ that have not been added or updated should be removed from the @PresenceMap@ and a @LEAVE@ event should be published for each. The @PresenceMessage@ published should contain the original attributes of the presence member with the @action@ set to @LEAVE@, @PresenceMessage#id@ set to @null@, and the @timestamp@ set to the current time. This behavior should be tested as follows: @ENTER@ presence on a channel, wait for @SYNC@ to complete, inject a member directly into the local @PresenceMap@ so that it only exists locally and not on the server, send a @SYNC@ message with the @channel@ attribute populated with the current channel which will trigger a server initiated @SYNC@. A @LEAVE@ event should then be published for the injected member, and checking the @PresenceMap@ should reveal that the member was removed and the valid member entered for this connection is still present | ||
|
@@ -743,9 +743,9 @@ h3(#realtime-presence). RealtimePresence | |
* @(RTP5)@ RealtimeChannel state change side effects: | ||
** @(RTP5a)@ If the channel enters the @DETACHED@ or @FAILED@ state then all queued presence messages will fail immediately, and the @PresenceMap@ and "internal PresenceMap (see RTP17)":#RTP17 is cleared. The latter ensures members are not automatically re-entered if the @RealtimeChannel@ later becomes attached. Since channels in the @DETACHED@ and @FAILED@ states will not receive any presence updates from Ably, presence events (specifically @LEAVE@) should not be emitted when the @PresenceMap@ is cleared as each presence member's state is unknown | ||
*** @(RTP5a1)@ A channel entering the @DETACHED@, @SUSPENDED@, or @FAILED@ state should also clear any @channelSerial@ it has stored per @RTL15b@. | ||
** @(RTP5f)@ If the channel enters the @SUSPENDED@ state then all queued presence messages will fail immediately, and the @PresenceMap@ is maintained. This ensures that if the channel later becomes @ATTACHED@, it will only emit presence events for the changes in the @PresenceMap@ that have occurred whilst the client was disconnected. A test should exist for a channel that is in the @SUSPENDED@ state containing presence members to transition to the @ATTACHED@ state, and following the @SYNC@ process after attaching, any members present before and after the sync should not emit presence events, all other changes should be reflected in the @PresenceMap@ and should emit presence events on the channel | ||
** @(RTP5f)@ If the channel enters the @SUSPENDED@ state then all queued presence messages will fail immediately, and the @PresenceMap@ and "internal PresenceMap (see RTP17)":#RTP17 is maintained. This ensures that if the channel later becomes @ATTACHED@, it will only emit presence events for the changes in the @PresenceMap@ that have occurred (during sync) whilst the channel was @SUSPENDED@. A test should exist for a channel that is in the @SUSPENDED@ state containing presence members to transition to the @ATTACHED@ state, and following the @SYNC@ process after attaching, any members present before and after the sync should not emit presence events, all other changes should be reflected in the @PresenceMap@ and should emit presence events on the channel | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [RTP5f] Do we also need to add a test that says, it should enter internal presence map members to the channel after attached? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This addition I think makes it more confusing. The sdk finds out about the changes during the sync, but they might have happened at any point while the channel was suspended There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Just my opinion, but personally, I don't like the "A test should exist that..." spec items. All the spec items specify behaviour, most behaviour should be tested as normal practice. The spec shouldn't need to explicitly require that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
yeah it's confusing either way, because you have to consider server presencemap and not local presencemap while referring to the statement : ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was confused at first, because for all spec statements, |
||
** @(RTP5b)@ If a channel enters the @ATTACHED@ state then all queued presence messages will be sent immediately. A presence @SYNC@ may be initiated per "@RTP1@":#RTP1 | ||
* @(RTP16)@ Connection state conditions: | ||
* @(RTP16)@ Channel/Connection state conditions side effects while "enter, update and leave presence message (see RTP8, RTP9, RTP10 and RTP14)":#RTP8 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this addition is quite unclear. "conditions side effects", "while enter presence message" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean it's related to enter, update and leave presence message. Problem is, all specs mentioned right above RTP16 are supposed to be applied while processing presence message. This one is while sending the message, and it's not explicitly mentioned. I had to refer to the code to truly understand the context. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In that case I think we can do even better -- after all "enter, update, and leave presence messages" doesn't really clarify, those can all be received too. How about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, this makes more sense |
||
** @(RTP16a)@ If the channel is @ATTACHED@ then the presence messages are handled per @RTL6c2@. (That is: they should be sent to the connection, to be published immediately if the connection is @CONNECTED@, else if the connection state and @queueMessages@ option allows they may be placed in a connection-wide queue to be published once the connection becomes @CONNECTED@, else rejected) | ||
** @(RTP16b)@ If the channel is @ATTACHING@ or @INITIALIZED@, then if @ClientOptions.queueMessages@ has not been explicitly set to @false@, the presence messages should be queued at a channel level, to be handled per @RTP16a@ once the channel becomes @ATTACHED@, or failed if the channel state becomes @SUSPENDED@, @FAILED@, or @DETACHED@ first | ||
** @(RTP16c)@ In any other case the operation should result in an error | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes it sound like the sdk needs to remember what the presence set was like before the previous sync and revert back to it, which is not necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, what is supposed to be done instead? That's the only thing I can think of, reverting presencemap state to previous sync state
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or do we need to wait for current sync to complete and apply the next immediate sync 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eg clearing the state that needs to be tracked for RTP19, removing any members that you don't see in the sync.