From 25ac426406355636c178e1e43c81cce9fe188d75 Mon Sep 17 00:00:00 2001 From: Evan Laird Date: Wed, 7 Dec 2022 09:13:17 -0500 Subject: [PATCH] [Status bar refactor] Fix cache removal exception The old method of removing unused subscriptions from the repo cache was causing a CME Test: DemoMobileConnectionsRepositoryTest Test: MobileConnectionsRepositoryTest Fixes: 261706421 Change-Id: I4977aa47591a7d4888c176e420cc1a0c7f783591 --- .../demo/DemoMobileConnectionsRepository.kt | 11 ++--- .../prod/MobileConnectionsRepositoryImpl.kt | 11 ++--- .../DemoMobileConnectionsRepositoryTest.kt | 19 ++++++++ .../prod/MobileConnectionsRepositoryTest.kt | 47 ++++++++++++++++++- 4 files changed, 75 insertions(+), 13 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/demo/DemoMobileConnectionsRepository.kt b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/demo/DemoMobileConnectionsRepository.kt index b463796671b69..1e7fae717a2de 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/demo/DemoMobileConnectionsRepository.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/demo/DemoMobileConnectionsRepository.kt @@ -61,7 +61,7 @@ constructor( private var demoCommandJob: Job? = null - private val connectionRepoCache = mutableMapOf() + private var connectionRepoCache = mutableMapOf() private val subscriptionInfoCache = mutableMapOf() val demoModeFinishedEvent = MutableSharedFlow(extraBufferCapacity = 1) @@ -76,11 +76,10 @@ constructor( // will get garbage collected once their subscribers go away val currentValidSubscriptionIds = newInfos.map { it.subscriptionId } - connectionRepoCache.keys.forEach { - if (!currentValidSubscriptionIds.contains(it)) { - connectionRepoCache.remove(it) - } - } + connectionRepoCache = + connectionRepoCache + .filter { currentValidSubscriptionIds.contains(it.key) } + .toMutableMap() } private fun maybeCreateSubscription(subId: Int) { diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileConnectionsRepositoryImpl.kt b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileConnectionsRepositoryImpl.kt index 5c0b40f073e83..f27a9c9cca987 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileConnectionsRepositoryImpl.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileConnectionsRepositoryImpl.kt @@ -87,7 +87,7 @@ constructor( @Application private val scope: CoroutineScope, private val mobileConnectionRepositoryFactory: MobileConnectionRepositoryImpl.Factory ) : MobileConnectionsRepository { - private val subIdRepositoryCache: MutableMap = mutableMapOf() + private var subIdRepositoryCache: MutableMap = mutableMapOf() /** * State flow that emits the set of mobile data subscriptions, each represented by its own @@ -264,11 +264,10 @@ constructor( // will get garbage collected once their subscribers go away val currentValidSubscriptionIds = newInfos.map { it.subscriptionId } - subIdRepositoryCache.keys.forEach { - if (!currentValidSubscriptionIds.contains(it)) { - subIdRepositoryCache.remove(it) - } - } + subIdRepositoryCache = + subIdRepositoryCache + .filter { currentValidSubscriptionIds.contains(it.key) } + .toMutableMap() } private suspend fun fetchSubscriptionsList(): List = diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/demo/DemoMobileConnectionsRepositoryTest.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/demo/DemoMobileConnectionsRepositoryTest.kt index 4171cf39a5add..32d0410d589d8 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/demo/DemoMobileConnectionsRepositoryTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/demo/DemoMobileConnectionsRepositoryTest.kt @@ -184,6 +184,25 @@ class DemoMobileConnectionsRepositoryTest : SysuiTestCase() { job.cancel() } + /** Regression test for b/261706421 */ + @Test + fun `multiple connections - remove all - does not throw`() = + testScope.runTest { + var latest: List? = null + val job = underTest.subscriptions.onEach { latest = it }.launchIn(this) + + // Two subscriptions are added + fakeNetworkEventFlow.value = validMobileEvent(subId = 1, level = 1) + fakeNetworkEventFlow.value = validMobileEvent(subId = 2, level = 1) + + // Then both are removed by turning off demo mode + underTest.stopProcessingCommands() + + assertThat(latest).isEmpty() + + job.cancel() + } + @Test fun `demo connection - single subscription`() = testScope.runTest { diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileConnectionsRepositoryTest.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileConnectionsRepositoryTest.kt index 7870807b93bf9..4b82b398360d6 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileConnectionsRepositoryTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileConnectionsRepositoryTest.kt @@ -52,6 +52,7 @@ import org.junit.After import org.junit.Assert.assertThrows import org.junit.Before import org.junit.Test +import org.mockito.ArgumentMatchers.anyInt import org.mockito.Mock import org.mockito.Mockito.verify import org.mockito.MockitoAnnotations @@ -76,6 +77,24 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() { fun setUp() { MockitoAnnotations.initMocks(this) + // Set up so the individual connection repositories + whenever(telephonyManager.createForSubscriptionId(anyInt())).thenAnswer { invocation -> + telephonyManager.also { + whenever(telephonyManager.subscriptionId).thenReturn(invocation.getArgument(0)) + } + } + + val connectionFactory: MobileConnectionRepositoryImpl.Factory = + MobileConnectionRepositoryImpl.Factory( + context = context, + telephonyManager = telephonyManager, + bgDispatcher = IMMEDIATE, + globalSettings = globalSettings, + logger = logger, + mobileMappingsProxy = mobileMappings, + scope = scope, + ) + underTest = MobileConnectionsRepositoryImpl( connectivityManager, @@ -88,7 +107,7 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() { context, IMMEDIATE, scope, - mock(), + connectionFactory, ) } @@ -207,6 +226,32 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() { job.cancel() } + /** Regression test for b/261706421 */ + @Test + fun testConnectionsCache_clearMultipleSubscriptionsAtOnce_doesNotThrow() = + runBlocking(IMMEDIATE) { + val job = underTest.subscriptions.launchIn(this) + + whenever(subscriptionManager.completeActiveSubscriptionInfoList) + .thenReturn(listOf(SUB_1, SUB_2)) + getSubscriptionCallback().onSubscriptionsChanged() + + // Get repos to trigger caching + val repo1 = underTest.getRepoForSubId(SUB_1_ID) + val repo2 = underTest.getRepoForSubId(SUB_2_ID) + + assertThat(underTest.getSubIdRepoCache()) + .containsExactly(SUB_1_ID, repo1, SUB_2_ID, repo2) + + // All subscriptions disappear + whenever(subscriptionManager.completeActiveSubscriptionInfoList).thenReturn(listOf()) + getSubscriptionCallback().onSubscriptionsChanged() + + assertThat(underTest.getSubIdRepoCache()).isEmpty() + + job.cancel() + } + @Test fun testConnectionRepository_invalidSubId_throws() = runBlocking(IMMEDIATE) {