Fix inconsistent state when leaving a call fails #7799
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When a call is left the UI always changes to the "not-in-a-call" state. However, if leaving the call fails the signaling did not emit the "leaveCall" event, so the WebRTC connections were not stopped due to leaving the call, nor they were left later when the signaling sent the updated list of participants, as the participant did not leave the call from the point of view of the server. Even if leaving the call fails in the server the call should be locally left when the user requests it, so now "leaveCall" is emitted even if leaving the call failed. Signed-off-by: Daniel Calviño Sánchez <[email protected]>
When "usersChanged(signaling, [], previousUsersInRoom)" is called "previousUsersInRoom" is assigned to the difference with itself and, therefore, to an empty array, so there is no need to explicitly do it afterwards. Signed-off-by: Daniel Calviño Sánchez <[email protected]>
When a call is left the peer connections are immediately ended. However, the data of the participants (like the call state), the CallParticipantModels as well as the timers to retry failed connections were cleared once the signaling message updating the current participant state to disconnected was received. If leaving the call failed in the server that message will not be received, so pending connections were retried even if no longer in the call, and when joining again a previous model could be reused, leading to different issues. Due to that the call related data is now immediately cleared when the call is left, no matter if the signaling message about leaving the call is then received or not (and, if it is, nothing will happen as the data was already cleared). Signed-off-by: Daniel Calviño Sánchez <[email protected]>
When a participant was not in the call but received an offer it was ignored to avoid establishing a connection. However, all the other call signaling messages were still processed. Due to this, if a participant leaves the call but that fails in the server that participant still receives all the participant updates (which also list that participant as still in the call) and therefore that participant tries to establish a connection with the other participants. To prevent that now all call signaling messages are ignored when the local participant is not in the call. Due to this the call signaling message that changes the state from the local participant to disconnected is now ignored as well, although that should not be a problem given that (since the previous commmit) the call related data is immediately cleared when the call is left, without waiting for the signaling message. Signed-off-by: Daniel Calviño Sánchez <[email protected]>
danxuliu
added
3. to review
bug
feature: frontend 🖌️
This was referenced Aug 26, 2022
nickvergessen
approved these changes
Aug 26, 2022
nickvergessen
deleted the
fix-inconsistent-state-when-leaving-a-call-fails
branch
Member
Author
|
/backport to stable24 |
Member
Author
|
/backport to stable23 |
This was referenced Aug 26, 2022
danxuliu
added a commit
that referenced
this pull request
Aug 26, 2022
This should have been part of 3af5e86, but the backport of #6948 was not merged yet when the backport of #7799 was created. Signed-off-by: Daniel Calviño Sánchez <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When a call is left a request to leave the call is sent to the server and then the UI changes to the not-in-a-call state. Even if the request sent to the server fails (which sometimes happens with flaky connections) the layout is changed (failures are ignored in the service). However, if the request sent to the server fails the call was not left from the WebRTC side, because the
leaveCallevent was triggered only on success. Due to this the UI implied that the call was left, but the RTCPeerConnections were all still active.This pull request unifies the behaviour to always locally leave the call (both in the UI and the WebRTC side), even if the request sent to the server failed, as forcing the user to stay in the call until it was possible to properly leave it did not seem like a good approach. If the request to leave fails then that participant will still appear as in the call for the rest of participants, but now the connections will be ended and not restarted (unless the participant joins the call again).
Except otherwise noted, the scenarios below can be tested with or without HPB.
How to test (scenario 1)
return new DataResponse([], Http::STATUS_NOT_FOUND);to the beginning of CallController::leaveCallResult with this pull request
The participant from the private window can not be heard in the original window; the participant from the original window can not be heard nor seen in the private window
Result without this pull request
The participant from the private window can still be heard in the original window; the participant from the original window can be heard and seen in the private window
How to test (scenario 2)
return new DataResponse([], Http::STATUS_NOT_FOUND);to the beginning of CallController::leaveCallResult with this pull request
The participant from the private window can not be heard in the original window; the participant from the original window can not be heard nor seen in the private window
Result without this pull request
The participant from the private window can be heard in the original window; the participant from the original window can be heard and seen in the private window
How to test (scenario 3)
return new DataResponse([], Http::STATUS_NOT_FOUND);to the beginning of CallController::leaveCallResult with this pull request
The participant from the private window can be heard and seen in the original window
Note that, without HPB, in some rare cases the connection may be established yet nothing is visible; I have not been able to fix that (and it could even be a bug in the RTCPeerConnection implementation of the browser, as in my experience ICE restarts are not very reliable), so something for later.
Result with only first commit
The participant from the private window appears in the original window, but no connection is tried to be established and therefore can not be heard nor seen
How to test (scenario 4)
$this->messages->addMessage($message...withif ($decodedMessage['type'] !== 'offer') { ... }return new DataResponse([], Http::STATUS_NOT_FOUND);to the beginning of CallController::leaveCallResult with this pull request
Offers are no longer sent to try to establish a connection
Result without this pull request
New offers are sent again and again to try to establish a connection