Refactor realtime presence spec #166
Conversation
sacOO7
changed the title
| @@ -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.
@SimonWoolf I have not encountered this particular case [RTP18c] yet. Is this still valid or do we need to remove it?
As of now, I mostly get channelSerial in the format sdfsdf: with all presence data. So, first two cases RTP18a and RTP18b are covered. But third one is not covered : (
There was a problem hiding this comment.
sure feel free to remove RTP18c
There was a problem hiding this comment.
Are we sure about it? This means we have to remove check at
https://github.com/ably/ably-dotnet/blob/b359101412f57844c09b8ae276d49008a7adfbd9/src/IO.Ably.Shared/Realtime/Presence.cs#L609
So instead of
if (syncChannelSerial == null || syncCursor.Length <= 1)
it will become
if (syncCursor.IsEmpty())
There was a problem hiding this comment.
Okay, this shouldn't break, since I will be doing this change across dotnet, go and java codebase : )
sacOO7
changed the title
| @@ -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.
[RTP5f] Do we also need to add a test that says, it should enter internal presence map members to the channel after attached?
I think it probably should get covered as a part of RTP17f
There was a problem hiding this comment.
the changes in the @PresenceMap@ that have occurred (during sync)
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.
Do we also need to add a test that says, it should enter internal presence map members to the channel after attached? I think it probably should get covered as a part of RTP17f
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.
the changes in the @PresenceMap@ that have occurred (during sync)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
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.
I was confused at first, because for all spec statements, presencemap refers to the local one, except this statement.
| @@ -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. | |||
There was a problem hiding this comment.
discarding related changes
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.
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.
Or do we need to wait for current sync to complete and apply the next immediate sync 🤔
There was a problem hiding this comment.
So, what is supposed to be done instead? That's the only thing I can think of, reverting presencemap state to previous sync state
eg clearing the state that needs to be tracked for RTP19, removing any members that you don't see in the sync.
| @@ -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.
sure feel free to remove RTP18c
| @@ -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.
the changes in the @PresenceMap@ that have occurred (during sync)
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
| ** @(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.
this addition is quite unclear. "conditions side effects", "while enter presence message"
There was a problem hiding this comment.
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.
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 @(RTP16)@ Effect of channel state on enter(), update(), and leave() methods?
There was a problem hiding this comment.
yeah, this makes more sense
Link to internal discussion https://ably-real-time.slack.com/archives/C030C5YLY/p1693863972631609