From f437806637efabb72b77d765f4710eabef908c69 Mon Sep 17 00:00:00 2001 From: Bryce Lee Date: Fri, 19 Nov 2021 20:02:10 -0800 Subject: [PATCH] Do not repeatedly connect to flaky communal sources. This changelist treats communal sources that disconnect shortly after connection as a connection failure. This causes reconnections to be capped until either restart or package update. Bug: 207167612 Test: atest com.android.systemui.communal.CommunalSourcePrimerTest Change-Id: I253937b06949c9b5c4bf30b5c048ed9c354fcbf1 --- packages/SystemUI/res/values/config.xml | 3 ++ .../communal/CommunalSourcePrimer.java | 16 +++++++- .../communal/CommunalSourcePrimerTest.java | 41 ++++++++++++++++++- 3 files changed, 57 insertions(+), 3 deletions(-) diff --git a/packages/SystemUI/res/values/config.xml b/packages/SystemUI/res/values/config.xml index 8b59d86ee73af..b3bbd87c63f99 100644 --- a/packages/SystemUI/res/values/config.xml +++ b/packages/SystemUI/res/values/config.xml @@ -682,6 +682,9 @@ attempts. --> 1000 + + 1000 + false diff --git a/packages/SystemUI/src/com/android/systemui/communal/CommunalSourcePrimer.java b/packages/SystemUI/src/com/android/systemui/communal/CommunalSourcePrimer.java index 3c2b79e25467e..10442396c835d 100644 --- a/packages/SystemUI/src/com/android/systemui/communal/CommunalSourcePrimer.java +++ b/packages/SystemUI/src/com/android/systemui/communal/CommunalSourcePrimer.java @@ -25,6 +25,7 @@ import com.android.systemui.R; import com.android.systemui.dagger.SysUISingleton; import com.android.systemui.dagger.qualifiers.Main; import com.android.systemui.util.concurrency.DelayableExecutor; +import com.android.systemui.util.time.SystemClock; import com.google.common.util.concurrent.ListenableFuture; @@ -43,10 +44,12 @@ public class CommunalSourcePrimer extends CoreStartable { private static final String TAG = "CommunalSourcePrimer"; private static final boolean DEBUG = Log.isLoggable(TAG, Log.DEBUG); + private final SystemClock mSystemClock; private final DelayableExecutor mMainExecutor; private final CommunalSourceMonitor mMonitor; private final int mBaseReconnectDelayMs; private final int mMaxReconnectAttempts; + private final int mMinConnectionDuration; private int mReconnectAttempts = 0; private Runnable mCurrentReconnectCancelable; @@ -65,11 +68,13 @@ public class CommunalSourcePrimer extends CoreStartable { @Inject public CommunalSourcePrimer(Context context, @Main Resources resources, + SystemClock clock, DelayableExecutor mainExecutor, CommunalSourceMonitor monitor, Optional connector, Optional observer) { super(context); + mSystemClock = clock; mMainExecutor = mainExecutor; mMonitor = monitor; mConnector = connector; @@ -79,6 +84,8 @@ public class CommunalSourcePrimer extends CoreStartable { R.integer.config_communalSourceMaxReconnectAttempts); mBaseReconnectDelayMs = resources.getInteger( R.integer.config_communalSourceReconnectBaseDelay); + mMinConnectionDuration = resources.getInteger( + R.integer.config_connectionMinDuration); } @Override @@ -145,10 +152,17 @@ public class CommunalSourcePrimer extends CoreStartable { mGetSourceFuture = mConnector.get().connect(); mGetSourceFuture.addListener(() -> { try { + final long startTime = mSystemClock.currentTimeMillis(); Optional result = mGetSourceFuture.get(); if (result.isPresent()) { final CommunalSource source = result.get(); - source.addCallback(() -> initiateConnectionAttempt()); + source.addCallback(() -> { + if (mSystemClock.currentTimeMillis() - startTime > mMinConnectionDuration) { + initiateConnectionAttempt(); + } else { + scheduleConnectionAttempt(); + } + }); mMonitor.setSource(source); } else { scheduleConnectionAttempt(); diff --git a/packages/SystemUI/tests/src/com/android/systemui/communal/CommunalSourcePrimerTest.java b/packages/SystemUI/tests/src/com/android/systemui/communal/CommunalSourcePrimerTest.java index 659b1a31a7a7b..2ed38a4f36024 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/communal/CommunalSourcePrimerTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/communal/CommunalSourcePrimerTest.java @@ -50,6 +50,7 @@ public class CommunalSourcePrimerTest extends SysuiTestCase { private static final String TEST_COMPONENT_NAME = "com.google.tests/.CommunalService"; private static final int MAX_RETRIES = 5; private static final int RETRY_DELAY_MS = 1000; + private static final int CONNECTION_MIN_DURATION_MS = 5000; @Mock private Context mContext; @@ -57,7 +58,8 @@ public class CommunalSourcePrimerTest extends SysuiTestCase { @Mock private Resources mResources; - private FakeExecutor mFakeExecutor = new FakeExecutor(new FakeSystemClock()); + private FakeSystemClock mFakeClock = new FakeSystemClock(); + private FakeExecutor mFakeExecutor = new FakeExecutor(mFakeClock); @Mock private CommunalSource mSource; @@ -80,10 +82,14 @@ public class CommunalSourcePrimerTest extends SysuiTestCase { .thenReturn(MAX_RETRIES); when(mResources.getInteger(R.integer.config_communalSourceReconnectBaseDelay)) .thenReturn(RETRY_DELAY_MS); + when(mResources.getInteger(R.integer.config_communalSourceReconnectBaseDelay)) + .thenReturn(RETRY_DELAY_MS); when(mResources.getString(R.string.config_communalSourceComponent)) .thenReturn(TEST_COMPONENT_NAME); + when(mResources.getInteger(R.integer.config_connectionMinDuration)) + .thenReturn(CONNECTION_MIN_DURATION_MS); - mPrimer = new CommunalSourcePrimer(mContext, mResources, mFakeExecutor, + mPrimer = new CommunalSourcePrimer(mContext, mResources, mFakeClock, mFakeExecutor, mCommunalSourceMonitor, Optional.of(mConnector), Optional.of(mObserver)); } @@ -123,6 +129,36 @@ public class CommunalSourcePrimerTest extends SysuiTestCase { verify(mCommunalSourceMonitor, never()).setSource(Mockito.notNull()); } + @Test + public void testRetryOnDisconnectFailure() throws Exception { + when(mConnector.connect()).thenReturn( + CallbackToFutureAdapter.getFuture(completer -> { + completer.set(Optional.of(mSource)); + return "test"; + })); + + mPrimer.onBootCompleted(); + mFakeExecutor.runAllReady(); + + // Verify attempts happen. Note that we account for the retries plus initial attempt, which + // is not scheduled. + for (int attemptCount = 0; attemptCount < MAX_RETRIES + 1; attemptCount++) { + verify(mConnector, times(1)).connect(); + clearInvocations(mConnector); + ArgumentCaptor callbackCaptor = + ArgumentCaptor.forClass(CommunalSource.Callback.class); + verify(mSource).addCallback(callbackCaptor.capture()); + clearInvocations(mSource); + verify(mCommunalSourceMonitor).setSource(Mockito.notNull()); + clearInvocations(mCommunalSourceMonitor); + callbackCaptor.getValue().onDisconnected(); + mFakeExecutor.advanceClockToNext(); + mFakeExecutor.runAllReady(); + } + + verify(mConnector, never()).connect(); + } + @Test public void testAttemptOnPackageChange() { when(mConnector.connect()).thenReturn( @@ -161,6 +197,7 @@ public class CommunalSourcePrimerTest extends SysuiTestCase { verify(mSource).addCallback(callbackCaptor.capture()); clearInvocations(mConnector); + mFakeClock.advanceTime(CONNECTION_MIN_DURATION_MS + 1); callbackCaptor.getValue().onDisconnected(); mFakeExecutor.runAllReady();