NAVAND-777: use scopes instead of CancellableHandler

Author: dzinad

libnavigation-core/src/main/java/com/mapbox/navigation/core/internal/utils/CoroutineUtils.kt

@@ -0,0 +1,15 @@
+package com.mapbox.navigation.core.internal.utils
+
+import kotlinx.coroutines.CoroutineScope
+import kotlinx.coroutines.Job
+import kotlinx.coroutines.SupervisorJob
+import kotlin.coroutines.CoroutineContext
+import kotlin.coroutines.EmptyCoroutineContext
+
+internal object CoroutineUtils {
+
+    fun createChildScope(
+        parentJob: Job,
+        additionalContext: CoroutineContext = EmptyCoroutineContext
+    ): CoroutineScope = CoroutineScope(SupervisorJob(parentJob) + additionalContext)
+}

libnavigation-core/src/main/java/com/mapbox/navigation/core/routerefresh/CancellableHandler.kt

@@ -4,41 +4,49 @@ import androidx.annotation.VisibleForTesting
 import com.mapbox.navigation.base.ExperimentalPreviewMapboxNavigationAPI
 import com.mapbox.navigation.base.route.NavigationRoute
 import com.mapbox.navigation.base.route.RouteRefreshOptions
+import com.mapbox.navigation.core.internal.utils.CoroutineUtils
 import com.mapbox.navigation.utils.internal.logI
 import com.mapbox.navigation.utils.internal.logW
