NAVAND-777: fix wrong child dispatcher

dzinad · dzinad · commit e043aec9934f · 2023-03-01T17:29:39.000+03:00
diff --git a/libnavigation-core/src/main/java/com/mapbox/navigation/core/internal/utils/CoroutineUtils.kt b/libnavigation-core/src/main/java/com/mapbox/navigation/core/internal/utils/CoroutineUtils.kt
@@ -3,13 +3,21 @@ package com.mapbox.navigation.core.internal.utils
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Job
 import kotlinx.coroutines.SupervisorJob
+import kotlinx.coroutines.job
 import kotlin.coroutines.CoroutineContext
-import kotlin.coroutines.EmptyCoroutineContext
 
 internal object CoroutineUtils {
 
-    fun createChildScope(
+    fun createScope(
         parentJob: Job,
-        additionalContext: CoroutineContext = EmptyCoroutineContext
-    ): CoroutineScope = CoroutineScope(SupervisorJob(parentJob) + additionalContext)
+        additionalContext: CoroutineContext
+    ): CoroutineScope {
+        return CoroutineScope(SupervisorJob(parentJob) + additionalContext)
+    }
+
+    fun createChildScope(
+        parentScope: CoroutineScope
+    ): CoroutineScope = CoroutineScope(
+        SupervisorJob(parentScope.coroutineContext.job) + parentScope.coroutineContext.minusKey(Job)
+    )
 }
diff --git a/libnavigation-core/src/main/java/com/mapbox/navigation/core/routerefresh/PlannedRouteRefreshController.kt b/libnavigation-core/src/main/java/com/mapbox/navigation/core/routerefresh/PlannedRouteRefreshController.kt
@@ -11,7 +11,6 @@ import kotlinx.coroutines.CancellationException
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.cancel
 import kotlinx.coroutines.delay
-import kotlinx.coroutines.job
 import kotlinx.coroutines.launch
 
 @OptIn(ExperimentalPreviewMapboxNavigationAPI::class)
@@ -39,8 +38,7 @@ internal class PlannedRouteRefreshController @VisibleForTesting constructor(
         RetryRouteRefreshStrategy(maxAttemptsCount = MAX_RETRY_COUNT)
     )
 
-    private var plannedRefreshScope =
-        CoroutineUtils.createChildScope(parentScope.coroutineContext.job)
+    private var plannedRefreshScope = CoroutineUtils.createChildScope(parentScope)
     private var paused = false
     var routesToRefresh: List<NavigationRoute>? = null
         private set
