From 84fc7eefa4b114a4d4fa26f50323e0e801bea97f Mon Sep 17 00:00:00 2001 From: Jason LeBrun Date: Wed, 2 Dec 2020 12:58:37 -0800 Subject: [PATCH] Remove the need to launch a new coroutine when resurrecting Arcs PiperOrigin-RevId: 345294507 --- java/arcs/core/host/AbstractArcHost.kt | 14 ++-- .../labs/host/AndroidResurrectableHost.kt | 7 +- .../sdk/android/labs/host/ArcHostHelper.kt | 37 +++++++-- java/arcs/sdk/android/labs/host/BUILD | 2 + .../android/labs/host/ResurrectableHost.kt | 2 +- .../sdk/android/storage/ResurrectionHelper.kt | 20 +---- .../arcs/android/host/ArcHostHelperTest.kt | 81 ++++++++++++++++++- javatests/arcs/android/host/BUILD | 3 + .../host/TestExternalArcHostService.kt | 2 +- .../android/storage/ResurrectionHelperTest.kt | 46 +---------- .../storage/service/StorageServiceTest.kt | 22 ++--- 11 files changed, 135 insertions(+), 101 deletions(-) diff --git a/java/arcs/core/host/AbstractArcHost.kt b/java/arcs/core/host/AbstractArcHost.kt index aef6a77c90d..16ec9efb419 100644 --- a/java/arcs/core/host/AbstractArcHost.kt +++ b/java/arcs/core/host/AbstractArcHost.kt @@ -582,15 +582,13 @@ abstract class AbstractArcHost( /** Helper used by implementors of [ResurrectableHost]. */ @Suppress("UNUSED_PARAMETER") - fun onResurrected(arcId: String, affectedKeys: List) { - auxillaryScope.launch { - if (isRunning(arcId)) { - return@launch - } - val context = lookupOrCreateArcHostContext(arcId) - val partition = contextToPartition(arcId, context) - startArc(partition) + suspend fun onResurrected(arcId: String, affectedKeys: List) { + if (isRunning(arcId)) { + return } + val context = lookupOrCreateArcHostContext(arcId) + val partition = contextToPartition(arcId, context) + startArc(partition) } private fun contextToPartition(arcId: String, context: ArcHostContext) = diff --git a/java/arcs/sdk/android/labs/host/AndroidResurrectableHost.kt b/java/arcs/sdk/android/labs/host/AndroidResurrectableHost.kt index af3c5aaf7ef..acfc63a6984 100644 --- a/java/arcs/sdk/android/labs/host/AndroidResurrectableHost.kt +++ b/java/arcs/sdk/android/labs/host/AndroidResurrectableHost.kt @@ -10,6 +10,7 @@ */ package arcs.sdk.android.labs.host +/* ktlint-disable import-ordering */ import android.content.Context import androidx.lifecycle.Lifecycle import arcs.core.host.ArcHost @@ -43,11 +44,11 @@ abstract class AndroidResurrectableHost( schedulerProvider = schedulerProvider, storageEndpointManager = storageEndpointManager, particles = *particles -), ResurrectableHost { +), + ResurrectableHost { override val resurrectionHelper: ResurrectionHelper = ResurrectionHelper( - context, - ::onResurrected + context ) override fun maybeRequestResurrection(context: ArcHostContext) { diff --git a/java/arcs/sdk/android/labs/host/ArcHostHelper.kt b/java/arcs/sdk/android/labs/host/ArcHostHelper.kt index 1ab55c5f6a5..97d57a9080f 100644 --- a/java/arcs/sdk/android/labs/host/ArcHostHelper.kt +++ b/java/arcs/sdk/android/labs/host/ArcHostHelper.kt @@ -20,6 +20,7 @@ import android.os.Bundle import android.os.Parcelable import android.os.ResultReceiver import androidx.annotation.VisibleForTesting +import arcs.android.common.resurrection.ResurrectionRequest import arcs.android.host.parcelables.ActualParcelable import arcs.android.host.parcelables.ParcelableParticleIdentifier import arcs.android.host.parcelables.ParcelablePlanPartition @@ -32,6 +33,8 @@ import arcs.core.host.ArcHostException import arcs.core.host.ArcState import arcs.core.host.ArcStateChangeRegistration import arcs.core.host.ParticleIdentifier +import arcs.core.storage.StorageKeyParser +import arcs.core.util.TaggedLog import kotlin.reflect.KClass import kotlinx.coroutines.CoroutineName import kotlinx.coroutines.CoroutineScope @@ -71,7 +74,7 @@ class ArcHostHelper( ) { private val job = SupervisorJob() + Dispatchers.Unconfined + CoroutineName("ArcHostHelper") private val arcHostByHostId = mutableMapOf() - + private val log = TaggedLog { "ArcHostHelper" } init { arcHosts.forEach { arcHostByHostId[it.hostId] = it } } @@ -89,16 +92,38 @@ class ArcHostHelper( onStartCommandSuspendable(intent) } + private suspend fun resurrectArc(intent: Intent, arcHost: ResurrectableHost) { + val targetId = intent.getStringExtra(ResurrectionRequest.EXTRA_REGISTRATION_TARGET_ID) + if (targetId == null) { + log.warning { "Received resurrection intent with null target ID" } + return + } + + val notifiers = intent.getStringArrayListExtra(ResurrectionRequest.EXTRA_RESURRECT_NOTIFIER) + if (notifiers == null) { + log.warning { "Received resurrection intent with null notifiers" } + return + } + + arcHost.onResurrected( + targetId, + notifiers.map(StorageKeyParser.Companion::parse) ?: emptyList() + ) + } + @VisibleForTesting suspend fun onStartCommandSuspendable(intent: Intent?) { - arcHostByHostId.values.forEach { - if (it is ResurrectableHost) { - it.resurrectionHelper.onStartCommand(intent) - } + val action = intent?.action ?: return + + if (action.startsWith(ResurrectionRequest.ACTION_RESURRECT)) { + arcHostByHostId.values + .filterIsInstance() + .forEach { + resurrectArc(intent, it) + } } // Ignore other actions - val action = intent?.action ?: return if (!action.startsWith(ArcHostHelper.ACTION_HOST_INTENT)) return // Ignore Intent when it doesn't target our Service diff --git a/java/arcs/sdk/android/labs/host/BUILD b/java/arcs/sdk/android/labs/host/BUILD index ff58aafe1d4..73b1e0abe02 100644 --- a/java/arcs/sdk/android/labs/host/BUILD +++ b/java/arcs/sdk/android/labs/host/BUILD @@ -9,12 +9,14 @@ arcs_kt_android_library( srcs = glob(["*.kt"]), manifest = "//java/arcs/android/common:AndroidManifest.xml", deps = [ + "//java/arcs/android/common/resurrection", "//java/arcs/android/host/parcelables", "//java/arcs/core/common", "//java/arcs/core/data", "//java/arcs/core/host", "//java/arcs/core/storage", "//java/arcs/core/storage:storage_key", + "//java/arcs/core/util", "//java/arcs/jvm/util", "//java/arcs/sdk/android/storage", "//third_party/java/androidx/annotation", diff --git a/java/arcs/sdk/android/labs/host/ResurrectableHost.kt b/java/arcs/sdk/android/labs/host/ResurrectableHost.kt index a5fafb5deff..9cb8bbba1bb 100644 --- a/java/arcs/sdk/android/labs/host/ResurrectableHost.kt +++ b/java/arcs/sdk/android/labs/host/ResurrectableHost.kt @@ -23,5 +23,5 @@ interface ResurrectableHost : ArcHost { val resurrectionHelper: ResurrectionHelper /** Invoked by [ArcHostHelper.onStartCommand] when [ResurrectorService] needs to wake an arc. */ - fun onResurrected(arcId: String, affectedKeys: List) + suspend fun onResurrected(arcId: String, affectedKeys: List) } diff --git a/java/arcs/sdk/android/storage/ResurrectionHelper.kt b/java/arcs/sdk/android/storage/ResurrectionHelper.kt index 8483e6b68f1..0c5863af66d 100644 --- a/java/arcs/sdk/android/storage/ResurrectionHelper.kt +++ b/java/arcs/sdk/android/storage/ResurrectionHelper.kt @@ -16,7 +16,6 @@ import android.content.Context import android.content.Intent import arcs.android.common.resurrection.ResurrectionRequest import arcs.core.storage.StorageKey -import arcs.core.storage.StorageKeyParser import arcs.sdk.android.storage.service.StorageService /** @@ -55,25 +54,8 @@ import arcs.sdk.android.storage.service.StorageService * ``` */ class ResurrectionHelper( - private val context: Context, - private val onResurrected: (targetId: String, List) -> Unit + private val context: Context ) { - /** - * Determines whether or not the given [intent] represents a resurrection, and if it does: - * calls [onResurrected]. - */ - fun onStartCommand(intent: Intent?) { - if (intent?.action?.startsWith(ResurrectionRequest.ACTION_RESURRECT) != true) return - - val targetId = - intent.getStringExtra(ResurrectionRequest.EXTRA_REGISTRATION_TARGET_ID) ?: return - val notifiers = intent.getStringArrayListExtra( - ResurrectionRequest.EXTRA_RESURRECT_NOTIFIER - ) ?: return - - onResurrected(targetId, notifiers.map(StorageKeyParser.Companion::parse)) - } - /** * Issue a request to be resurrected by the [StorageService] whenever the data identified by * the provided [keys] changes. diff --git a/javatests/arcs/android/host/ArcHostHelperTest.kt b/javatests/arcs/android/host/ArcHostHelperTest.kt index 6352f6aba5e..7d7181a49a1 100644 --- a/javatests/arcs/android/host/ArcHostHelperTest.kt +++ b/javatests/arcs/android/host/ArcHostHelperTest.kt @@ -18,6 +18,7 @@ import android.os.Handler import android.os.ResultReceiver import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.test.platform.app.InstrumentationRegistry +import arcs.android.common.resurrection.ResurrectionRequest import arcs.core.common.ArcId import arcs.core.data.EntityType import arcs.core.data.FieldType.Companion.Text @@ -32,9 +33,13 @@ import arcs.core.host.ArcState import arcs.core.host.ArcStateChangeCallback import arcs.core.host.ArcStateChangeRegistration import arcs.core.host.ParticleIdentifier +import arcs.core.storage.DefaultStorageKeyManager +import arcs.core.storage.StorageKey import arcs.core.storage.keys.VolatileStorageKey +import arcs.core.storage.testutil.DummyStorageKey import arcs.core.util.guardedBy import arcs.sdk.android.labs.host.ArcHostHelper +import arcs.sdk.android.labs.host.ResurrectableHost import arcs.sdk.android.labs.host.createGetRegisteredParticlesIntent import arcs.sdk.android.labs.host.createLookupArcStatusIntent import arcs.sdk.android.labs.host.createPauseArcHostIntent @@ -42,6 +47,7 @@ import arcs.sdk.android.labs.host.createStartArcHostIntent import arcs.sdk.android.labs.host.createStopArcHostIntent import arcs.sdk.android.labs.host.createUnpauseArcHostIntent import arcs.sdk.android.labs.host.toComponentName +import arcs.sdk.android.storage.ResurrectionHelper import com.google.common.truth.Truth.assertThat import kotlin.coroutines.resume import kotlin.coroutines.suspendCoroutine @@ -85,7 +91,7 @@ class ArcHostHelperTest { val planPartition = Plan.Partition("id", "FooHost", listOf(particleSpec)) - open class TestArcHost : ArcHost { + open class TestArcHost(context: Context) : ArcHost, ResurrectableHost { private val hostMutex = Mutex() var startArcCalls: MutableList by guardedBy(hostMutex, mutableListOf()) @@ -93,8 +99,13 @@ class ArcHostHelperTest { var registeredParticles: MutableList by guardedBy( hostMutex, mutableListOf() ) + var lastResurrectedArcId: String? = null + var lastResurrectedKeys: List = emptyList() + var paused = false + override val resurrectionHelper = ResurrectionHelper(context) + suspend fun startCalls() = hostMutex.withLock { startArcCalls } suspend fun stopCalls() = hostMutex.withLock { stopArcCalls } suspend fun particles() = hostMutex.withLock { registeredParticles } @@ -112,6 +123,11 @@ class ArcHostHelperTest { } } + override suspend fun onResurrected(arcId: String, affectedKeys: List) { + lastResurrectedArcId = arcId + lastResurrectedKeys = affectedKeys + } + override suspend fun pause() { paused = true } @@ -153,8 +169,9 @@ class ArcHostHelperTest { TestArcHost.status = ArcState.Stopped context = InstrumentationRegistry.getInstrumentation().targetContext service = Robolectric.setupService(TestAndroidArcHostService::class.java) - arcHost = TestArcHost() + arcHost = TestArcHost(context) helper = ArcHostHelper(service, arcHost) + DefaultStorageKeyManager.addParser(DummyStorageKey) } @Test @@ -271,6 +288,66 @@ class ArcHostHelperTest { assertThat(arcHost.paused).isFalse() } + @Test + fun onStartCommand_withNullIntent_doesNotResurrect() = runBlockingTest { + helper.onStartCommandSuspendable(null) + assertThat(arcHost.lastResurrectedArcId).isNull() + assertThat(arcHost.lastResurrectedKeys).isEmpty() + } + + @Test + fun onStartCommand_withNullIntentAction_doesNotResurrect() = runBlockingTest { + val intent = Intent() + helper.onStartCommandSuspendable(intent) + assertThat(arcHost.lastResurrectedArcId).isNull() + assertThat(arcHost.lastResurrectedKeys).isEmpty() + } + + @Test + fun onStartCommand_withNoResurrectionTarget_doesNotResurrect() = runBlockingTest { + val dummyStorageKey1 = DummyStorageKey("foo") + val dummyStorageKey2 = DummyStorageKey("bar") + val notifiers: ArrayList = ArrayList( + listOf( + dummyStorageKey1.toString(), + dummyStorageKey2.toString() + ) + ) + val intent = Intent(ResurrectionRequest.ACTION_RESURRECT).apply { + putStringArrayListExtra(ResurrectionRequest.EXTRA_RESURRECT_NOTIFIER, notifiers) + } + helper.onStartCommandSuspendable(intent) + assertThat(arcHost.lastResurrectedArcId).isNull() + assertThat(arcHost.lastResurrectedKeys).isEmpty() + } + + @Test + fun onStartCommand_withNoNotifiers_doesNotResurrect() = runBlockingTest { + val resurrectingArcId = "arcId" + val intent = Intent(ResurrectionRequest.ACTION_RESURRECT).apply { + putExtra(ResurrectionRequest.EXTRA_REGISTRATION_TARGET_ID, resurrectingArcId) + } + helper.onStartCommandSuspendable(intent) + assertThat(arcHost.lastResurrectedArcId).isNull() + assertThat(arcHost.lastResurrectedKeys).isEmpty() + } + + @Test + fun onStartCommand_withResurrectionTargetAndNotifiers_callsOnResurrect() = runBlockingTest { + val resurrectingArcId = "arcId" + val dummyStorageKey1 = DummyStorageKey("foo") + val dummyStorageKey2 = DummyStorageKey("bar") + val notifiers = listOf(dummyStorageKey1, dummyStorageKey2) + val notifiersExtra = ArrayList(notifiers.map { it.toString() }) + val intent = Intent(ResurrectionRequest.ACTION_RESURRECT).apply { + putStringArrayListExtra(ResurrectionRequest.EXTRA_RESURRECT_NOTIFIER, notifiersExtra) + putExtra(ResurrectionRequest.EXTRA_REGISTRATION_TARGET_ID, resurrectingArcId) + } + helper.onStartCommandSuspendable(intent) + assertThat(arcHost.lastResurrectedArcId).isEqualTo(resurrectingArcId) + assertThat(arcHost.lastResurrectedKeys).containsExactlyElementsIn(notifiers) + } + private fun runWithResult( intent: Intent, transformer: (Bundle?) -> T diff --git a/javatests/arcs/android/host/BUILD b/javatests/arcs/android/host/BUILD index 8ca636da4e8..8fa066d7755 100644 --- a/javatests/arcs/android/host/BUILD +++ b/javatests/arcs/android/host/BUILD @@ -18,6 +18,7 @@ arcs_kt_android_test_suite( deps = [ ":schemas", ":services", + "//java/arcs/android/common/resurrection", "//java/arcs/android/labs/host", "//java/arcs/android/labs/host/prod", "//java/arcs/android/storage/database", @@ -27,10 +28,12 @@ arcs_kt_android_test_suite( "//java/arcs/core/data:schema_fields", "//java/arcs/core/entity", "//java/arcs/core/host", + "//java/arcs/core/storage:storage_key", "//java/arcs/core/storage/api", "//java/arcs/core/storage/driver:ramdisk", "//java/arcs/core/storage/keys", "//java/arcs/core/storage/referencemode", + "//java/arcs/core/storage/testutil", "//java/arcs/core/testutil", "//java/arcs/core/testutil/handles", "//java/arcs/core/util", diff --git a/javatests/arcs/android/host/TestExternalArcHostService.kt b/javatests/arcs/android/host/TestExternalArcHostService.kt index c8f39c55c0c..7da0408f969 100644 --- a/javatests/arcs/android/host/TestExternalArcHostService.kt +++ b/javatests/arcs/android/host/TestExternalArcHostService.kt @@ -63,7 +63,7 @@ abstract class TestExternalArcHostService : Service() { ), ResurrectableHost { override val resurrectionHelper: ResurrectionHelper = - ResurrectionHelper(context, ::onResurrected) + ResurrectionHelper(context) override val arcHostContextCapability = testingCapability } diff --git a/javatests/arcs/sdk/android/storage/ResurrectionHelperTest.kt b/javatests/arcs/sdk/android/storage/ResurrectionHelperTest.kt index 79902e75a45..9ed6be384b8 100644 --- a/javatests/arcs/sdk/android/storage/ResurrectionHelperTest.kt +++ b/javatests/arcs/sdk/android/storage/ResurrectionHelperTest.kt @@ -47,51 +47,7 @@ class ResurrectionHelperTest { callbackCalls.clear() - helper = ResurrectionHelper(context) { _, keys -> callbackCalls.add(keys) } - } - - @Test - fun onStartCommand_doesNotCallOnResurrected_when_intentIsNull() { - helper.onStartCommand(null) - assertThat(callbackCalls).isEmpty() - } - - @Test - fun onStartCommand_doesNotCallOnResurrected_when_intentActionIsNull() { - helper.onStartCommand(Intent()) - assertThat(callbackCalls).isEmpty() - } - - @Test - fun onStartCommand_doesNotCallOnResurrected_when_intentActionDoesNotMatch() { - helper.onStartCommand(Intent().apply { action = "Incorrect" }) - assertThat(callbackCalls).isEmpty() - } - - @Test - fun onStartCommand_doesNotCallOnResurrected_when_notifierExtra_isMissing() { - helper.onStartCommand(Intent().apply { action = ResurrectionRequest.ACTION_RESURRECT }) - assertThat(callbackCalls).isEmpty() - } - - @Test - fun onStartCommand_callsOnResurrected_whenStarsAlign() { - val storageKeys = listOf( - RamDiskStorageKey("foo"), - RamDiskStorageKey("bar") - ) - - val intent = Intent().apply { - action = ResurrectionRequest.ACTION_RESURRECT - putExtra( - ResurrectionRequest.EXTRA_RESURRECT_NOTIFIER, - ArrayList(storageKeys.map(StorageKey::toString)) - ) - putExtra(ResurrectionRequest.EXTRA_REGISTRATION_TARGET_ID, "test") - } - - helper.onStartCommand(intent) - assertThat(callbackCalls).containsExactly(storageKeys) + helper = ResurrectionHelper(context) } @Test diff --git a/javatests/arcs/sdk/android/storage/service/StorageServiceTest.kt b/javatests/arcs/sdk/android/storage/service/StorageServiceTest.kt index 2a1d6202083..da892d8d13b 100644 --- a/javatests/arcs/sdk/android/storage/service/StorageServiceTest.kt +++ b/javatests/arcs/sdk/android/storage/service/StorageServiceTest.kt @@ -28,11 +28,9 @@ import arcs.android.storage.ttl.PeriodicCleanupTask import arcs.core.crdt.CrdtCount import arcs.core.data.CountType import arcs.core.storage.ProxyMessage -import arcs.core.storage.StorageKey import arcs.core.storage.StoreOptions import arcs.core.storage.api.DriverAndKeyConfigurator import arcs.core.storage.keys.RamDiskStorageKey -import arcs.sdk.android.storage.ResurrectionHelper import com.google.common.truth.Truth.assertThat import kotlinx.coroutines.delay import kotlinx.coroutines.runBlocking @@ -66,16 +64,6 @@ class StorageServiceTest { @Test fun sendingProxyMessage_resultsInResurrection() = lifecycle(storeOptions) { service, context -> - // Setup: - // Create a resurrection helper we'll use to collect updated storage keys coming from the - // ShadowApplication-captured nextStartedService intents. - val receivedUpdates = mutableListOf>() - val receivedIds = mutableListOf() - val resurrectionHelper = ResurrectionHelper(app) { id: String, keys: List -> - receivedUpdates.add(keys) - receivedIds.add(id) - } - // Setup: // Add a resurrection request to the storage service. val resurrectionRequestIntent = Intent(app, StorageService::class.java).apply { @@ -115,10 +103,12 @@ class StorageServiceTest { // the helper's callback will be triggered, adding to `receivedUpdates`. val shadowApp = shadowOf(app) - resurrectionHelper.onStartCommand(shadowApp.nextStartedService) - assertThat(receivedUpdates).hasSize(1) - assertThat(receivedUpdates[0]).containsExactly(storeOptions.storageKey) - assertThat(receivedIds[0]).isEqualTo("test") + val next = shadowApp.nextStartedService + val id = next.getStringExtra(ResurrectionRequest.EXTRA_REGISTRATION_TARGET_ID) + val ids = next.getStringArrayListExtra(ResurrectionRequest.EXTRA_RESURRECT_NOTIFIER) + assertThat(id).isEqualTo("test") + assertThat(ids).containsExactly(storeOptions.storageKey.toString()) + assertThat(shadowApp.nextStartedService).isNull() } @Test