Fix stop behavior in PersistentConnectionManager.

PersistentConnectionManager tries to maintain a connection whenever
disconnected. Currently, there is no way to disconnect from this
connection as the manually unbind is not treated any differently than
any other disconnection. This change addresses this oversight by
suppressing reconnect logic on manual unbinds.

This change also fixes a callback issue where listeners were not
informed about disconnects when manually unbound.

Fixes: 237803638
Test: ObservableServiceConnectionTest#testUnbind
Test: PersistentConnectionManagerTest#testStopDoesNotReconnect
Change-Id: I0887e31754429cc02391df5783868d5c132a48eb

4 files changed, 52 insertions, 6 deletions

diff --git a/packages/SystemUI/src/com/android/systemui/util/service/ObservableServiceConnection.java b/packages/SystemUI/src/com/android/systemui/util/service/ObservableServiceConnection.java
index c57dbe37ca5f..064c224b0568 100644
--- a/packages/SystemUI/src/com/android/systemui/util/service/ObservableServiceConnection.java
+++ b/packages/SystemUI/src/com/android/systemui/util/service/ObservableServiceConnection.java
@@ -155,11 +155,6 @@ public class ObservableServiceConnection<T> implements ServiceConnection {
      * Disconnect from the service if bound.
      */
     public void unbind() {
-        if (!mBoundCalled) {
-            return;
-        }
-        mBoundCalled = false;
-        mContext.unbindService(this);
         onDisconnected(DISCONNECT_REASON_UNBIND);
     }
 
@@ -210,12 +205,15 @@ public class ObservableServiceConnection<T> implements ServiceConnection {
             Log.d(TAG, "onDisconnected:" + reason);
         }
 
+        // If not bound or already unbound, do not proceed setting reason, unbinding, and
+        // notifying
         if (!mBoundCalled) {
             return;
         }
 
+        mBoundCalled = false;
         mLastDisconnectReason = Optional.of(reason);
-        unbind();
+        mContext.unbindService(this);
         mProxy = null;
 
         applyToCallbacksLocked(callback-> callback.onDisconnected(this,
diff --git a/packages/SystemUI/src/com/android/systemui/util/service/PersistentConnectionManager.java b/packages/SystemUI/src/com/android/systemui/util/service/PersistentConnectionManager.java
index 292c062369c1..6e19bed49626 100644
--- a/packages/SystemUI/src/com/android/systemui/util/service/PersistentConnectionManager.java
+++ b/packages/SystemUI/src/com/android/systemui/util/service/PersistentConnectionManager.java
@@ -72,6 +72,11 @@ public class PersistentConnectionManager<T> {
 
         @Override
         public void onDisconnected(ObservableServiceConnection connection, int reason) {
+            // Do not attempt to reconnect if we were manually unbound
+            if (reason == ObservableServiceConnection.DISCONNECT_REASON_UNBIND) {
+                return;
+            }
+
             if (mSystemClock.currentTimeMillis() - mStartTime > mMinConnectionDuration) {
                 initiateConnectionAttempt();
             } else {
diff --git a/packages/SystemUI/tests/src/com/android/systemui/util/service/ObservableServiceConnectionTest.java b/packages/SystemUI/tests/src/com/android/systemui/util/service/ObservableServiceConnectionTest.java
index 22d7273dcd20..046ad1293521 100644
--- a/packages/SystemUI/tests/src/com/android/systemui/util/service/ObservableServiceConnectionTest.java
+++ b/packages/SystemUI/tests/src/com/android/systemui/util/service/ObservableServiceConnectionTest.java
@@ -145,4 +145,28 @@ public class ObservableServiceConnectionTest extends SysuiTestCase {
         connection.unbind();
         verify(mContext, never()).unbindService(eq(connection));
     }
+
+    @Test
+    public void testUnbind() {
+        ObservableServiceConnection<Foo> connection = new ObservableServiceConnection<>(mContext,
+                mIntent, mExecutor, mTransformer);
+        connection.addCallback(mCallback);
+        connection.onServiceDisconnected(mComponentName);
+
+        // Disconnects before binds should be ignored.
+        verify(mCallback, never()).onDisconnected(eq(connection), anyInt());
+
+        when(mContext.bindService(eq(mIntent), anyInt(), eq(mExecutor), eq(connection)))
+                .thenReturn(true);
+        connection.bind();
+
+        mExecutor.runAllReady();
+
+        connection.unbind();
+
+        mExecutor.runAllReady();
+
+        verify(mCallback).onDisconnected(eq(connection),
+                eq(ObservableServiceConnection.DISCONNECT_REASON_UNBIND));
+    }
 }
diff --git a/packages/SystemUI/tests/src/com/android/systemui/util/service/PersistentConnectionManagerTest.java b/packages/SystemUI/tests/src/com/android/systemui/util/service/PersistentConnectionManagerTest.java
index 53d4a96b0640..db0139c9b0d1 100644
--- a/packages/SystemUI/tests/src/com/android/systemui/util/service/PersistentConnectionManagerTest.java
+++ b/packages/SystemUI/tests/src/com/android/systemui/util/service/PersistentConnectionManagerTest.java
@@ -16,6 +16,7 @@
 
 package com.android.systemui.util.service;
 
+import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
 
 import android.testing.AndroidTestingRunner;
@@ -120,6 +121,24 @@ public class PersistentConnectionManagerTest extends SysuiTestCase {
     }
 
     /**
+     * Ensures manual unbind does not reconnect.
+     */
+    @Test
+    public void testStopDoesNotReconnect() {
+        mConnectionManager.start();
+        ArgumentCaptor<ObservableServiceConnection.Callback<Proxy>> connectionCallbackCaptor =
+                ArgumentCaptor.forClass(ObservableServiceConnection.Callback.class);
+
+        verify(mConnection).addCallback(connectionCallbackCaptor.capture());
+        verify(mConnection).bind();
+        Mockito.clearInvocations(mConnection);
+        mConnectionManager.stop();
+        mFakeExecutor.advanceClockToNext();
+        mFakeExecutor.runAllReady();
+        verify(mConnection, never()).bind();
+    }
+
+    /**
      * Ensures rebind on package change.
      */
     @Test