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

RFC: only emit a leave if there was an existing member #211

Open
SimonWoolf opened this issue Oct 17, 2024 · 4 comments
Open

RFC: only emit a leave if there was an existing member #211

SimonWoolf opened this issue Oct 17, 2024 · 4 comments

Comments

@SimonWoolf
Copy link
Member

Proposal: have the spec specify that you only emit a leave event if a member was removed.

Presence events are only emitted by the SDK if they pass a 'newness check': RTP2g. The general idea is that presence is a CRDT, and you don't need to emit an event that does not change your local presence set state, only newer versions of the presence message for a particular member (which might container newer versions of the data).

That reasoning would also imply that we should only emit a leave event if a matching member was previously present in the local view of the presence set, else the leave does not cause a change in local state. But we don't currently do that. Why not? This feels like an obvious thing to do, I feel like I must be missing something?

@AndyTWF
Copy link
Contributor

AndyTWF commented Oct 17, 2024

Would we be able to provide guarantees around which presence event would cause the emission?

For example, if a client explicitly leaves presence and immediately detaches (causing an implicit leave), is there a guarantee here that the explicit leave will always win-out? The implicit leave would have whatever data is currently in the presence set, which may be "outdated" compared to any data sent with leave. I can imagine a use-case where someone might depend on the leave data, which they won't get if the implicit wins. I believe the answer to the above is "yes", but wanted to double check!

Besides that, this seems like a reasonable proposal.

@SimonWoolf
Copy link
Member Author

For example, if a client explicitly leaves presence and immediately detaches (causing an implicit leave), is there a guarantee here that the explicit leave will always win-out?

Realtime transports preserve order. So if you leave before sending a detach, then yes, the explicit leave should arrive first.

(That said, if someone's designed something that depends on the client being able to specify the data in the leave event, that's a bit of a design smell. Half the point of presence as a feature is that the service will leave on your behalf if you eg have internet issues and disconnect abruptly, and in those situations the client clearly can't control the leave data)

@SimonWoolf
Copy link
Member Author

This was discussed in the RTF and agreed, I will turn this into a spec pr

SimonWoolf added a commit that referenced this issue Nov 13, 2024
…ng member

per #211

The actual behaviour change here is small (only whether non-sync leaves
where there is no matching member currently present should emit a leave
event), but I found the current structure of this spec section quite
confusing (why are broadcast and members-map add/remove requirements
defined separately, despite having the same requirements?), so ended up
rewriting it. (Without deprecating the old spec items since this isn't
actually a behaviour change except in that one case)
SimonWoolf added a commit that referenced this issue Nov 13, 2024
…ng member

per #211

The actual behaviour change here is small (only whether non-sync leaves
where there is no matching member currently present should emit a leave
event), but I found the current structure of this spec section quite
confusing (why are broadcast and members-map add/remove requirements
defined separately, despite having the same requirements?), so ended up
rewriting it. (Without deprecating the old spec items since this isn't
actually a behaviour change except in that one case)
SimonWoolf added a commit that referenced this issue Nov 13, 2024
…ng member

per #211

The actual behaviour change here is small (only whether non-sync leaves
where there is no matching member currently present should emit a leave
event), but I found the current structure of this spec section quite
confusing (why are broadcast and members-map add/remove requirements
defined separately, despite having the same requirements?), so ended up
rewriting it. (Without deprecating the old spec items since this isn't
actually a behaviour change except in that one case)
@SimonWoolf
Copy link
Member Author

#222

SimonWoolf added a commit that referenced this issue Nov 13, 2024
…ng member

per #211

The actual behaviour change here is small (only whether non-sync leaves
where there is no matching member currently present should emit a leave
event), but I found the current structure of this spec section quite
confusing (why are broadcast and members-map add/remove requirements
defined separately, despite having the same requirements?), so ended up
rewriting it. (Without deprecating the old spec items since this isn't
actually a behaviour change except in that one case)
SimonWoolf added a commit that referenced this issue Nov 28, 2024
…ng member

per #211

The actual behaviour change here is small (only whether non-sync leaves
where there is no matching member currently present should emit a leave
event), but I found the current structure of this spec section quite
confusing (why are broadcast and members-map add/remove requirements
defined separately, despite having the same requirements?), so ended up
rewriting it. (Without deprecating the old spec items since this isn't
actually a behaviour change except in that one case)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants