From 2295a42332be4368fac1a938530b944df8158723 Mon Sep 17 00:00:00 2001 From: Marcel Hibbe Date: Sun, 20 Sep 2026 11:59:41 +0200 Subject: [PATCH] fix(signaling): stop the app from opening parallel signaling WebSockets WebSocketConnectionHelper only reused the cached WebSocketInstance for a user while it reported isConnected() == true. Since the ping interval added in #6543 makes the connection cycle through a brief disconnected state every 30-60s by design, any caller (ChatActivity, CallActivity, WebsocketConnectionsWorker) asking for the instance during that window got handed a brand new WebSocketInstance instead, orphaning the old one which kept retrying unsupervised in the background. Within a single instance, restartWebSocket() could also be triggered concurrently (from sendMessage, processErrorMessage, closeWebSocket) without cancelling any socket already in flight, and a stale/superseded socket's failure would still force a reconnect of the current one. Both issues together produced the "5-13 parallel sockets" / repeated "Closing previous client ... for session X" churn from #6710. Fixes #6710. Assisted-by: Claude Code:claude-sonnet-5 Signed-off-by: Marcel Hibbe --- .../webrtc/WebSocketConnectionHelper.java | 16 ++-- .../talk/webrtc/WebSocketInstance.kt | 29 +++++--- .../webrtc/WebSocketConnectionHelperTest.kt | 73 +++++++++++++++++++ 3 files changed, 102 insertions(+), 16 deletions(-) create mode 100644 app/src/test/java/com/nextcloud/talk/webrtc/WebSocketConnectionHelperTest.kt diff --git a/app/src/main/java/com/nextcloud/talk/webrtc/WebSocketConnectionHelper.java b/app/src/main/java/com/nextcloud/talk/webrtc/WebSocketConnectionHelper.java index 24b31e2586..d6f869f3c2 100644 --- a/app/src/main/java/com/nextcloud/talk/webrtc/WebSocketConnectionHelper.java +++ b/app/src/main/java/com/nextcloud/talk/webrtc/WebSocketConnectionHelper.java @@ -77,17 +77,19 @@ public static synchronized WebSocketInstance getExternalSignalingInstanceForServ long userId = isGuest ? -1 : user.getId(); - WebSocketInstance webSocketInstance = getWebSocketInstanceForUser(user); - - if (userId != -1 && webSocketInstance != null && webSocketInstance.isConnected()) { - return webSocketInstance; - } - if (userId == -1) { deleteExternalSignalingInstanceForUserEntity(userId); + } else { + WebSocketInstance webSocketInstance = getWebSocketInstanceForUser(user); + if (webSocketInstance != null) { + Log.d(TAG, "Reusing webSocketInstance " + webSocketInstance.hashCode() + " for userId " + userId); + return webSocketInstance; + } } - webSocketInstance = new WebSocketInstance(user, generatedURL, webSocketTicket); + Log.d(TAG, "Creating new webSocketInstance for userId " + userId); + + WebSocketInstance webSocketInstance = new WebSocketInstance(user, generatedURL, webSocketTicket); webSocketInstanceMap.put(user.getId(), webSocketInstance); return webSocketInstance; } diff --git a/app/src/main/java/com/nextcloud/talk/webrtc/WebSocketInstance.kt b/app/src/main/java/com/nextcloud/talk/webrtc/WebSocketInstance.kt index 38a2cc4d66..430bce16ba 100644 --- a/app/src/main/java/com/nextcloud/talk/webrtc/WebSocketInstance.kt +++ b/app/src/main/java/com/nextcloud/talk/webrtc/WebSocketInstance.kt @@ -118,18 +118,24 @@ class WebSocketInstance internal constructor(conversationUser: User, connectionU } override fun onOpen(webSocket: WebSocket, response: Response) { - Log.d(TAG, "Open webSocket") + val previousWebSocket = internalWebSocket + Log.d( + TAG, + "Open webSocket ${webSocket.hashCode()} (previous was ${previousWebSocket?.hashCode()})" + ) internalWebSocket = webSocket sendHello() } private fun closeWebSocket(webSocket: WebSocket) { + Log.d(TAG, "closeWebSocket ${webSocket.hashCode()}") webSocket.close(NORMAL_CLOSURE, null) webSocket.cancel() - if (webSocket === internalWebSocket) { - isConnected = false - messagesQueue = ArrayList() + if (webSocket !== internalWebSocket) { + return } + isConnected = false + messagesQueue = ArrayList() sleep(ONE_SECOND) restartWebSocket() } @@ -139,10 +145,14 @@ class WebSocketInstance internal constructor(conversationUser: User, connectionU } fun restartWebSocket() { - reconnecting = true Log.d(TAG, "restartWebSocket: $connectionUrl") + val previousWebSocket = internalWebSocket + isConnected = false + reconnecting = true val request = Request.Builder().url(connectionUrl).build() - signalingHttpClient.newWebSocket(request, this) + internalWebSocket = signalingHttpClient.newWebSocket(request, this) + previousWebSocket?.close(NORMAL_CLOSURE, null) + previousWebSocket?.cancel() } override fun onMessage(webSocket: WebSocket, text: String) { @@ -381,16 +391,17 @@ class WebSocketInstance internal constructor(conversationUser: User, connectionU } override fun onClosing(webSocket: WebSocket, code: Int, reason: String) { - Log.d(TAG, "onClosing : $code / $reason") + Log.d(TAG, "onClosing : WebSocket ${webSocket.hashCode()} $code / $reason") } override fun onClosed(webSocket: WebSocket, code: Int, reason: String) { - Log.d(TAG, "onClosed : $code / $reason") + Log.d(TAG, "onClosed : WebSocket ${webSocket.hashCode()} $code / $reason") isConnected = false } override fun onFailure(webSocket: WebSocket, t: Throwable, response: Response?) { - Log.e(TAG, "Error : WebSocket " + webSocket.hashCode(), t) + val isCurrent = webSocket === internalWebSocket + Log.e(TAG, "Error : WebSocket ${webSocket.hashCode()} (isCurrentInternalWebSocket=$isCurrent)", t) closeWebSocket(webSocket) } diff --git a/app/src/test/java/com/nextcloud/talk/webrtc/WebSocketConnectionHelperTest.kt b/app/src/test/java/com/nextcloud/talk/webrtc/WebSocketConnectionHelperTest.kt new file mode 100644 index 0000000000..b9c5a94fc2 --- /dev/null +++ b/app/src/test/java/com/nextcloud/talk/webrtc/WebSocketConnectionHelperTest.kt @@ -0,0 +1,73 @@ +/* + * Nextcloud Talk - Android Client + * + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: GPL-3.0-or-later + */ +package com.nextcloud.talk.webrtc + +import com.nextcloud.talk.data.user.model.User +import org.junit.After +import org.junit.Assert.assertSame +import org.junit.Test +import org.mockito.kotlin.mock +import org.mockito.kotlin.whenever + +/** + * Regression tests for #6710: [WebSocketInstance] retries by itself, so the helper must reuse an + * already registered instance instead of spawning a second one whenever it is momentarily not + * connected - otherwise the first instance keeps running unsupervised, opening a parallel + * connection to the signaling server for the same user. + */ +class WebSocketConnectionHelperTest { + + @After + fun tearDown() { + instanceMap().clear() + } + + @Suppress("UNCHECKED_CAST") + private fun instanceMap(): MutableMap { + val field = WebSocketConnectionHelper::class.java.getDeclaredField("webSocketInstanceMap") + field.isAccessible = true + return field.get(null) as MutableMap + } + + @Test + fun reusesExistingInstanceEvenWhileItIsStillReconnecting() { + val user = User(id = USER_ID) + val existingInstance = mock() + whenever(existingInstance.isConnected).thenReturn(false) + instanceMap()[USER_ID] = existingInstance + + val result = WebSocketConnectionHelper.getExternalSignalingInstanceForServer( + "https://signaling.example.com", + user, + "ticket", + false + ) + + assertSame(existingInstance, result) + } + + @Test + fun reusesExistingConnectedInstance() { + val user = User(id = USER_ID) + val existingInstance = mock() + whenever(existingInstance.isConnected).thenReturn(true) + instanceMap()[USER_ID] = existingInstance + + val result = WebSocketConnectionHelper.getExternalSignalingInstanceForServer( + "https://signaling.example.com", + user, + "ticket", + false + ) + + assertSame(existingInstance, result) + } + + companion object { + private const val USER_ID = 42L + } +}