From b397f5f961e18eb13602283f635b055702483dce Mon Sep 17 00:00:00 2001 From: psteiger Date: Sat, 17 Feb 2024 00:23:41 -0500 Subject: [PATCH] Change `ScopeProvider.coroutineScope` throwing behavior when accessing it before scope is active, or after scope is inactive. Instead of throwing at call site, we deliver `CoroutineScopeLifecycleException(message: String, cause: OutsideScopeException)` to the `CoroutineExceptionHandler` installed at `RibCoroutinesConfig.exceptionHandler`. Note `cause` is always non-null, and it's either `LifecycleNotStartedException` or `LifecycleEndedException`. ## Motivation Suppose you have an interactor that starts subscribing to an Rx stream asynchronously. ```kotlin class MyInteractor { override fun didBecomeActive() { runLater { someObservable.autoDispose(this).subscribe() } } } ``` It is possible that this Rx subscription will be triggered after interactor's already inactive. In that case, subscription will be No-Op. This is not good code for a major reason: `Interactor` is attempting to do work after it's inactive. In other words, `runLater` does not respect the interactor lifecycle. Still, considering this is some legacy code being migrated from Rx to Coroutines, the new code would look like the following. ```kotlin class MyInteractor { override fun didBecomeActive() { runLater { someObservable.autoDispose(coroutineScope).subscribe() } } } ``` Unlike the Rx counterpart, this code will sometimes fatally throw. If `ScopeProvider.coroutineScope` is called after the scope's inactive, it will throw `LifecycleEndedException` at call site. Calling `coroutineScope` outside of lifecycle bounds is always erroneous code. But in order to favour a smoother migration to Coroutines, and to avoid surprises of `val` throwing exceptions, we are changing current implementation to deliver the exception to the `CoroutineExceptionHandler` instead of throwing at call site. If some app decides to override the default behavior of crashing (in JVM/Android) in face of this erroneous code, one can customize `RibCoroutinesConfig.exceptionHandler` at app startup: ```kotlin RibCoroutinesConfig.exceptionHandler = CoroutineExceptionHandler { _, throwable -> when (throwable) { is CoroutineScopeLifecycleException -> log(throwable) // only log, don't re-throw. else -> throw throwable } } ``` --- android/gradle/libs.versions.toml | 4 +- android/libraries/rib-coroutines/build.gradle | 2 + .../com/uber/rib/core/RibCoroutineScopes.kt | 63 ++++++--- .../rib/core/internal/DirectDispatcher.kt | 39 ++++++ .../uber/rib/core/RibCoroutineScopesTest.kt | 124 ++++++++++++++++++ 5 files changed, 213 insertions(+), 19 deletions(-) create mode 100644 android/libraries/rib-coroutines/src/main/kotlin/com/uber/rib/core/internal/DirectDispatcher.kt create mode 100644 android/libraries/rib-coroutines/src/test/kotlin/com/uber/rib/core/RibCoroutineScopesTest.kt diff --git a/android/gradle/libs.versions.toml b/android/gradle/libs.versions.toml index 3004d9c4..11b50887 100644 --- a/android/gradle/libs.versions.toml +++ b/android/gradle/libs.versions.toml @@ -32,8 +32,8 @@ intellij = "2023.2" javapoet = "1.11.1" jsr250 = "1.0" junit = "4.12" -kotlin = "1.8.20" -kotlinx-coroutines = "1.7.3" +kotlin = "1.9.23" +kotlinx-coroutines = "1.8.0" ktfmt = "0.43" ktlint = "0.48.2" leakcanary = "1.5.4" diff --git a/android/libraries/rib-coroutines/build.gradle b/android/libraries/rib-coroutines/build.gradle index a1c45717..cf952e45 100644 --- a/android/libraries/rib-coroutines/build.gradle +++ b/android/libraries/rib-coroutines/build.gradle @@ -24,11 +24,13 @@ dependencies { api(libs.autodispose.coroutines) api(libs.coroutines.android) api(libs.coroutines.rx2) + implementation(libs.autodispose.lifecycle) compileOnly(libs.android.api) testImplementation(project(":libraries:rib-base")) testImplementation(project(":libraries:rib-test")) + testImplementation(project(":libraries:rib-coroutines-test")) testImplementation(testLibs.junit) testImplementation(testLibs.mockito) testImplementation(testLibs.mockitoKotlin) diff --git a/android/libraries/rib-coroutines/src/main/kotlin/com/uber/rib/core/RibCoroutineScopes.kt b/android/libraries/rib-coroutines/src/main/kotlin/com/uber/rib/core/RibCoroutineScopes.kt index 6d67979d..cb254c7e 100755 --- a/android/libraries/rib-coroutines/src/main/kotlin/com/uber/rib/core/RibCoroutineScopes.kt +++ b/android/libraries/rib-coroutines/src/main/kotlin/com/uber/rib/core/RibCoroutineScopes.kt @@ -16,17 +16,23 @@ package com.uber.rib.core import android.app.Application +import com.uber.autodispose.OutsideScopeException import com.uber.autodispose.ScopeProvider -import com.uber.autodispose.coroutinesinterop.asCoroutineScope import com.uber.rib.core.internal.CoroutinesFriendModuleApi +import com.uber.rib.core.internal.DirectDispatcher import java.util.WeakHashMap import kotlin.coroutines.CoroutineContext import kotlin.coroutines.EmptyCoroutineContext import kotlin.reflect.KProperty import kotlinx.coroutines.CoroutineName import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.CoroutineStart +import kotlinx.coroutines.Job import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.cancel import kotlinx.coroutines.job +import kotlinx.coroutines.launch +import kotlinx.coroutines.rx2.await /** * [CoroutineScope] tied to this [ScopeProvider]. This scope will be canceled when ScopeProvider is @@ -37,15 +43,7 @@ import kotlinx.coroutines.job */ @OptIn(CoroutinesFriendModuleApi::class) public val ScopeProvider.coroutineScope: CoroutineScope by - LazyCoroutineScope { - val context: CoroutineContext = - SupervisorJob() + - RibDispatchers.Main.immediate + - CoroutineName("${this::class.simpleName}:coroutineScope") + - (RibCoroutinesConfig.exceptionHandler ?: EmptyCoroutineContext) - - asCoroutineScope(context) - } + LazyCoroutineScope { ScopeProviderCoroutineScope(this, createCoroutineContext()) } /** * [CoroutineScope] tied to this [Application]. This scope will not be cancelled, it lives for the @@ -56,15 +54,40 @@ public val ScopeProvider.coroutineScope: CoroutineScope by */ @OptIn(CoroutinesFriendModuleApi::class) public val Application.coroutineScope: CoroutineScope by - LazyCoroutineScope { - val context: CoroutineContext = - SupervisorJob() + - RibDispatchers.Main.immediate + - CoroutineName("${this::class.simpleName}:coroutineScope") + - (RibCoroutinesConfig.exceptionHandler ?: EmptyCoroutineContext) + LazyCoroutineScope { CoroutineScope(createCoroutineContext()) } + +private fun Any.createCoroutineContext() = + SupervisorJob() + + RibDispatchers.Main.immediate + + CoroutineName("${this::class.simpleName}:coroutineScope") + + (RibCoroutinesConfig.exceptionHandler ?: EmptyCoroutineContext) - CoroutineScope(context) +private class ScopeProviderCoroutineScope( + scopeProvider: ScopeProvider, + override val coroutineContext: CoroutineContext, +) : ScopeProvider by scopeProvider, CoroutineScope { + init { + requireNotNull(coroutineContext[Job]) { "coroutineContext must have a job in it" } + cancelWhenLifecycleEnded() } +} + +@OptIn(CoroutinesFriendModuleApi::class) +private fun ScopeProviderCoroutineScope.cancelWhenLifecycleEnded() { + launch(DirectDispatcher, CoroutineStart.UNDISPATCHED) { awaitCompletion() } + .invokeOnCompletion { e -> cancel("Lifecycle is not active", e) } +} + +private suspend inline fun ScopeProvider.awaitCompletion() { + try { + requestScope().await() + } catch (e: OutsideScopeException) { + throw CoroutineScopeLifecycleException( + message = "Attempted to obtain ScopeProvider.coroutineScope out of lifecycle bounds", + cause = e, + ) + } +} @CoroutinesFriendModuleApi public class LazyCoroutineScope(private val initializer: This.() -> CoroutineScope) { @@ -88,3 +111,9 @@ public class LazyCoroutineScope(private val initializer: This.() -> } } } + +public class CoroutineScopeLifecycleException +internal constructor( + message: String, + cause: OutsideScopeException, +) : IllegalStateException(message, cause) diff --git a/android/libraries/rib-coroutines/src/main/kotlin/com/uber/rib/core/internal/DirectDispatcher.kt b/android/libraries/rib-coroutines/src/main/kotlin/com/uber/rib/core/internal/DirectDispatcher.kt new file mode 100644 index 00000000..be065a75 --- /dev/null +++ b/android/libraries/rib-coroutines/src/main/kotlin/com/uber/rib/core/internal/DirectDispatcher.kt @@ -0,0 +1,39 @@ +/* + * Copyright (C) 2024. Uber Technologies + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.uber.rib.core.internal + +import kotlin.coroutines.CoroutineContext +import kotlinx.coroutines.CoroutineDispatcher +import kotlinx.coroutines.Runnable + +/** + * A coroutine dispatcher that is not confined to any specific thread. It executes the initial + * continuation of a coroutine in the current call-frame and lets the coroutine resume in whatever + * thread that is used by the corresponding suspending function, without mandating any specific + * threading policy. + * + * This dispatcher is similar to [Unconfined][com.uber.rib.core.RibDispatchers.Unconfined], with the + * difference that it does not form an event-loop on nested coroutines, which implies that it has + * predictable ordering of events with the tradeoff of an increased risk of [StackOverflowError]. + * + * **This is internal API, not supposed to be used by library consumers.** + */ +@CoroutinesFriendModuleApi +public object DirectDispatcher : CoroutineDispatcher() { + override fun dispatch(context: CoroutineContext, block: Runnable) { + block.run() + } +} diff --git a/android/libraries/rib-coroutines/src/test/kotlin/com/uber/rib/core/RibCoroutineScopesTest.kt b/android/libraries/rib-coroutines/src/test/kotlin/com/uber/rib/core/RibCoroutineScopesTest.kt new file mode 100644 index 00000000..3f906e47 --- /dev/null +++ b/android/libraries/rib-coroutines/src/test/kotlin/com/uber/rib/core/RibCoroutineScopesTest.kt @@ -0,0 +1,124 @@ +/* + * Copyright (C) 2024. Uber Technologies + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.uber.rib.core + +import com.google.common.truth.Truth.assertThat +import com.uber.autodispose.lifecycle.LifecycleEndedException +import com.uber.autodispose.lifecycle.LifecycleNotStartedException +import kotlin.contracts.ExperimentalContracts +import kotlin.contracts.InvocationKind +import kotlin.contracts.contract +import kotlinx.coroutines.CoroutineExceptionHandler +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.Job +import kotlinx.coroutines.awaitCancellation +import kotlinx.coroutines.isActive +import kotlinx.coroutines.launch +import kotlinx.coroutines.test.runCurrent +import kotlinx.coroutines.test.runTest +import org.hamcrest.CoreMatchers.instanceOf +import org.junit.Before +import org.junit.Rule +import org.junit.Test +import org.junit.rules.ExpectedException +import org.junit.runner.RunWith +import org.junit.runners.Parameterized +import org.mockito.kotlin.mock + +@OptIn(ExperimentalCoroutinesApi::class) +@RunWith(Parameterized::class) +class RibCoroutineScopesTest( + private val throwWhenBeforeActive: Boolean, + private val throwWhenAfterInactive: Boolean, +) { + @get:Rule val ribCoroutinesRule = RibCoroutinesRule() + + @get:Rule val exceptionRule: ExpectedException = ExpectedException.none() + + private val interactor = TestInteractor() + + @Before + fun setUp() { + RibCoroutinesConfig.exceptionHandler = CoroutineExceptionHandler { _, throwable -> + when (throwable) { + is CoroutineScopeLifecycleException -> if (shouldThrow(throwable)) throw throwable + else -> throw throwable + } + } + } + + @Test + fun coroutineScope_whenCalledBeforeActive_throwsCoroutineScopeLifecycleException() = runTest { + if (throwWhenBeforeActive) { + exceptionRule.expect(CoroutineScopeLifecycleException::class.java) + exceptionRule.expectCause(instanceOf(LifecycleNotStartedException::class.java)) + } + assertThat(interactor.coroutineScope.isActive).isFalse() + } + + @Test + fun coroutineScope_whenCalledAfterInactive_throwsCoroutineScopeLifecycleException() = runTest { + if (throwWhenAfterInactive) { + exceptionRule.expect(CoroutineScopeLifecycleException::class.java) + exceptionRule.expectCause(instanceOf(LifecycleEndedException::class.java)) + } + interactor.attachAndDetach {} + assertThat(interactor.coroutineScope.isActive).isFalse() + } + + @Test + fun coroutineScope_whenCalledWhileActive_cancelsWhenInactive() = runTest { + var launched = false + val job: Job + interactor.attachAndDetach { + job = + coroutineScope.launch { + launched = true + awaitCancellation() + } + runCurrent() + assertThat(launched).isTrue() + assertThat(job.isActive).isTrue() + } + assertThat(job.isCancelled).isTrue() + } + + private fun shouldThrow(e: CoroutineScopeLifecycleException): Boolean = + (throwWhenBeforeActive && e.cause is LifecycleNotStartedException) || + (throwWhenAfterInactive && e.cause is LifecycleEndedException) + + companion object { + @JvmStatic + @Parameterized.Parameters(name = "throwWhenBeforeActive = {0}, throwWhenAfterInactive = {1}") + fun data() = + listOf( + arrayOf(true, true), + arrayOf(true, false), + arrayOf(false, true), + arrayOf(false, false), + ) + } +} + +private class TestInteractor : Interactor>() + +@OptIn(ExperimentalContracts::class) +private inline fun TestInteractor.attachAndDetach(block: TestInteractor.() -> Unit) { + contract { callsInPlace(block, InvocationKind.EXACTLY_ONCE) } + InteractorHelper.attach(this, Unit, mock(), null) + block() + InteractorHelper.detach(this) +}