From 834d0b2479914d99e0237cb023295fd4c1f28f68 Mon Sep 17 00:00:00 2001 From: Bryce Lee Date: Fri, 1 Jul 2022 16:16:58 -0700 Subject: [PATCH] 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 --- .../service/ObservableServiceConnection.java | 10 ++++---- .../service/PersistentConnectionManager.java | 5 ++++ .../ObservableServiceConnectionTest.java | 24 +++++++++++++++++++ .../PersistentConnectionManagerTest.java | 19 +++++++++++++++ 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 c57dbe37ca5f3..064c224b0568d 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 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 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 292c062369c1d..6e19bed49626a 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 { @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 22d7273dcd20b..046ad1293521d 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 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 53d4a96b06403..db0139c9b0d19 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; @@ -119,6 +120,24 @@ public class PersistentConnectionManagerTest extends SysuiTestCase { } } + /** + * Ensures manual unbind does not reconnect. + */ + @Test + public void testStopDoesNotReconnect() { + mConnectionManager.start(); + ArgumentCaptor> 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. */