Merge pull request #629 from psteiger/rrw-converters-fix

Support classes that are both `Worker` and `RibCoroutineWorker` in co…

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 {

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)
+}