@@ -151,7 +149,7 @@ internal class PlannedRouteRefreshController @VisibleForTesting constructor(
 
     private fun recreateScope() {
         plannedRefreshScope.cancel()
-        plannedRefreshScope = CoroutineUtils.createChildScope(parentScope.coroutineContext.job)
+        plannedRefreshScope = CoroutineUtils.createChildScope(parentScope)
     }
 
     companion object {
diff --git a/libnavigation-core/src/main/java/com/mapbox/navigation/core/routerefresh/RouteRefreshControllerProvider.kt b/libnavigation-core/src/main/java/com/mapbox/navigation/core/routerefresh/RouteRefreshControllerProvider.kt
@@ -55,13 +55,13 @@ internal object RouteRefreshControllerProvider {
             routeRefresherExecutor,
             routeRefreshOptions,
             stateHolder,
-            CoroutineUtils.createChildScope(routeRefreshParentJob, immediateDispatcher),
+            CoroutineUtils.createScope(routeRefreshParentJob, immediateDispatcher),
             routeRefresherResultProcessor
         )
         val immediateRouteRefreshController = ImmediateRouteRefreshController(
             routeRefresherExecutor,
             stateHolder,
-            CoroutineUtils.createChildScope(routeRefreshParentJob, dispatcher),
+            CoroutineUtils.createScope(routeRefreshParentJob, dispatcher),
             routeRefresherResultProcessor
         )
         return RouteRefreshController(
diff --git a/libnavigation-core/src/test/java/com/mapbox/navigation/core/internal/utils/CoroutineUtilsTest.kt b/libnavigation-core/src/test/java/com/mapbox/navigation/core/internal/utils/CoroutineUtilsTest.kt
@@ -1,34 +1,53 @@
 package com.mapbox.navigation.core.internal.utils
 
+import android.os.Handler
+import android.os.HandlerThread
+import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Job
 import kotlinx.coroutines.SupervisorJob
+import kotlinx.coroutines.android.asCoroutineDispatcher
 import kotlinx.coroutines.cancel
 import kotlinx.coroutines.delay
 import kotlinx.coroutines.launch
+import kotlinx.coroutines.runBlocking
 import org.junit.After
+import org.junit.Assert.assertEquals
 import org.junit.Assert.assertFalse
+import org.junit.Assert.assertNotNull
 import org.junit.Assert.assertTrue
 import org.junit.Before
 import org.junit.Test
+import org.junit.runner.RunWith
+import org.robolectric.RobolectricTestRunner
+import kotlin.coroutines.EmptyCoroutineContext
 
+@RunWith(RobolectricTestRunner::class)
 class CoroutineUtilsTest {
 
+    private lateinit var parentThread: HandlerThread
+    private lateinit var parentScope: CoroutineScope
     private lateinit var parentJob: Job
 
     @Before
     fun setUp() {
+        parentThread = HandlerThread("parent thread").also { it.start() }
         parentJob = SupervisorJob()
+        parentScope = CoroutineScope(
+            SupervisorJob() + Handler(parentThread.looper).asCoroutineDispatcher()
+        )
     }
 
     @After
     fun tearDown() {
+        parentScope.cancel()
         parentJob.cancel()
+        parentThread.quit()
     }
 
     @Test
     fun createChildScope_parentCancellationCancelsChildren() {
-        val scope1 = CoroutineUtils.createChildScope(parentJob)
-        val scope2 = CoroutineUtils.createChildScope(parentJob)
+        val scope1 = CoroutineUtils.createChildScope(parentScope)
+        val scope2 = CoroutineUtils.createChildScope(parentScope)
 
         val job1 = scope1.launch {
             delay(5000)
@@ -40,16 +59,78 @@ class CoroutineUtilsTest {
         assertFalse(job1.isCancelled)
         assertFalse(job2.isCancelled)
 
-        parentJob.cancel()
+        parentScope.cancel()
 
         assertTrue(job1.isCancelled)
         assertTrue(job2.isCancelled)
     }
 
     @Test
     fun createChildScope_childDoesNotCancelOtherChildren() {
-        val scope1 = CoroutineUtils.createChildScope(parentJob)
-        val scope2 = CoroutineUtils.createChildScope(parentJob)
+        val scope1 = CoroutineUtils.createChildScope(parentScope)
+        val scope2 = CoroutineUtils.createChildScope(parentScope)
+
+        val job1 = scope1.launch {
+            delay(5000)
+        }
+        val job2 = scope2.launch {
+            delay(5000)
+        }
+
+        assertFalse(job1.isCancelled)
+        assertFalse(job2.isCancelled)
+
+        scope1.cancel()
+
+        assertTrue(job1.isCancelled)
+        assertFalse(job2.isCancelled)
+    }
+
+    @Test
+    fun createChildScope_childrenUseParentDispatcher() = runBlocking {
+        val childScope = CoroutineUtils.createChildScope(parentScope)
+
+        var childThread: Long? = null
+        var parentThread: Long? = null
+        val childJob = childScope.launch {
+            childThread = Thread.currentThread().id
+        }
+        val parentJob = parentScope.launch {
+            parentThread = Thread.currentThread().id
+        }
+
+        parentJob.join()
+        childJob.join()
+
+        assertNotNull(childThread)
+        assertEquals(parentThread, childThread)
+    }
+
+    @Test
+    fun createScope_parentCancellationCancelsChildren() {
+        val scope1 = CoroutineUtils.createScope(parentJob, EmptyCoroutineContext)
+        val scope2 = CoroutineUtils.createScope(parentJob, EmptyCoroutineContext)
+
+        val job1 = scope1.launch {
+            delay(5000)
+        }
+        val job2 = scope2.launch {
+            delay(5000)
+        }
+
+        assertFalse(job1.isCancelled)
+        assertFalse(job2.isCancelled)
+
+        parentJob.cancel()
+
+        assertTrue(job1.isCancelled)
+        assertTrue(job2.isCancelled)
+    }
+
+    @Test
+    fun createScope_childDoesNotCancelOtherChildren() {
+        val scope1 = CoroutineUtils.createScope(parentJob, EmptyCoroutineContext)
+        val scope2 = CoroutineUtils.createScope(parentJob, EmptyCoroutineContext)
 
         val job1 = scope1.launch {
             delay(5000)
@@ -66,4 +147,38 @@ class CoroutineUtilsTest {
         assertTrue(job1.isCancelled)
         assertFalse(job2.isCancelled)
     }
+
+    @Test
+    fun createScope_childrenUseOwnDispatcher() = runBlocking {
+        val thread1 = HandlerThread("thread 1").also { it.start() }
+        val thread2 = HandlerThread("thread 2").also { it.start() }
+        try {
+            val childScope1 = CoroutineUtils.createScope(
+                parentJob,
+                Handler(thread1.looper).asCoroutineDispatcher()
+            )
+            val childScope2 = CoroutineUtils.createScope(
+                parentJob,
+                Handler(thread2.looper).asCoroutineDispatcher()
+            )
+
+            var childThread1: Thread? = null
+            var childThread2: Thread? = null
+            val childJob1 = childScope1.launch {
+                childThread1 = Thread.currentThread()
+            }
+            val childJob2 = childScope2.launch {
+                childThread2 = Thread.currentThread()
+            }
+
+            childJob1.join()
+            childJob2.join()
+
+            assertEquals(thread1, childThread1)
+            assertEquals(thread2, childThread2)
+        } finally {
+            thread1.quit()
+            thread2.quit()
+        }
+    }
 }
diff --git a/libnavigation-core/src/test/java/com/mapbox/navigation/core/routerefresh/PlannedRouteRefreshControllerTest.kt b/libnavigation-core/src/test/java/com/mapbox/navigation/core/routerefresh/PlannedRouteRefreshControllerTest.kt
@@ -23,7 +23,6 @@ import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.ExperimentalCoroutinesApi
 import kotlinx.coroutines.SupervisorJob
 import kotlinx.coroutines.cancel
-import kotlinx.coroutines.job
 import kotlinx.coroutines.test.TestCoroutineDispatcher
 import kotlinx.coroutines.test.TestCoroutineScope
 import org.junit.After
@@ -59,7 +58,7 @@ class PlannedRouteRefreshControllerTest {
     fun setUp() {
         mockkObject(RouteRefreshValidator)
         mockkObject(CoroutineUtils)
-        every { CoroutineUtils.createChildScope(parentScope.coroutineContext.job) } answers {
+        every { CoroutineUtils.createChildScope(parentScope) } answers {
             TestCoroutineScope(SupervisorJob() + childScopeDispatcher).also { childScope = it }
         }
         sut = PlannedRouteRefreshController(
diff --git a/libnavigation-core/src/test/java/com/mapbox/navigation/core/routerefresh/RouteRefreshIntegrationTest.kt b/libnavigation-core/src/test/java/com/mapbox/navigation/core/routerefresh/RouteRefreshIntegrationTest.kt
@@ -100,7 +100,7 @@ internal open class RouteRefreshIntegrationTest {
         } returns ExpectedFactory.createValue(listOf(mockk(relaxed = true)))
         mockkObject(CoroutineUtils)
         every {
-            CoroutineUtils.createChildScope(any(), any())
+            CoroutineUtils.createScope(any(), any())
         } answers { coroutineRule.createTestScope() }
 
         primaryRouteProgressDataProvider.onRouteProgressChanged(