+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)
 internal class PlannedRouteRefreshController @VisibleForTesting constructor(
     private val routeRefresherExecutor: RouteRefresherExecutor,
     private val routeRefreshOptions: RouteRefreshOptions,
     private val stateHolder: RouteRefreshStateHolder,
     private val listener: RouteRefresherListener,
-    private val cancellableHandler: CancellableHandler,
+    private val parentScope: CoroutineScope,
     private val retryStrategy: RetryRouteRefreshStrategy,
 ) {
 
     constructor(
         routeRefresherExecutor: RouteRefresherExecutor,
         routeRefreshOptions: RouteRefreshOptions,
         stateHolder: RouteRefreshStateHolder,
-        scope: CoroutineScope,
+        parentScope: CoroutineScope,
         listener: RouteRefresherListener,
     ) : this(
         routeRefresherExecutor,
         routeRefreshOptions,
         stateHolder,
         listener,
-        CancellableHandler(scope),
+        parentScope,
         RetryRouteRefreshStrategy(maxAttemptsCount = MAX_RETRY_COUNT)
     )
 
+    private var plannedRefreshScope =
+        CoroutineUtils.createChildScope(parentScope.coroutineContext.job)
     private var paused = false
     var routesToRefresh: List<NavigationRoute>? = null
         private set
 
     fun startRoutesRefreshing(routes: List<NavigationRoute>) {
-        cancellableHandler.cancelAll()
+        recreateScope()
         routesToRefresh = null
         if (routes.isEmpty()) {
             logI("Routes are empty, nothing to refresh", RouteRefreshLog.LOG_CATEGORY)
@@ -69,7 +77,7 @@ internal class PlannedRouteRefreshController @VisibleForTesting constructor(
     fun pause() {
         if (!paused) {
             paused = true
-            cancellableHandler.cancelAll()
+            recreateScope()
         }
     }
 
@@ -99,11 +107,15 @@ internal class PlannedRouteRefreshController @VisibleForTesting constructor(
     }
 
     private fun postAttempt(attemptBlock: suspend () -> Unit) {
-        cancellableHandler.postDelayed(
-            timeout = routeRefreshOptions.intervalMillis,
-            block = attemptBlock,
-            cancellationCallback = { stateHolder.onCancel() }
-        )
+        plannedRefreshScope.launch {
+            try {
+                delay(routeRefreshOptions.intervalMillis)
+                attemptBlock()
+            } catch (ex: CancellationException) {
+                stateHolder.onCancel()
+                throw ex
+            }
+        }
     }
 
     private suspend fun executePlannedRefresh(
@@ -137,6 +149,11 @@ internal class PlannedRouteRefreshController @VisibleForTesting constructor(
         )
     }
 
+    private fun recreateScope() {
+        plannedRefreshScope.cancel()
+        plannedRefreshScope = CoroutineUtils.createChildScope(parentScope.coroutineContext.job)
+    }
+
     companion object {
 
         const val MAX_RETRY_COUNT = 2

libnavigation-core/src/main/java/com/mapbox/navigation/core/routerefresh/PlannedRouteRefreshController.kt

@@ -3,12 +3,11 @@ package com.mapbox.navigation.core.routerefresh
 import com.mapbox.navigation.base.ExperimentalPreviewMapboxNavigationAPI
 import com.mapbox.navigation.base.route.NavigationRoute
 import com.mapbox.navigation.utils.internal.logI
-import kotlinx.coroutines.CoroutineScope
-import kotlinx.coroutines.cancel
+import kotlinx.coroutines.Job
 
 @OptIn(ExperimentalPreviewMapboxNavigationAPI::class)
 class RouteRefreshController internal constructor(
-    private val routeRefreshScope: CoroutineScope,
+    private val routeRefreshParentJob: Job,
     private val plannedRouteRefreshController: PlannedRouteRefreshController,
     private val immediateRouteRefreshController: ImmediateRouteRefreshController,
     private val stateHolder: RouteRefreshStateHolder,
@@ -56,7 +55,7 @@ class RouteRefreshController internal constructor(
         refreshObserversManager.unregisterAllObservers()
         stateHolder.unregisterAllRouteRefreshStateObservers()
         // first unregister observers, then cancel scope - otherwise we dispatch CANCELLED state from onDestroy
-        routeRefreshScope.cancel()
+        routeRefreshParentJob.cancel()
     }
 
     internal fun requestPlannedRouteRefresh(routes: List<NavigationRoute>) {

libnavigation-core/src/main/java/com/mapbox/navigation/core/routerefresh/RouteRefreshController.kt

@@ -7,10 +7,10 @@ import com.mapbox.navigation.core.RoutesProgressDataProvider
 import com.mapbox.navigation.core.directions.session.DirectionsSession
 import com.mapbox.navigation.core.ev.EVDynamicDataHolder
 import com.mapbox.navigation.core.ev.EVRefreshDataProvider
+import com.mapbox.navigation.core.internal.utils.CoroutineUtils
 import com.mapbox.navigation.core.routealternatives.AlternativeMetadataProvider
 import com.mapbox.navigation.utils.internal.Time
 import kotlinx.coroutines.CoroutineDispatcher
-import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.SupervisorJob
 import java.util.Date
 
@@ -50,22 +50,22 @@ internal object RouteRefreshControllerProvider {
             routeRefreshOptions.intervalMillis * 3
         )
 
-        val routeRefreshParentScope = CoroutineScope(SupervisorJob())
+        val routeRefreshParentJob = SupervisorJob()
         val plannedRouteRefreshController = PlannedRouteRefreshController(
             routeRefresherExecutor,
             routeRefreshOptions,
             stateHolder,
-            CoroutineScope(routeRefreshParentScope.coroutineContext + immediateDispatcher),
+            CoroutineUtils.createChildScope(routeRefreshParentJob, immediateDispatcher),
             routeRefresherResultProcessor
         )
         val immediateRouteRefreshController = ImmediateRouteRefreshController(
             routeRefresherExecutor,
             stateHolder,
-            CoroutineScope(routeRefreshParentScope.coroutineContext + dispatcher),
+            CoroutineUtils.createChildScope(routeRefreshParentJob, dispatcher),
             routeRefresherResultProcessor
         )
         return RouteRefreshController(
-            routeRefreshParentScope,
+            routeRefreshParentJob,
             plannedRouteRefreshController,
             immediateRouteRefreshController,
             stateHolder,

libnavigation-core/src/main/java/com/mapbox/navigation/core/routerefresh/RouteRefreshControllerProvider.kt

@@ -0,0 +1,69 @@
+package com.mapbox.navigation.core.internal.utils
+
+import kotlinx.coroutines.Job
+import kotlinx.coroutines.SupervisorJob
+import kotlinx.coroutines.cancel
+import kotlinx.coroutines.delay
+import kotlinx.coroutines.launch
+import org.junit.After
+import org.junit.Assert.assertFalse
+import org.junit.Assert.assertTrue
+import org.junit.Before
+import org.junit.Test
+
+class CoroutineUtilsTest {
+
+    private lateinit var parentJob: Job
+
+    @Before
+    fun setUp() {
+        parentJob = SupervisorJob()
+    }
+
+    @After
+    fun tearDown() {
+        parentJob.cancel()
+    }
+
+    @Test
+    fun createChildScope_parentCancellationCancelsChildren() {
+        val scope1 = CoroutineUtils.createChildScope(parentJob)
+        val scope2 = CoroutineUtils.createChildScope(parentJob)
+
+        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 createChildScope_childDoesNotCancelOtherChildren() {
+        val scope1 = CoroutineUtils.createChildScope(parentJob)
+        val scope2 = CoroutineUtils.createChildScope(parentJob)
+
+        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)
+    }
+}