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

Fix establishing of call connection on HPB #3177

Merged
merged 1 commit into from
Jul 12, 2023

Conversation

mahibi
Copy link
Collaborator

@mahibi mahibi commented Jul 12, 2023

fix #1725
fix #1712

Fix establishing of call connection when try to connect a second time on HPB

Everytime a second attempt was made to enter a call, the connection failed.
How to reproduce:

  • Enter the ChatActivity
    -> joins the room (so the new session is in the ApplicationWideCurrentRoomHolder)

  • Start call
    -> in the CallActivity we don't join again and instead execute callOrJoinRoomViaWebSocket()

  • Call connection is successful

  • Hangup on android
    -> the ApplicationWideCurrentRoomHolder gets cleared (so also it's session)

  • Staying in the chat and start the call another time
    -> When we open CallActivity another time, ApplicationWideCurrentRoomHolder.sessionId is empty.Because of this, in joinRoomAndCall, joinRoom is executed again.
    But as we are still in the room and have a session, joinRoom is problematic because on serverside in SignalingController - if there is still a session - it's considered as old.
    So Nextcloud now sends a backend message (disinvite) to the external signaling controller that the session (of the first join) was removed.
    So the External signaling server removes the session and closes the websocket. (The message for this might be improved, see Improve log message in case of disinvite strukturag/nextcloud-spreed-signaling#512)

As the websocket is now closed, it won't be possible for the android app to send any signaling message anymore. There will just be the connecting screen and the call connection fails.

Solution for now:

ApplicationWideCurrentRoomHolder.getInstance().clear() should not be executed when hanging up, so the session won't be cleared and in the next attempt to start the call the room is not joined again mistakenly.
Instead to clear the ApplicationWideCurrentRoomHolder, only

setInCall(false);    
setDialing(false);

are set so that the method isNotInCall() in ChatActivity remains working correctly.

Followup TODOs

  • followup improvements regarding the isConnected state in WebSocketInstance
  • check lifecycle behaviour of ChatActivity together with ApplicationWideCurrentRoomHolder. When leaving ChatActivity sessionIdAfterRoomJoined remains currently while it's empty in ApplicationWideCurrentRoomHolder ....
  • check session in com.nextcloud.talk.webrtc.WebSocketInstance#joinRoomWithRoomTokenAndSession (not only roomToken)
  • ....

🏁 Checklist

  • ⛑️ Tests (unit and/or integration) are included or not needed
  • 🔖 Capability is checked or not needed
  • 🔙 Backport requests are created or not needed: /backport to stable-xx.x
  • 📅 Milestone is set
  • 🌸 PR title is meaningful (if it should be in the changelog: is it meaningful to users?)

… on HPB

Everytime a second attempt was made to enter a call, the connection failed.
How to reproduce:

- Enter the ChatActivity
-> joins the room (so the new session is in the ApplicationWideCurrentRoomHolder)

- Start call
-> in the CallActivity we don't join again and instead execute callOrJoinRoomViaWebSocket()

- Call connection is successful

- Hangup on android
-> the ApplicationWideCurrentRoomHolder gets cleared (so also it's session)

- Staying in the chat and start the call another time
-> When we open CallActivity another time, ApplicationWideCurrentRoomHolder.sessionId is empty.Because of this, in joinRoomAndCall, joinRoom is executed again.
But as we are still in the room and have a session, joinRoom is problematic because on serverside in SignalingController - if there is still a session - it's considered as old.
So Nextcloud now sends a backend message (disinvite) to the external signaling controller that the session (of the first join) was removed.
So the External signaling server removes the session and closes the websocket. (The message for this might be improved, see strukturag/nextcloud-spreed-signaling#512)

As the websocket is now closed, it won't be possible for the android app to send any signaling message anymore. There will just be the connecting screen and the call connection fails.

Solution for now:
ApplicationWideCurrentRoomHolder.getInstance().clear() should not be executed when hanging up, so the session won't be cleared and in the next attempt to start the call the room is not joined again mistakenly.
Instead to clear the `ApplicationWideCurrentRoomHolder`, only
setInCall(false);
setDialing(false);
are set so that the method isNotInCall() in ChatActivity remains working correctly.

Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
@mahibi mahibi added this to the 17.0.2 milestone Jul 12, 2023
@mahibi mahibi self-assigned this Jul 12, 2023
@mahibi mahibi modified the milestones: 17.0.2, 17.1.0 Jul 12, 2023
@mahibi
Copy link
Collaborator Author

mahibi commented Jul 12, 2023

/backport to stable-17.0

@mahibi
Copy link
Collaborator Author

mahibi commented Jul 12, 2023

thanks to @SystemKeeper & @Ivansss !!!

@github-actions
Copy link

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/3177-talk.apk

qrcode

To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud Talk app.

Copy link

@SystemKeeper SystemKeeper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥳

@Byter3
Copy link

Byter3 commented Jul 16, 2023

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/3177-talk.apk

qrcode

To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud Talk app.

Zry this apk file out. Same behaviour on Samsun Galaxy S21 Ultra. Android 13. Nextcloud AIO 6.2.1
Nextcloud Talk: 17.0.1
HPB version (built in AIO): de701dbcae52c77f3f98634cb9a61b15f79674ed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants