Pass null to Java chat interrupted cbs if intentional

signalapp · Jan 17, 2025 · 0b5650f · 0b5650f
1 parent c82fbe8
commit 0b5650f
Show file tree

Hide file tree

Showing 6 changed files with 100 additions and 43 deletions.
diff --git a/java/client/src/main/java/org/signal/libsignal/net/ChatConnection.java b/java/client/src/main/java/org/signal/libsignal/net/ChatConnection.java
@@ -72,9 +72,11 @@ public void onConnectionInterrupted(Throwable disconnectReason) {
       if (chat.chatListener == null) return;
 
       ChatServiceException disconnectReasonChatServiceException =
-          (disconnectReason instanceof ChatServiceException)
-              ? (ChatServiceException) disconnectReason
-              : new ChatServiceException("OtherDisconnectReason", disconnectReason);
+          (disconnectReason == null)
+              ? null
+              : (disconnectReason instanceof ChatServiceException)
+                  ? (ChatServiceException) disconnectReason
+                  : new ChatServiceException("OtherDisconnectReason", disconnectReason);
       chat.chatListener.onConnectionInterrupted(chat, disconnectReasonChatServiceException);
     }
   }

diff --git a/java/client/src/main/java/org/signal/libsignal/net/ChatConnectionListener.java b/java/client/src/main/java/org/signal/libsignal/net/ChatConnectionListener.java
@@ -34,7 +34,8 @@ default void onQueueEmpty(ChatConnection chat) {}
   /**
    * Called when the client gets disconnected from the server.
    *
-   * <p>This includes both deliberate disconnects as well as unexpected socket closures.
+   * <p>This includes both deliberate disconnects as well as unexpected socket closures. In the case
+   * of the former, the {@param disconnectReason} will be null.
    *
    * <p>The default implementation of this method does nothing.
    */

