fixed an issue with state while reroute request is interrupted by a new reroute request

Łukasz Paczos · Łukasz Paczos · commit a3da3bae713f · 2022-12-13T11:20:45.000+01:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -9,6 +9,7 @@ Mapbox welcomes participation and contributions from everyone.
 #### Bug fixes and improvements
 - Fixed approaches list update in `RouteOptionsUpdater`(uses for reroute). It was putting to the origin approach corresponding approach from legacy approach list. [#6540](https://github.com/mapbox/mapbox-navigation-android/pull/6540)
 - Updated the `MapboxRestAreaApi` logic to load a SAPA map only if the upcoming rest stop is at the current step of the route leg. [#6695](https://github.com/mapbox/mapbox-navigation-android/pull/6695)
+- Fixed an issue where `MapboxRerouteController` could deliver a delayed interruption state notifications. Now, the interruption state is always delivered synchronously, as soon as it's available. [#6718](https://github.com/mapbox/mapbox-navigation-android/pull/6718)
 
 ## Mapbox Navigation SDK 2.9.5 - 13 December, 2022
 ### Changelog
diff --git a/libnavigation-core/src/main/java/com/mapbox/navigation/core/reroute/MapboxRerouteController.kt b/libnavigation-core/src/main/java/com/mapbox/navigation/core/reroute/MapboxRerouteController.kt
@@ -20,9 +20,8 @@ import com.mapbox.navigation.utils.internal.JobControl
 import com.mapbox.navigation.utils.internal.ThreadController
 import com.mapbox.navigation.utils.internal.ifNonNull
 import com.mapbox.navigation.utils.internal.logD
-import com.mapbox.navigation.utils.internal.logW
+import com.mapbox.navigation.utils.internal.logI
 import kotlinx.coroutines.Job
-import kotlinx.coroutines.cancel
 import kotlinx.coroutines.launch
 import kotlinx.coroutines.suspendCancellableCoroutine
 import java.util.concurrent.CopyOnWriteArraySet
@@ -44,8 +43,6 @@ internal class MapboxRerouteController @VisibleForTesting constructor(
 
     private val mainJobController: JobControl = threadController.getMainScopeAndRootJob()
 
-    private var requestId: Long? = null
-
     private var rerouteJob: Job? = null
 
     constructor(
@@ -70,17 +67,6 @@ internal class MapboxRerouteController @VisibleForTesting constructor(
                 return
             }
             field = value
-            when (value) {
-                RerouteState.Idle,
-                RerouteState.Interrupted,
-                is RerouteState.Failed,
-                is RerouteState.RouteFetched -> {
-                    requestId = null
-                }
-                RerouteState.FetchingRoute -> {
-                    // no impl
-                }
-            }
             observers.forEach { it.onRerouteStateChanged(field) }
         }
 
@@ -189,18 +175,14 @@ internal class MapboxRerouteController @VisibleForTesting constructor(
 
     @MainThread
     override fun interrupt() {
+        rerouteJob?.cancel()
+        rerouteJob = null
         if (state == RerouteState.FetchingRoute) {
-            requestId?.also { id ->
-                directionsSession.cancelRouteRequest(id)
-                logD(LOG_CATEGORY) {
-                    "Route request interrupted"
-                }
-            } ?: logW(LOG_CATEGORY) {
-                "Tried interrupting but there's no ongoing request"
+            logI(LOG_CATEGORY) {
+                "Request interrupted via controller"
             }
-            rerouteJob?.cancel()
-            rerouteJob = null
         }
+        onRequestInterrupted()
     }
 
     override fun registerRerouteStateObserver(
@@ -223,8 +205,7 @@ internal class MapboxRerouteController @VisibleForTesting constructor(
         routeOptions: RouteOptions
     ) {
         rerouteJob = mainJobController.scope.launch {
-            val result = requestAsync(routeOptions)
-            when (result) {
+            when (val result = requestAsync(routeOptions)) {
                 is RouteRequestResult.Success -> {
                     mainJobController.scope.launch {
                         state = RerouteState.RouteFetched(result.routerOrigin)
@@ -241,7 +222,12 @@ internal class MapboxRerouteController @VisibleForTesting constructor(
                         state = RerouteState.Idle
                     }
                 }
-                is RouteRequestResult.Cancellation -> onRequestInterrupted()
+                is RouteRequestResult.Cancellation -> {
+                    if (state == RerouteState.FetchingRoute) {
+                        logI("Request canceled via router")
+                    }
+                    onRequestInterrupted()
+                }
             }
         }
     }
@@ -251,16 +237,15 @@ internal class MapboxRerouteController @VisibleForTesting constructor(
     }
 
     private fun onRequestInterrupted() {
-        mainJobController.scope.launch {
+        if (state == RerouteState.FetchingRoute) {
             state = RerouteState.Interrupted
             state = RerouteState.Idle
         }
     }
 
     private suspend fun requestAsync(routeOptions: RouteOptions): RouteRequestResult {
         return suspendCancellableCoroutine { cont ->
-            cont.invokeOnCancellation { onRequestInterrupted() }
-            requestId = directionsSession.requestRoutes(
+            val requestId = directionsSession.requestRoutes(
                 routeOptions,
                 object : NavigationRouterCallback {
                     override fun onRoutesReady(
@@ -291,6 +276,9 @@ internal class MapboxRerouteController @VisibleForTesting constructor(
                     }
                 }
             )
+            cont.invokeOnCancellation {
+                directionsSession.cancelRouteRequest(requestId)
+            }
         }
     }
 }
diff --git a/libnavigation-core/src/test/java/com/mapbox/navigation/core/reroute/MapboxRerouteControllerTest.kt b/libnavigation-core/src/test/java/com/mapbox/navigation/core/reroute/MapboxRerouteControllerTest.kt
@@ -49,7 +49,7 @@ class MapboxRerouteControllerTest {
 
     private lateinit var rerouteController: MapboxRerouteController
 
-    @MockK
+    @MockK(relaxUnitFun = true)
     private lateinit var directionsSession: DirectionsSession
 
     @MockK
@@ -317,25 +317,57 @@ class MapboxRerouteControllerTest {
 
     @Test
     fun reroute_calls_interrupt_if_currently_fetching() = coroutineRule.runBlockingTest {
-        mockRouteOptionsResult(successFromResult)
-        val routeRequestCallback = slot<NavigationRouterCallback>()
+        addRerouteStateObserver()
+        val routeOptions1 = MapboxJavaObjectsFactory.routeOptions(
+            coordinates = listOf(Point.fromLngLat(1.0, 2.0), Point.fromLngLat(3.0, 4.0))
+        )
+        val updaterSuccess1 = RouteOptionsUpdater.RouteOptionsResult.Success(routeOptions1)
+        val routeOptions2 = MapboxJavaObjectsFactory.routeOptions(
+            coordinates = listOf(Point.fromLngLat(1.5, 2.5), Point.fromLngLat(3.0, 4.0))
+        )
+        val updaterSuccess2 = RouteOptionsUpdater.RouteOptionsResult.Success(routeOptions2)
+        val routeRequestCallback1 = slot<NavigationRouterCallback>()
         every {
             directionsSession.requestRoutes(
-                routeOptionsFromSuccessResult,
-                capture(routeRequestCallback)
+                routeOptions1,
+                capture(routeRequestCallback1)
             )
         } returns 1L
-        rerouteController.reroute(routeCallback)
+        val routeRequestCallback2 = slot<NavigationRouterCallback>()
+        every {
+            directionsSession.requestRoutes(
+                routeOptions2,
+                capture(routeRequestCallback2)
+            )
+        } returns 2L
 
+        mockRouteOptionsResult(updaterSuccess1)
         rerouteController.reroute(routeCallback)
-        routeRequestCallback.captured.onRoutesReady(listOf(mockk(relaxed = true)), mockk())
+        pauseDispatcher {
+            // this ensure that we don't run coroutines synchronously that could
+            // make the test pass even if state changes were scheduled back to the message queue
+            // in an incorrect order
+            mockRouteOptionsResult(updaterSuccess2)
+            rerouteController.reroute(routeCallback)
+            verify(exactly = 1) { directionsSession.cancelRouteRequest(1L) }
+            routeRequestCallback1.captured.onCanceled(routeOptions1, mockk())
+        }
+
+        routeRequestCallback2.captured.onRoutesReady(listOf(mockk(relaxed = true)), mockk())
 
-        verify(exactly = 1) { directionsSession.cancelRouteRequest(1L) }
+        verifyOrder {
+            primaryRerouteObserver.onRerouteStateChanged(RerouteState.FetchingRoute)
+            primaryRerouteObserver.onRerouteStateChanged(RerouteState.Interrupted)
+            primaryRerouteObserver.onRerouteStateChanged(RerouteState.Idle)
+            primaryRerouteObserver.onRerouteStateChanged(RerouteState.FetchingRoute)
+            primaryRerouteObserver.onRerouteStateChanged(ofType<RerouteState.RouteFetched>())
+        }
     }
 
     @Test
     fun reroute_only_calls_interrupt_if_currently_fetching() {
         mockRouteOptionsResult(successFromResult)
+        addRerouteStateObserver()
         val routeRequestCallback = slot<NavigationRouterCallback>()
         every {
             directionsSession.requestRoutes(
@@ -349,6 +381,9 @@ class MapboxRerouteControllerTest {
 
         verify(exactly = 0) { directionsSession.cancelAll() }
         verify(exactly = 0) { directionsSession.cancelRouteRequest(any()) }
+        verify(exactly = 0) {
+            primaryRerouteObserver.onRerouteStateChanged(RerouteState.Interrupted)
+        }
     }
 
     @Test
