From d63bb3159522a35275c354569833ce905259fac7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Mon, 9 Dec 2024 18:36:22 +0100 Subject: [PATCH] Fix "send" not respecting order of pending messages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the data channel is not open yet data channel messages are queued and then sent once opened. "onStateChange" is called from the WebRTC signaling thread, while "send" can be called potentially from any thread, so to send the data channel messages in the same order that they were added new messages need to be enqueued until all the pending messages have been sent. Otherwise, even if there is synchronization already, it could happen that "onStateChange" was called but, before getting the lock, "send" gets it and sends the new message before the pending messages were sent. Signed-off-by: Daniel Calviño Sánchez --- .../com/nextcloud/talk/webrtc/PeerConnectionWrapper.java | 9 +++++++-- .../nextcloud/talk/webrtc/PeerConnectionWrapperTest.kt | 5 ++++- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/webrtc/PeerConnectionWrapper.java b/app/src/main/java/com/nextcloud/talk/webrtc/PeerConnectionWrapper.java index 13bb5f2f7..9f2a90f85 100644 --- a/app/src/main/java/com/nextcloud/talk/webrtc/PeerConnectionWrapper.java +++ b/app/src/main/java/com/nextcloud/talk/webrtc/PeerConnectionWrapper.java @@ -298,7 +298,8 @@ public class PeerConnectionWrapper { } DataChannel statusDataChannel = dataChannels.get("status"); - if (statusDataChannel == null || statusDataChannel.state() != DataChannel.State.OPEN) { + if (statusDataChannel == null || statusDataChannel.state() != DataChannel.State.OPEN || + !pendingDataChannelMessages.isEmpty()) { Log.d(TAG, "Queuing data channel message (" + dataChannelMessage + ") " + sessionId); pendingDataChannelMessages.add(dataChannelMessage); @@ -306,6 +307,10 @@ public class PeerConnectionWrapper { return; } + sendWithoutQueuing(statusDataChannel, dataChannelMessage); + } + + private void sendWithoutQueuing(DataChannel statusDataChannel, DataChannelMessage dataChannelMessage) { try { Log.d(TAG, "Sending data channel message (" + dataChannelMessage + ") " + sessionId); @@ -423,7 +428,7 @@ public class PeerConnectionWrapper { if (dataChannel.state() == DataChannel.State.OPEN && "status".equals(dataChannelLabel)) { for (DataChannelMessage dataChannelMessage : pendingDataChannelMessages) { - send(dataChannelMessage); + sendWithoutQueuing(dataChannel, dataChannelMessage); } pendingDataChannelMessages.clear(); } diff --git a/app/src/test/java/com/nextcloud/talk/webrtc/PeerConnectionWrapperTest.kt b/app/src/test/java/com/nextcloud/talk/webrtc/PeerConnectionWrapperTest.kt index f4f7e6443..b6dd40962 100644 --- a/app/src/test/java/com/nextcloud/talk/webrtc/PeerConnectionWrapperTest.kt +++ b/app/src/test/java/com/nextcloud/talk/webrtc/PeerConnectionWrapperTest.kt @@ -23,6 +23,7 @@ import org.mockito.Mockito.atLeast import org.mockito.Mockito.atMostOnce import org.mockito.Mockito.doAnswer import org.mockito.Mockito.doNothing +import org.mockito.Mockito.inOrder import org.mockito.Mockito.never import org.mockito.invocation.InvocationOnMock import org.mockito.stubbing.Answer @@ -288,8 +289,10 @@ class PeerConnectionWrapperTest { throw exceptionOnStateChange!! } + val inOrder = inOrder(mockedStatusDataChannel) + for (j in 1..dataChannelMessageCount) { - Mockito.verify(mockedStatusDataChannel).send( + inOrder.verify(mockedStatusDataChannel).send( argThat(MatchesDataChannelMessage(DataChannelMessage("the-message-type-$j"))) ) }