diff --git a/java/client/src/main/java/org/signal/libsignal/net/ChatListener.java b/java/client/src/main/java/org/signal/libsignal/net/ChatListener.java
@@ -34,8 +34,8 @@ default void onQueueEmpty(ChatService chat) {}
   /**
    * Called when the client gets disconnected from the server.
    *
-   * <p>This includes both deliberate disconnects as well as unexpected socket closures that will be
-   * automatically retried.
+   * <p>This includes both deliberate disconnects as well as unexpected socket closures. In the case
+   * of the former, the {@param disconnectReason} will be null.
    *
    * <p>Will not be called if no other requests have been invoked for this connection attempt. That
    * is, you should never see this as the first callback, nor two of these callbacks in a row.

diff --git a/java/client/src/main/java/org/signal/libsignal/net/ChatService.java b/java/client/src/main/java/org/signal/libsignal/net/ChatService.java
@@ -78,9 +78,11 @@ public void onConnectionInterrupted(Throwable disconnectReason) {
       if (chat.chatListener == null) return;
 
       ChatServiceException disconnectReasonChatServiceException =
-          (disconnectReason instanceof ChatServiceException)
-              ? (ChatServiceException) disconnectReason
-              : new ChatServiceException("OtherDisconnectReason", disconnectReason);
+          (disconnectReason == null)
+              ? null
+              : (disconnectReason instanceof ChatServiceException)
+                  ? (ChatServiceException) disconnectReason
+                  : new ChatServiceException("OtherDisconnectReason", disconnectReason);
       chat.chatListener.onConnectionInterrupted(chat, disconnectReasonChatServiceException);
     }
   }

diff --git a/java/client/src/test/java/org/signal/libsignal/net/ChatServiceTest.java b/java/client/src/test/java/org/signal/libsignal/net/ChatServiceTest.java
@@ -14,9 +14,11 @@
 import java.util.Map;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
-import java.util.function.Function;
+import java.util.function.BiFunction;
 import org.junit.Assume;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.Timeout;
 import org.junit.runner.RunWith;
 import org.junit.runners.Parameterized;
 import org.junit.runners.Parameterized.Parameter;
@@ -136,26 +138,62 @@ public void testConstructRequest() throws Exception {
 
   @RunWith(Parameterized.class)
   public static class ConnectTests {
+    private static class Listener implements ChatListener, ChatConnectionListener {
+      CompletableFuture<ChatServiceException> disconnectReason = new CompletableFuture<>();
+
+      public void onConnectionInterrupted(ChatService chat, ChatServiceException disconnectReason) {
+        this.disconnectReason.complete(disconnectReason);
+      }
+
+      public void onConnectionInterrupted(
+          ChatConnection chat, ChatServiceException disconnectReason) {
+        this.disconnectReason.complete(disconnectReason);
+      }
+
+      public void onIncomingMessage(
+          ChatService chat,
+          byte[] envelope,
+          long serverDeliveryTimestamp,
+          ChatListener.ServerMessageAck sendAck) {
+        throw new AssertionError("Unexpected incoming message");
+      }
+
+      public void onIncomingMessage(
+          ChatConnection chat,
+          byte[] envelope,
+          long serverDeliveryTimestamp,
+          ChatConnectionListener.ServerMessageAck sendAck) {
+        throw new AssertionError("Unexpected incoming message");
+      }
+    }
+    ;
+
+    @Rule public Timeout perCaseTimeout = new Timeout(15, TimeUnit.SECONDS);
+
     @Parameters
-    public static Iterable<Function<Network, CompletableFuture<Void>>> connectUnauthFns() {
-      ArrayList<Function<Network, CompletableFuture<Void>>> fns = new ArrayList<>(2);
+    public static Iterable<BiFunction<Network, Listener, CompletableFuture<Void>>>
+        connectUnauthFns() {
+      ArrayList<BiFunction<Network, Listener, CompletableFuture<Void>>> fns = new ArrayList<>(2);
       fns.add(
-          (Network net) -> {
+          (Network net, Listener listener) -> {
             // Chat service
-            final UnauthenticatedChatService chat = net.createUnauthChatService(null);
-            return chat.connect()
-                .thenCompose((ChatService.DebugInfo debugInfo) -> chat.disconnect());
+            final UnauthenticatedChatService chat = net.createUnauthChatService(listener);
+            return chat.connect().thenCompose(debugInfo -> chat.disconnect());
           });
       fns.add(
-          (Network net) -> {
+          (Network net, Listener listener) -> {
             // Chat connection
-            return net.connectUnauthChat(null)
-                .thenCompose((UnauthenticatedChatConnection chat) -> chat.disconnect());
+            return net.connectUnauthChat(listener)
+                .thenCompose(
+                    chat -> {
+                      chat.start();
+                      return chat.disconnect();
+                    });
           });
       return fns;
     }
 
-    @Parameter public Function<Network, CompletableFuture<Void>> connectUnauthChat;
+    @Parameter public BiFunction<Network, Listener, CompletableFuture<Void>> connectUnauthChat;
 
     @Test
     public void testConnectUnauth() throws Exception {
@@ -165,7 +203,11 @@ public void testConnectUnauth() throws Exception {
       Assume.assumeNotNull(ENABLE_TEST);
 
       final Network net = new Network(Network.Environment.STAGING, USER_AGENT);
-      this.connectUnauthChat.apply(net).get();
+      final Listener listener = new Listener();
+      Void disconnectFinished = this.connectUnauthChat.apply(net, listener).get();
+
+      ChatServiceException disconnectReason = listener.disconnectReason.get();
+      assertNull(disconnectReason);
     }
 
     @Test
@@ -187,7 +229,11 @@ public void testConnectUnauthThroughProxy() throws Exception {
           throw new IllegalArgumentException("invalid LIBSIGNAL_TESTING_PROXY_SERVER");
       }
 
-      this.connectUnauthChat.apply(net).get();
+      final Listener listener = new Listener();
+      Void disconnectFinished = this.connectUnauthChat.apply(net, listener).get();
+
+      ChatServiceException disconnectReason = listener.disconnectReason.get();
+      assertNull(disconnectReason);
     }
   }
   ;

diff --git a/rust/bridge/shared/types/src/jni/chat.rs b/rust/bridge/shared/types/src/jni/chat.rs
@@ -87,27 +87,33 @@ impl ChatListener for JniChatListener {
     fn connection_interrupted(&mut self, disconnect_cause: ChatServiceError) {
         let listener = &self.listener;
         self.attach_and_log_on_error("connection interrupted", move |env| {
-            convert_to_exception(
-                env,
-                SignalJniError::from(disconnect_cause),
-                move |env, throwable, _error| {
-                    throwable
-                        .and_then(move |throwable| {
-                            call_method_checked(
-                                env,
-                                listener,
-                                "onConnectionInterrupted",
-                                jni_args!((throwable => java.lang.Throwable) -> void),
-                            )?;
-                            Ok(())
-                        })
-                        .unwrap_or_else(|error| {
-                            log::error!(
-                                "failed to call onConnectionInterrupted with cause: {error}"
-                            );
-                        });
-                },
-            );
+            let throw_exception = move |env, listener, throwable: JThrowable<'_>| {
+                call_method_checked(
+                    env,
+                    listener,
+                    "onConnectionInterrupted",
+                    jni_args!((throwable => java.lang.Throwable) -> void),
+                )?;
+                Ok(())
+            };
+            match disconnect_cause {
+                ChatServiceError::ServiceIntentionallyDisconnected => {
+                    throw_exception(env, listener, JObject::null().into())?
+                }
+                disconnect_cause => convert_to_exception(
+                    env,
+                    SignalJniError::from(disconnect_cause),
+                    move |env, throwable, _error| {
+                        throwable
+                            .and_then(|throwable| throw_exception(env, listener, throwable))
+                            .unwrap_or_else(|error| {
+                                log::error!(
+                                    "failed to call onConnectionInterrupted with cause: {error}"
+                                );
+                            });
+                    },
+                ),
+            };
             Ok(())
         });
     }