feat(signaling): Don't send all room userids in signaling server requests - #16759
Conversation
df831a0 to
0b5509d
Compare
nickvergessen
added
3. to review
feature: api 🛠️
|
What's the exact merge order? I assume:
|
Yes, that must be checked first.
Here the order doesn't really matter. If the whole user lists are no longer sent by Talk, the HPB will not forward them / skip some notifications (see strukturag/nextcloud-spreed-signaling#1175 (comment)). If the HPB doesn't parse them, it will only send the changed ones / skip some notifications. |
91db64d to
2193758
Compare
2193758 to
5d8adfc
Compare
danxuliu
left a comment
danxuliu
left a comment
There was a problem hiding this comment.
(Sorry beforehand for the wall of text that follows)
Breaking changes
Calls no longer work if users is not sent in roomInCallChanged and the updated signaling server is not used. The current signaling server sends the list of all participants in the call to the clients1 based on users2, but if users is not sent then it can only send the updated participants. Therefore, if a participant joins a call, that participant only gets a signaling message with itself, but it does not know any participant already in the call. The updated signaling server solves that by internally tracking the participants in the call instead of relying on the users parameter.
1 Signaling message participants->update where users includes all the participants in the call
2 Despite the name users again includes participants, both users and guests
As mentioned in the Tasks in this pull request description, the signaling server should add a new feature id so Talk can check if a supported version is running. If it is not, then users need to be sent in roomInCallChanged.
Nevertheless, I think that right now we do not store anywhere the features available in the signaling server to act upon that in the backend. Sending a request to the signaling server to get the features before actually sending the actual request would be overkill, so maybe the features could be cached whenever the signaling server sends a request to the Nextcloud server (as if I am not mistaken the header with the features is sent in all requests to the api/{apiVersion}/signaling/backend endpoint) 🤔
Messages that could still be too long
Even with the changes in this pull request there are still some places that could cause a too long message to be sent to the signaling server:
- Related to user ids:
- Inviting many (thousands...) users at the same time could still cause a too large message to be sent due to the userids of the invited users. Note that this could happen not only when adding many users explicitly, but also when adding a group.
- Similarly, disinviting many users at the same time could also cause a too large message to be sent due to the userids of the disinvited users. Again this can happen also when removing a group.
- User ids can be, at most, 64 bytes long, so the issues with the userids could start to appear with 256Kb/64B = ~4000 participants if they all have the longest ids possible. But with shorter ids then it would require even more participants to trigger the issues.
- Related to session ids:
- Modifying the room permissions in a conversation with a large number of active participants could cause an error, as each active participant in the conversation needs to be notified about the change.
- Starting or stopping breakout rooms could also fail due to the session IDs. But... I would not expect this to be a problem in the real world, as using breakout rooms with thousands of participants sounds unlikely.
- Strictly speaking it would happen too when extending a room with many participants, as the
switchtomessage is also sent in that case, but like with breakout rooms using it with thousands of participants sounds unlikely. - Session ids can be, at most, 512 bytes long, and they will always be at least 255 bytes long, so the issues would happen in best case with ~1000 participants and ~500 in the worst case.
The most "likely" error to occur would be when changing the room permissions. Other cases will either require a really large number of participants, or an uncommon usage of Talk.
Possible fix to handle updates of many participants (for example, when changing the room permissions)
-
The signaling server could fetch the list of participants in a room for certain events, rather than receiving it from the Nextcloud server. The Nextcloud server needs to be able to provide the list of participants to the clients, even if it is very large, so it should not be a problem if the signaling server also requests it (and an additional API endpoint could be also added with only the data needed by the signaling server).
- Regarding how large the response can be, each participant data takes around ~250 bytes without actor ID, display name, session ID and status parameters (the status is not included when there are more than 1000 participants in the conversation, but it would take at least 72 bytes with all fields empty/null). Therefore for participants that are not active it can be ~250 + 64 + 64 = ~378 bytes at most, and for active participants it can be ~250 + 64 + 64 + 512 = ~890 bytes at most (with a single session and without status), although in most cases the values will be of course lower; for active participants it will always be at least ~250 + 1 + 1 + 255 = ~507 bytes.
- Nevertheless, if the HTTP response is gziped, an uncompressed ~255kiB response for 1000 inactive participants can be transmitted with only ~11kiB. However, an uncompressed ~512kiB response for 1000 active participants needs ~215kiB (probably because the data is much less regular in this case)
-
If pagination is needed in the list of participants using just a count and offset could be problematic, because if you get the first 100 participants and then one of them is removed when you get the next 100 participants the previous 101 will be now 100, and thus will be missed in the next request. Using a count and the last timestamp could be problematic as well, because there could be several participants with the last timestamp (for example, many participants invited to a room that never joined it, which will have 0 as timestamp), so if you ask for 100 participants but participants 100, 101, 102, 103... all have the same timestamp they should be returned as well, potentially making the request too large. So I am not sure what would be a safe way to paginate the participants, although we can think about that once/if it is implemented.
Previously existing issues
-
When a participant is modified the Nextcloud server notifies the signaling server, and the signaling server sends a
participants->updateto all active participants in the conversation. However,BackendNotifier::participantsModifiedonly includes the data of the active participants too, so when the modified participant is not activeparticipants->updatedoes not include its data (and, with the new code, there is not even aparticipants->updatemessage, as it would be empty).- Due to that the rest of participants are patched in the list, but not the one that was actually modified. An update is debounced for 60 seconds, so in the end it will get updated with the new fetch, but it is of course not immediate.
- Maybe this could be fixed by basing the
changedparameter inBackendNotifier::participantsModifiedon the actor type and id rather than on the session id, so the data also includes participants not active in the room.
-
Modifying some room properties does not cause
BackendNotifier::roomModifiedto be called, so the signaling server can not send a signaling message to the clients either to notify them. This is the case, for example, if the room avatar changes or if a call starts in the room.
Acceptable? changes (as, in most cases, they should happen only until the signaling server is updated)
-
Signaling message
roomlist->updatewithupdate->properties->participant-refresh = trueis no longer received by all the users in the conversation when a user is added to a conversation, a guest joins a conversation, a guest leaves a conversation, a guest or user is removed from a conversation (either themselves or by a moderator), or a participant is promoted/demoted (this last case is a legacy behaviour that was expected to be eventually removed, and it does not include theparticipant-refreshparameter)- For users that are not active in the conversation the missing message makes no difference, as they will update the participant list anyway once they join.
- For users that are active in the conversation the participant list will be outdated until something else causes the list to be got again (for example, a participant joining or leaving). Therefore, if a user is added or an inactive user is removed the participant list will not be automatically updated, although in other cases the list will be updated (although it can take up to a minute) as adding/removing also includes joining/leaving. Unfortunately the system message about a participant being added/removed and the out of sync participant list could be confusing.
-
Signaling message
roomlist->disinvitewithdisinvite->reason = deletedis no longer received by all the users in the conversation when a conversation is deleted.- Users (other than the deleter) active in the conversation when it is deleted does not exit the removed conversation, and they eventually end in 429 Too many requests due to repeated requests to endpoints related to the deleted conversation. Problematic due to the 429, but I am not sure if worth an explicit fix besides updating the signaling server 🤔
-
Signaling message
roomlist->updateis no longer received by all the users in the conversation when a room is modified.- For users not currently in the room if the conversation name, description, type, lobby state or listable state is modified they will not be notified and their conversation list will be outdated until the next forced refresh
- Note that the conversation list being outdated also includes the conversation settings, which can be opened from the conversation list without being in the conversation, and use the cached values rather than fetching them when opening
- In the case of lobby state, besides the settings, being outdated means showing the last message instead of No messages, or No messages instead of The admin opened the conversation (there is no visual indication in the conversation list about the lobby state)
- In the case of listable state, besides the settings, being outdated means showing the last message instead of XXX opened the conv... / XXX limited the conv... (there is no visual indication in the conversation list about the listable state)
- For participants not currently in the conversation is irrelevant, as the full list of open conversations is got when listing them
- All this still applies with the new signaling server
- Maybe the signaling server could get the list of participants from the Nextcloud server and then send the signaling messages to the active sessions based on the actor type and id?
- For users not currently in the room if the conversation name, description, type, lobby state or listable state is modified they will not be notified and their conversation list will be outdated until the next forced refresh
-
Signaling message
participants->updateis no longer received by all participants active in the conversation when an inactive participant is modified- The participant list of all users (except the moderator triggering the change) will be outdated until something else causes it to be fetched again. Unfortunately the system message about a participant being promoted/demoted and the out of sync participant list could be confusing.
-
Signaling message
participants->updateis still received by all participants active in the conversation when an active participant is modified, butusersnow includes only the changed participant- This should not be a problem, as only the changed participants should be relevant
-
Signaling message
participants->updatewithusersas a list of all the active participants is no longer received by all the active participants when guests are cleaned.- This should not be a problem, as it will clean only guests that do not have a name, and independently of that the guests will no longer be listed once their session expired, so once they leave the conversation the participant list will be eventually updated and no longer show them.
Summary
- Must be fixed
- Send
usersparameter inroomInCallChangedwhen a signaling server that does not include strukturag/nextcloud-spreed-signaling#1175 is used
- Send
- Would be nice to fix (follow-ups)
- Handle updates of many participants that can still cause an error
- Include inactive participants in the list of changed participants in
BackendNotifier::participantsModified() - Notify missing room changes, like avatars or in call status
- Notify inactive participants when a room is modified
I'll take care of that, the rest we handle as follow up. Please note that I did not read the entire comment as it is too long. So if there is something in between that should be known, please repeat it in a condense/concise single comment |
All that applies to that is in the Breaking changes section. The other sections are different things. |
5d8adfc to
9231694
Compare
|
Adjusted to old behaviour by default, based on an appconfig. 
|
|
danxuliu
left a comment
danxuliu
left a comment
There was a problem hiding this comment.
Other than the comments, tested and works* 👍
* Works in the sense of having the same behaviour as before if an old signaling server is used. The follow-ups still need to be addressed.
| } | ||
|
|
||
| if ($session->getInCall() !== Participant::FLAG_DISCONNECTED) { | ||
| if (!$supportsChangedUsers && $session->getInCall() !== Participant::FLAG_DISCONNECTED) { |
There was a problem hiding this comment.
supportsChangedUsers should be unneeded here, or it should be also added when assigning users in participantsModified for consistency.
There was a problem hiding this comment.
I added it, so it's easier to spot that once the feature is required we can drop the code block
There was a problem hiding this comment.
Then it should be added in
spreed/lib/Signaling/BackendNotifier.php
Line 361 in b339da1
spreed/lib/Signaling/BackendNotifier.php
Line 380 in b339da1
| if (!$this->appConfig->getAppValueBool(Manager::HAS_FEATURE_CHANGED_USERS)) { | ||
| if ($this->signalingManager->hasFeature($response, 'changed-users')) { | ||
| $this->appConfig->setAppValueBool(Manager::HAS_FEATURE_CHANGED_USERS, true); | ||
| } | ||
| } |
There was a problem hiding this comment.
Any specific reason to use a "ratchet" (so it never goes back once enabled) instead of setting the value based on the actual header? Although a signaling server downgrade or having more than one would be unlikely it is nevertheless possible.
There was a problem hiding this comment.
Any specific reason to use a "ratchet" (so it never goes back once enabled) instead of setting the value based on the actual header?
So we can drop the config completely once we require the feature (most likely in an upcoming version)
Although a signaling server downgrade or having more than one would be unlikely it is nevertheless possible.
Having multiple yields a setupcheck error by now, so this is fine.
There was a problem hiding this comment.
Any specific reason to use a "ratchet" (so it never goes back once enabled) instead of setting the value based on the actual header?
So we can drop the config completely once we require the feature (most likely in an upcoming version)
But the configuration can still be dropped once the feature is required even if it keeps track of the actual changed-users header with something like the following, no? 🤔 :
if ($this->appConfig->getAppValueBool(Manager::HAS_FEATURE_CHANGED_USERS) !== $this->signalingManager->hasFeature($response, 'changed-users')) {
$this->appConfig->setAppValueBool(Manager::HAS_FEATURE_CHANGED_USERS, $this->signalingManager->hasFeature($response, 'changed-users'));
}
Although a signaling server downgrade or having more than one would be unlikely it is nevertheless possible.
Having multiple yields a setupcheck error by now, so this is fine.
I did not mean multiple configured, but multiple signaling servers that could be switched between them. Like the one in s10 and the one in talk.meeting.
…ests Signed-off-by: Joachim Bauch <bauch@struktur.de>
Signed-off-by: Joas Schilling <coding@schilljs.com>
This allows Talk to switch the mode so the all user ids arrays are skipped from various requests to allow scaling. Signed-off-by: Joas Schilling <coding@schilljs.com>
a25aed1 to
7367a60
Compare
|
|
|
/backport to stable34 |
3 participants
☑️ Resolves
alluseridsand similar lists from the signaling messages #13815🛠️ API Checklist
🚧 Tasks
🏁 Checklist
docs/has been updated or is not required