NAVAND-1472: do not always relaunch timer when routes are updated (#7497

)

changelog/unreleased/bugfixes/7497.md

@@ -0,0 +1 @@
+- Fixed an issue where route refresh interval might have been too long in case of a specific alternatives update rate. 

instrumentation-tests/src/androidTest/java/com/mapbox/navigation/instrumentation_tests/core/RefreshTtlTest.kt

@@ -216,7 +216,9 @@ class RefreshTtlTest : BaseCoreNoCleanUpTest() {
                     route.id.startsWith("alternative_route_response_for_route_with_large_ttl")
                 }
         }.first()
-        val invalidatedResults = withTimeout(4000) {
+        // worst case: refresh request is scheduled 1900 ms after we receive an alternative,
+        // refresh_ttl (=2) is not expired, then we wait for new attempt (+3s). +1s accuracy.
+        val invalidatedResults = withTimeout(6000) {
             mapboxNavigation.routesInvalidatedResults().first()
         }
         assertEquals(

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

@@ -583,7 +583,7 @@ class MapboxNavigation @VisibleForTesting internal constructor(
             directionsSession,
             routesProgressDataProvider,
             evDynamicDataHolder,
-            Time.SystemImpl
+            Time.SystemClockImpl
         )
         @OptIn(ExperimentalPreviewMapboxNavigationAPI::class)
         routeRefreshController.registerRouteRefreshObserver {
@@ -610,6 +610,7 @@ class MapboxNavigation @VisibleForTesting internal constructor(
         tripSession.registerOffRouteObserver(internalOffRouteObserver)
         tripSession.registerFallbackVersionsObserver(internalFallbackVersionsObserver)
         registerRoutesObserver(internalRoutesObserver)
+        registerRoutesObserver(routeRefreshController)
         setUpRouteCacheClearer()
 
         roadObjectsStore = RoadObjectsStore(navigator)
@@ -1990,11 +1991,9 @@ class MapboxNavigation @VisibleForTesting internal constructor(
         }
     }
 
-    private fun createInternalRoutesObserver() = RoutesObserver { result ->
+    private fun createInternalRoutesObserver() = RoutesObserver { _ ->
         latestLegIndex = null
         routesProgressDataProvider.onNewRoutes()
-        @OptIn(ExperimentalPreviewMapboxNavigationAPI::class)
-        routeRefreshController.requestPlannedRouteRefresh(result.navigationRoutes)
     }
 
     private fun createInternalOffRouteObserver() = OffRouteObserver { offRoute ->

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

@@ -1,10 +1,12 @@
 package com.mapbox.navigation.core.routerefresh
 
-import com.mapbox.bindgen.Expected
 import com.mapbox.navigation.base.ExperimentalPreviewMapboxNavigationAPI
 import com.mapbox.navigation.base.route.NavigationRoute
 import com.mapbox.navigation.utils.internal.logW
+import kotlinx.coroutines.CancellationException
 import kotlinx.coroutines.CoroutineScope
+import kotlinx.coroutines.cancelChildren
+import kotlinx.coroutines.job
 import kotlinx.coroutines.launch
 
 @OptIn(ExperimentalPreviewMapboxNavigationAPI::class)
@@ -19,29 +21,45 @@ internal class ImmediateRouteRefreshController(
     @Throws(IllegalArgumentException::class)
     fun requestRoutesRefresh(
         routes: List<NavigationRoute>,
-        callback: (Expected<String, RoutesRefresherResult>) -> Unit
+        callback: (RoutesRefresherExecutorResult) -> Unit
     ) {
         if (routes.isEmpty()) {
             throw IllegalArgumentException("Routes to refresh should not be empty")
         }
         scope.launch {
-            val result = routeRefresherExecutor.executeRoutesRefresh(
-                routes,
-                startCallback = { stateHolder.onStarted() }
-            )
+            val result = try {
+                routeRefresherExecutor.executeRoutesRefresh(
+                    routes,
+                    startCallback = { stateHolder.onStarted() }
+                )
+            } catch (ex: CancellationException) {
+                stateHolder.onCancel()
+                throw ex
+            }
+
             callback(result)
-            result.fold(
-                { logW("Route refresh on-demand error: $it", RouteRefreshLog.LOG_CATEGORY) },
-                {
-                    attemptListener.onRoutesRefreshAttemptFinished(it)
-                    if (it.anySuccess()) {
+            when (result) {
+                is RoutesRefresherExecutorResult.ReplacedByNewer -> {
+                    logW(
+                        "Route refresh on-demand error: " +
+                            "request is skipped as a newer one is available",
+                        RouteRefreshLog.LOG_CATEGORY
+                    )
+                }
+                is RoutesRefresherExecutorResult.Finished -> {
+                    attemptListener.onRoutesRefreshAttemptFinished(result.value)
+                    if (result.value.anySuccess()) {
                         stateHolder.onSuccess()
                     } else {
                         stateHolder.onFailure(null)
                     }
-                    listener.onRoutesRefreshed(it)
+                    listener.onRoutesRefreshed(result.value)
                 }
-            )
+            }
         }
     }
+
+    fun cancel() {
+        scope.coroutineContext.job.cancelChildren()
+    }
 }

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

@@ -5,18 +5,19 @@ 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.core.utils.Delayer
+import com.mapbox.navigation.utils.internal.Time
 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.launch
 
 @OptIn(ExperimentalPreviewMapboxNavigationAPI::class)
 internal class PlannedRouteRefreshController @VisibleForTesting constructor(
     private val routeRefresherExecutor: RouteRefresherExecutor,
-    private val routeRefreshOptions: RouteRefreshOptions,
+    private val delayer: Delayer,
     private val stateHolder: RouteRefreshStateHolder,
     private val listener: RouteRefresherListener,
     private val attemptListener: RoutesRefreshAttemptListener,
@@ -31,9 +32,10 @@ internal class PlannedRouteRefreshController @VisibleForTesting constructor(
         parentScope: CoroutineScope,
         listener: RouteRefresherListener,
         attemptListener: RoutesRefreshAttemptListener,
+        timeProvider: Time,
     ) : this(
         routeRefresherExecutor,
-        routeRefreshOptions,
+        Delayer(routeRefreshOptions.intervalMillis, timeProvider),
         stateHolder,
         listener,
         attemptListener,
@@ -48,8 +50,8 @@ internal class PlannedRouteRefreshController @VisibleForTesting constructor(
 
     fun startRoutesRefreshing(routes: List<NavigationRoute>) {
         recreateScope()
-        routesToRefresh = null
         if (routes.isEmpty()) {
+            routesToRefresh = null
             logI("Routes are empty, nothing to refresh", RouteRefreshLog.LOG_CATEGORY)
             stateHolder.reset()
             return
@@ -58,9 +60,13 @@ internal class PlannedRouteRefreshController @VisibleForTesting constructor(
         if (
             routesValidationResults.any { it is RouteRefreshValidator.RouteValidationResult.Valid }
         ) {
+            val hasSameRoutes = routesToRefresh?.any { route1 ->
+                routes.any { route2 -> route1.id == route2.id }
+            } == true
             routesToRefresh = routes
-            scheduleNewUpdate(routes)
+            scheduleNewUpdate(routes, hasSameRoutes)
         } else {
+            routesToRefresh = null
             val message =
                 RouteRefreshValidator.joinValidationErrorMessages(
                     routesValidationResults.mapIndexed { index, routeValidationResult ->
@@ -93,24 +99,28 @@ internal class PlannedRouteRefreshController @VisibleForTesting constructor(
         }
     }
 
-    private fun scheduleNewUpdate(routes: List<NavigationRoute>) {
+    private fun scheduleNewUpdate(routes: List<NavigationRoute>, shouldResume: Boolean) {
         retryStrategy.reset()
-        postAttempt {
+        postAttempt(shouldResume) {
             executePlannedRefresh(routes, shouldNotifyOnStart = true)
         }
     }
 
     private fun scheduleUpdateRetry(routes: List<NavigationRoute>, shouldNotifyOnStart: Boolean) {
-        postAttempt {
+        postAttempt(false) {
             retryStrategy.onNextAttempt()
             executePlannedRefresh(routes, shouldNotifyOnStart = shouldNotifyOnStart)
         }
     }
 
-    private fun postAttempt(attemptBlock: suspend () -> Unit) {
+    private fun postAttempt(shouldResume: Boolean, attemptBlock: suspend () -> Unit) {
         plannedRefreshScope.launch {
             try {
-                delay(routeRefreshOptions.intervalMillis)
+                if (shouldResume) {
+                    delayer.resumeDelay()
+                } else {
+                    delayer.delay()
+                }
                 attemptBlock()
             } catch (ex: CancellationException) {
                 stateHolder.onCancel()
@@ -131,24 +141,40 @@ internal class PlannedRouteRefreshController @VisibleForTesting constructor(
                 }
             }
         )
-        routeRefresherResult.fold(
-            { logW("Planned route refresh error: $it", RouteRefreshLog.LOG_CATEGORY) },
-            {
-                attemptListener.onRoutesRefreshAttemptFinished(it)
-                if (it.anySuccess()) {
+        when (routeRefresherResult) {
+            is RoutesRefresherExecutorResult.ReplacedByNewer -> {
+                logW(
+                    "Planned route refresh error: " +
+                        "request is skipped as a newer one is available",
+                    RouteRefreshLog.LOG_CATEGORY
+                )
+            }
+            is RoutesRefresherExecutorResult.Finished -> {
+                attemptListener.onRoutesRefreshAttemptFinished(routeRefresherResult.value)
+                if (routeRefresherResult.value.anySuccess()) {
                     stateHolder.onSuccess()
-                    listener.onRoutesRefreshed(it)
+                    listener.onRoutesRefreshed(routeRefresherResult.value)
+                    val refreshedRoutes = listOf(
+                        routeRefresherResult.value.primaryRouteRefresherResult.route
+                    ) + routeRefresherResult.value.alternativesRouteRefresherResults.map {
+                        it.route
+                    }
+                    routesToRefresh = refreshedRoutes
+                    scheduleNewUpdate(refreshedRoutes, false)
                 } else {
-                    if (it.anyRequestFailed() && retryStrategy.shouldRetry()) {
+                    if (
+                        routeRefresherResult.value.anyRequestFailed() &&
+                        retryStrategy.shouldRetry()
+                    ) {
                         scheduleUpdateRetry(routes, shouldNotifyOnStart = false)
                     } else {
                         stateHolder.onFailure(null)
-                        listener.onRoutesRefreshed(it)
-                        scheduleNewUpdate(routes)
+                        listener.onRoutesRefreshed(routeRefresherResult.value)
+                        scheduleNewUpdate(routes, false)
                     }
                 }
             }
-        )
+        }
     }
 
     private fun recreateScope() {

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

@@ -1,9 +1,10 @@
 package com.mapbox.navigation.core.routerefresh
 
 import com.mapbox.navigation.base.ExperimentalPreviewMapboxNavigationAPI
-import com.mapbox.navigation.base.route.NavigationRoute
 import com.mapbox.navigation.core.RoutesInvalidatedObserver
+import com.mapbox.navigation.core.directions.session.RoutesExtra
 import com.mapbox.navigation.core.directions.session.RoutesObserver
+import com.mapbox.navigation.core.directions.session.RoutesUpdatedResult
 import com.mapbox.navigation.utils.internal.logI
 import kotlinx.coroutines.Job
 
@@ -18,7 +19,7 @@ class RouteRefreshController internal constructor(
     private val stateHolder: RouteRefreshStateHolder,
     private val refreshObserversManager: RefreshObserversManager,
     private val routeRefresherResultProcessor: RouteRefresherResultProcessor,
-) {
+) : RoutesObserver {
 
     /**
      * Register a [RouteRefreshStatesObserver] to be notified of Route refresh state changes.
@@ -62,7 +63,7 @@ class RouteRefreshController internal constructor(
         }
         plannedRouteRefreshController.pause()
         immediateRouteRefreshController.requestRoutesRefresh(routes) {
-            if (it.value?.anySuccess() == false) {
+            if (it is RoutesRefresherExecutorResult.Finished) {
                 plannedRouteRefreshController.resume()
             }
         }
@@ -109,8 +110,11 @@ class RouteRefreshController internal constructor(
         routeRefreshParentJob.cancel()
     }
 
-    internal fun requestPlannedRouteRefresh(routes: List<NavigationRoute>) {
-        routeRefresherResultProcessor.reset()
-        plannedRouteRefreshController.startRoutesRefreshing(routes)
+    override fun onRoutesChanged(result: RoutesUpdatedResult) {
+        if (result.reason != RoutesExtra.ROUTES_UPDATE_REASON_REFRESH) {
+            routeRefresherResultProcessor.reset()
+            immediateRouteRefreshController.cancel()
+            plannedRouteRefreshController.startRoutesRefreshing(result.navigationRoutes)
+        }
     }
 }

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

@@ -57,6 +57,7 @@ internal object RouteRefreshControllerProvider {
             CoroutineUtils.createScope(routeRefreshParentJob, immediateDispatcher),
             routeRefresherResultProcessor,
             routeRefreshResultAttemptProcessor,
+            timeProvider,
         )
         val immediateRouteRefreshController = ImmediateRouteRefreshController(
             routeRefresherExecutor,

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

@@ -1,17 +1,22 @@
 package com.mapbox.navigation.core.routerefresh
 
-import com.mapbox.bindgen.Expected
-import com.mapbox.bindgen.ExpectedFactory
 import com.mapbox.navigation.base.route.NavigationRoute
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.launch
 import kotlinx.coroutines.suspendCancellableCoroutine
 import kotlin.coroutines.resume
 
+internal sealed class RoutesRefresherExecutorResult {
+
+    internal data class Finished(val value: RoutesRefresherResult) : RoutesRefresherExecutorResult()
+
+    internal object ReplacedByNewer : RoutesRefresherExecutorResult()
+}
+
 private data class QueuedRequest(
     val routes: List<NavigationRoute>,
     val startCallback: () -> Unit,
-    val finishCallback: (Expected<String, RoutesRefresherResult>) -> Unit,
+    val finishCallback: (RoutesRefresherExecutorResult) -> Unit,
 )
 
 internal class RouteRefresherExecutor(
@@ -26,7 +31,7 @@ internal class RouteRefresherExecutor(
     suspend fun executeRoutesRefresh(
         routes: List<NavigationRoute>,
         startCallback: () -> Unit,
-    ): Expected<String, RoutesRefresherResult> = suspendCancellableCoroutine { cont ->
+    ): RoutesRefresherExecutorResult = suspendCancellableCoroutine { cont ->
         cont.invokeOnCancellation {
             currentRequest = null
             queuedRequest = null
@@ -39,10 +44,10 @@ internal class RouteRefresherExecutor(
     private fun executeRoutesRefresh(
         routes: List<NavigationRoute>,
         startCallback: () -> Unit,
-        finishCallback: (Expected<String, RoutesRefresherResult>) -> Unit,
+        finishCallback: (RoutesRefresherExecutorResult) -> Unit,
     ) {
         queuedRequest?.finishCallback?.invoke(
-            ExpectedFactory.createError("Skipping request as a newer one is queued.")
+            RoutesRefresherExecutorResult.ReplacedByNewer
         )
         queuedRequest = QueuedRequest(routes, startCallback, finishCallback)
         runQueue()
@@ -60,7 +65,7 @@ internal class RouteRefresherExecutor(
                     currentRequest = null
                 }
                 runQueue()
-                localCurrentRequest.finishCallback(ExpectedFactory.createValue(result))
+                localCurrentRequest.finishCallback(RoutesRefresherExecutorResult.Finished(result))
             }
         }
     }