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.

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

Presence re-entry requirement change for 1.1 #878

Open
paddybyers opened this issue Jul 23, 2019 · 7 comments
Open

Presence re-entry requirement change for 1.1 #878

paddybyers opened this issue Jul 23, 2019 · 7 comments
Assignees
Labels
bug Something isn't working. It's clear that this does need to be fixed.

Comments

@paddybyers
Copy link
Member

paddybyers commented Jul 23, 2019

See ably/docs#711.

After confirming that the lib satisfies the new RTP17c, please update the spec spreadsheet accordingly

┆Issue is synchronized with this Jira Bug by Unito

@paddybyers paddybyers added bug Something isn't working. It's clear that this does need to be fixed. important labels Jul 23, 2019
@ricardopereira
Copy link
Contributor

@paddybyers I'm reviewing this and I think it's updated but maybe I'm missing something. So, on every SYNC message (RTP17c1) the library will do a sync and if the message is the last serial then an automatic re-entry is performed.

Now, the library does a sync when the ATTACHED message has false in HAS_PRESENCE flag (RTP17c2) but it checks some extra checks:

if (message.hasPresence) {
[self.presenceMap startSync];
}
else if ([self.presenceMap.members count] > 0 || [self.presenceMap.localMembers count] > 0) {
if (!message.resumed) {
// When an ATTACHED message is received without a HAS_PRESENCE flag and PresenceMap has existing members
[self.presenceMap startSync];
[self.presenceMap endSync];
[self.logger debug:__FILE__ line:__LINE__ message:@"R:%p C:%p (%@) PresenceMap has been reset", _realtime, self, self.name];
}
}

Is this still valid?
Or the library should do an automatic re-entry every time the HAS_PRESENCE is false?

@ricardopereira
Copy link
Contributor

@paddybyers bump ^

@AndyNicks
Copy link

@paddybyers Hey Paddy, can you have a look at Ricardo's comment please

@lukasz-szyszkowski
Copy link
Contributor

As far as I understand it we shouldn't do any additional checks like here:

}
else if ([self.presenceMap.members count] > 0 || [self.presenceMap.localMembers count] > 0) {
if (!message.resumed) {

according to RTP17f

The RealtimePresence object should perform automatic re-entry whenever a channel moves into the ATTACHED state. (It does not need to do so for RTL12 ATTACHED events received on already-ATTACHED channels).

Am I correct @paddybyers?

@lawrence-forooghian
Copy link
Collaborator

@maratal is currently working on #1494, which updates the library to use protocol version 2 and specification version 2.0.0. This specification version simplifies the criteria for performing an automatic re-entry, replacing RTP17c with RTP17f. I would suggest that we park the current bug and close it once we've implemented RTP17f as part of #1494.

@lawrence-forooghian
Copy link
Collaborator

Hey @maratal, would you mind confirming whether you agree with my above assessment, that #1494 will resolve this issue? I'll assign this one to you, to close after #1494. If you disagree with my assessment feel free to assign back to me.

@maratal
Copy link
Collaborator

maratal commented May 25, 2023

@lawrence-forooghian Correct, RTP17c was replaced with RTP17f and implemented in #1714

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working. It's clear that this does need to be fixed.
Development

No branches or pull requests

7 participants