From 0bdd7cef140258f16f9e63f6f404e7dd56214f1b Mon Sep 17 00:00:00 2001 From: Patrick Steiger Date: Sun, 3 Dec 2023 00:12:33 -0300 Subject: [PATCH] Support classes that are both `Worker` and `RibCoroutineWorker` in converters. `Worker.asRibCoroutineWorker` and `RibCoroutineWorker.asWorker` converters now return the instance unchanged if the class already implements the target interface. This is important for correctness. For example: ``` class DualWorker : Worker, RibCoroutineWorker { fun onStart(lifecycle: WorkerScopeProvider) { doX() } suspend fun onStart(scope: CoroutineScope) { doY() } } ``` Binding this `worker.asWorker()` should call `doX()`, not `doY()`, and binding this `worker.asRibCoroutineWorker()` should call `doY()`, not `doX()`. --- .../com/uber/rib/core/RibCoroutineWorker.kt | 27 +++++--- .../uber/rib/core/RibCoroutineWorkerTest.kt | 61 +++++++++++++++++++ 2 files changed, 80 insertions(+), 8 deletions(-) diff --git a/android/libraries/rib-base/src/main/kotlin/com/uber/rib/core/RibCoroutineWorker.kt b/android/libraries/rib-base/src/main/kotlin/com/uber/rib/core/RibCoroutineWorker.kt index 4c1b192b..55ed2c9c 100644 --- a/android/libraries/rib-base/src/main/kotlin/com/uber/rib/core/RibCoroutineWorker.kt +++ b/android/libraries/rib-base/src/main/kotlin/com/uber/rib/core/RibCoroutineWorker.kt @@ -195,6 +195,8 @@ private suspend fun bindAndAwaitCancellation(worker: RibCoroutineWorker, bindJob try { supervisorScope { worker.onStart(this) + // In case no cancellation check was done at all in `onStart` (e.g. it did not suspend), + // we want to cancel it before completing. ensureActive() bindJob.complete() awaitCancellation() // Never returns normally, so we are sure an exception will be caught. @@ -218,18 +220,28 @@ private fun CompletableJob.cancelOrCompleteExceptionally(throwable: Throwable) { // ---- RibCoroutineWorker <-> Worker adapters ---- // -/** Converts a [Worker] to a [RibCoroutineWorker]. */ +/** + * Converts a [Worker] to a [RibCoroutineWorker]. + * + * Returns the instance unchanged if it already implements [RibCoroutineWorker]. + */ public fun Worker.asRibCoroutineWorker(): RibCoroutineWorker = - WorkerToRibCoroutineWorkerAdapter(this) + this as? RibCoroutineWorker ?: WorkerToRibCoroutineWorkerAdapter(this) -/** Converts a [RibCoroutineWorker] to a [Worker]. */ +/** + * Converts a [RibCoroutineWorker] to a [Worker]. + * + * Returns the instance unchanged if it already implements [Worker]. In that case, + * [coroutineContext] will not be used. + */ @JvmOverloads public fun RibCoroutineWorker.asWorker( coroutineContext: CoroutineContext = RibDispatchers.Default, -): Worker = RibCoroutineWorkerToWorkerAdapter(this, coroutineContext) +): Worker = this as? Worker ?: RibCoroutineWorkerToWorkerAdapter(this, coroutineContext) -internal open class WorkerToRibCoroutineWorkerAdapter(private val worker: Worker) : - RibCoroutineWorker { +internal open class WorkerToRibCoroutineWorkerAdapter( + private val worker: Worker, +) : RibCoroutineWorker { override suspend fun onStart(scope: CoroutineScope) { withContext(worker.coroutineContext ?: EmptyCoroutineContext) { worker.onStart(scope.asWorkerScopeProvider()) @@ -239,8 +251,7 @@ internal open class WorkerToRibCoroutineWorkerAdapter(private val worker: Worker override fun onStop(cause: Throwable): Unit = worker.onStop() } -internal open class RibCoroutineWorkerToWorkerAdapter -internal constructor( +internal open class RibCoroutineWorkerToWorkerAdapter( private val ribCoroutineWorker: RibCoroutineWorker, override val coroutineContext: CoroutineContext, ) : Worker { diff --git a/android/libraries/rib-base/src/test/kotlin/com/uber/rib/core/RibCoroutineWorkerTest.kt b/android/libraries/rib-base/src/test/kotlin/com/uber/rib/core/RibCoroutineWorkerTest.kt index d63ca5b7..5aa4f0ef 100644 --- a/android/libraries/rib-base/src/test/kotlin/com/uber/rib/core/RibCoroutineWorkerTest.kt +++ b/android/libraries/rib-base/src/test/kotlin/com/uber/rib/core/RibCoroutineWorkerTest.kt @@ -16,7 +16,10 @@ package com.uber.rib.core import com.google.common.truth.Truth.assertThat +import com.jakewharton.rxrelay2.BehaviorRelay import com.uber.autodispose.coroutinesinterop.autoDispose +import com.uber.rib.core.WorkerBinder.mapInteractorLifecycleToWorker +import com.uber.rib.core.lifecycle.InteractorEvent import io.reactivex.subjects.PublishSubject import kotlin.coroutines.CoroutineContext import kotlin.coroutines.EmptyCoroutineContext @@ -236,6 +239,43 @@ class RibCoroutineWorkerTest { assertThat(worker.onStopCause).isInstanceOf(CancellationException::class.java) assertThat(worker.onStopCause).hasMessageThat().isEqualTo("Worker was manually unbound.") } + + @Test + fun testClassThatIsBothWorkerAndRibCoroutineWorker() = runTest { + var workerOnStartCalled = false + var workerOnStopCalled = false + var ribCoroutineWorkerOnStartCalled = false + var ribCoroutineWorkerOnStopCalled = false + val worker = + WorkerAndRibCoroutineWorker( + workerOnStart = { workerOnStartCalled = true }, + workerOnStop = { workerOnStopCalled = true }, + ribCoroutineWorkerOnStart = { ribCoroutineWorkerOnStartCalled = true }, + ribCoroutineWorkerOnStop = { ribCoroutineWorkerOnStopCalled = true }, + ) + val lifecycle = BehaviorRelay.createDefault(InteractorEvent.ACTIVE) + val workerUnbinder = + WorkerBinder.bind(mapInteractorLifecycleToWorker(lifecycle), worker.asWorker()) + assertThat(workerOnStartCalled).isTrue() + assertThat(workerOnStopCalled).isFalse() + assertThat(ribCoroutineWorkerOnStartCalled).isFalse() + assertThat(ribCoroutineWorkerOnStopCalled).isFalse() + workerUnbinder.unbind() + assertThat(workerOnStopCalled).isTrue() + assertThat(ribCoroutineWorkerOnStartCalled).isFalse() + assertThat(ribCoroutineWorkerOnStopCalled).isFalse() + workerOnStartCalled = false + workerOnStopCalled = false + val unbinder = bind(worker.asRibCoroutineWorker()).also { it.join() } + assertThat(workerOnStartCalled).isFalse() + assertThat(workerOnStopCalled).isFalse() + assertThat(ribCoroutineWorkerOnStartCalled).isTrue() + assertThat(ribCoroutineWorkerOnStopCalled).isFalse() + unbinder.unbind().join() + assertThat(workerOnStartCalled).isFalse() + assertThat(workerOnStopCalled).isFalse() + assertThat(ribCoroutineWorkerOnStopCalled).isTrue() + } } @OptIn(InternalCoroutinesApi::class) @@ -315,3 +355,24 @@ private class TestWorker : RibCoroutineWorker { } } } + +/** + * This pattern is *not* recommended. New classes should *only* implement [RibCoroutineWorker]. + * If a class already implements [Worker] and is to be used as a [RibCoroutineWorker], it should either: + * 1. Migrate away from [Worker] to [RibCoroutineWorker], or + * 2. Be converted to [RibCoroutineWorker] using [Worker.asRibCoroutineWorker]. + */ +private class WorkerAndRibCoroutineWorker( + val workerOnStart: (WorkerScopeProvider) -> Unit, + val workerOnStop: () -> Unit, + val ribCoroutineWorkerOnStart: (CoroutineScope) -> Unit, + val ribCoroutineWorkerOnStop: (Throwable) -> Unit, +) : Worker, RibCoroutineWorker { + // Worker impl + override fun onStart(lifecycle: WorkerScopeProvider) = workerOnStart(lifecycle) + override fun onStop() = workerOnStop() + + // RibCoroutineWorker impl + override suspend fun onStart(scope: CoroutineScope) = ribCoroutineWorkerOnStart(scope) + override fun onStop(cause: Throwable) = ribCoroutineWorkerOnStop(cause) +}