NAVAND-777: add route refresh on demand #6610
Conversation
dzinad
force-pushed
the
NAVAND-777-dd
branch
from
4f2b275 to
24954a3
Compare
Codecov Report
@@ Coverage Diff @@
## main #6610 +/- ##
============================================
+ Coverage 72.68% 72.95% +0.26%
- Complexity 5576 5633 +57
============================================
Files 783 793 +10
Lines 30186 30399 +213
Branches 3571 3581 +10
============================================
+ Hits 21941 22177 +236
+ Misses 6819 6798 -21
+ Partials 1426 1424 -2
|
dzinad
force-pushed
the
NAVAND-777-dd
branch
3 times, most recently
from
b855099 to
da27db1
Compare
...ndroidTest/java/com/mapbox/navigation/instrumentation_tests/core/RouteRefreshOnDemandTest.kt
Outdated
Show resolved
Hide resolved
...ndroidTest/java/com/mapbox/navigation/instrumentation_tests/core/RouteRefreshOnDemandTest.kt
Outdated
Show resolved
Hide resolved
| val requestedRoutes = requestRoutes(enableRefresh = false) | ||
| mapboxNavigation.setNavigationRoutes(requestedRoutes) | ||
|
|
||
| delay(7000) // refresh interval + accuracy |
There was a problem hiding this comment.
Same as above, these can really add up to the execution time. Wouldn't we be able to cover these granular cases with unit tests instead? Why are we preferring an instrumentation tests here?
There was a problem hiding this comment.
I don't think it can be tested properly in unit tests. They test many parts of the SDK along with the server interaction. While writing these tests I've discovered a couple of bugs and inconsistencies. What's more, they give you a feel of how the SDK works which might not have been so obvious before writing/reading them. I find them very useful and would like to keep them.
What exactly is the problem with additional 2 mins? Aren't the tests paralleled somehow?
There was a problem hiding this comment.
This is 2 minutes for a single test class. As we keep expanding at some point these types of granular addition will just unbearably extend the CI execution time. We're not at the critical point for the instrumentation tests job yet but we should be mindful of that as we go. If we can trim the execution down (for example, 7 seconds of a delay really shouldn't be necessary), let's do that.
Aren't the tests paralleled somehow?
No, we're not doing test sharding yet but that idea has been floated in the past. Something to consider cc @SevaZhukov @dchernousov.
There was a problem hiding this comment.
I just mean that we shouldn't be held back from writing tests because they require some time to execute... In order not to be flaky at the same time. I don't know how to reduce the delay here:
- I want to check states triggered by refresh on demand;
- in order to do that I have to trigger refresh in-between planned attempts;
- I have to wait for the first planned attempt;
If I make the interval 2-3 seconds instead of 5, code execution time will play a much bigger role in this test being stable.
In general, it's a common problem for tests that check that something is done with correct interval. And I think we should have such tests. For example, it would check that we don't do by mistake any unnecessary requests and so on.
There was a problem hiding this comment.
I just mean that we shouldn't be held back from writing tests because they require some time to execute...
Of course not, test coverage always prevails over test execution time. I just want us to be mindful, that's all. If you think that we can't optimize execution time any further without impacting stability, then let's leave this as is.
There was a problem hiding this comment.
I redid some of the tests into Robolectric tests.
...c/androidTest/java/com/mapbox/navigation/instrumentation_tests/core/RouteRefreshStateTest.kt
Outdated
Show resolved
Hide resolved
...c/androidTest/java/com/mapbox/navigation/instrumentation_tests/core/RouteRefreshStateTest.kt
Outdated
Show resolved
Hide resolved
libnavigation-core/src/main/java/com/mapbox/navigation/core/routerefresh/RouteRefresher.kt
Outdated
Show resolved
Hide resolved
libnavigation-core/src/main/java/com/mapbox/navigation/core/routerefresh/RouteRefresher.kt
Outdated
Show resolved
Hide resolved
...igation-core/src/main/java/com/mapbox/navigation/core/routerefresh/RouteRefresherExecutor.kt
Outdated
Show resolved
Hide resolved
| finishRequest(result) | ||
| startRequest() | ||
|
|
||
| verify(exactly = 0) { stateHolder.onStarted() } |
There was a problem hiding this comment.
I understand the retry shouldn't call start again but here it looks like we didn't call start at all.
There was a problem hiding this comment.
That's because I did finishRequest without startRequest. First startRequest is not required for the unit test.
...e/src/test/java/com/mapbox/navigation/core/routerefresh/PlannedRouteRefreshControllerTest.kt
Outdated
Show resolved
Hide resolved
dzinad
force-pushed
the
NAVAND-777-dd
branch
from
9f4bae6 to
edc11f2
Compare
dzinad
force-pushed
the
NAVAND-777-dd
branch
3 times, most recently
from
3b62d7b to
b606eb6
Compare
dzinad
force-pushed
the
NAVAND-777-dd
branch
2 times, most recently
from
3c0a44a to
a70bb9b
Compare
...ndroidTest/java/com/mapbox/navigation/instrumentation_tests/core/RouteRefreshOnDemandTest.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I'm very biased about route refresh system as I'm the one who migrated it to coroutines. I have a suggestion of how to achieve the same result writing less code. See the diff. @dzinad , do you see any refresh cases which the diff doesn't cover?
I must admit that new route refresh system may be easier and more comfortable for not biased developers(not the ones who implemented new or current route refresh system).
@LukasPaczos, @RingerJK, which way do you think should we go, modifying existing coroutine based system or switching to the new presented in this PR?
Index: libnavigation-core/src/test/java/com/mapbox/navigation/core/routerefresh/RouteRefreshControllerTest.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/libnavigation-core/src/test/java/com/mapbox/navigation/core/routerefresh/RouteRefreshControllerTest.kt b/libnavigation-core/src/test/java/com/mapbox/navigation/core/routerefresh/RouteRefreshControllerTest.kt
--- a/libnavigation-core/src/test/java/com/mapbox/navigation/core/routerefresh/RouteRefreshControllerTest.kt (revision 592818bbba6bf65accf7a4082311e94ebf938efe)
+++ b/libnavigation-core/src/test/java/com/mapbox/navigation/core/routerefresh/RouteRefreshControllerTest.kt (date 1669824835092)
@@ -129,6 +129,27 @@
}
}
+ @Test
+ fun `route refreshes immediately`() =
+ runBlockingTest {
+ val (initialRoute, refreshedRoute) = createTestInitialAndRefreshedTestRoutes()
+ val routeRefreshStub = RouteRefreshStub().apply {
+ setRefreshedRoute(refreshedRoute)
+ }
+ val routeRefreshController = createRouteRefreshController(
+ routeRefresh = routeRefreshStub,
+ routeRefreshOptions = RouteRefreshOptions.Builder()
+ .intervalMillis(30_000)
+ .build(),
+ )
+
+ val refreshJob = async { routeRefreshController.refresh(listOf(initialRoute)) }
+ assertTrue(refreshJob.isActive)
+ routeRefreshController.refreshImmediately()
+ assertEquals(listOf(refreshedRoute), refreshJob.getCompletedTest().routes)
+ refreshJob.cancel()
+ }
+
@Test
fun `should refresh route with any annotation`() = runBlockingTest {
val routeWithoutAnnotations = createNavigationRoute(
Index: libnavigation-core/src/main/java/com/mapbox/navigation/core/routerefresh/RouteRefreshController.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/libnavigation-core/src/main/java/com/mapbox/navigation/core/routerefresh/RouteRefreshController.kt b/libnavigation-core/src/main/java/com/mapbox/navigation/core/routerefresh/RouteRefreshController.kt
--- a/libnavigation-core/src/main/java/com/mapbox/navigation/core/routerefresh/RouteRefreshController.kt (revision 592818bbba6bf65accf7a4082311e94ebf938efe)
+++ b/libnavigation-core/src/main/java/com/mapbox/navigation/core/routerefresh/RouteRefreshController.kt (date 1669825162219)
@@ -15,14 +15,21 @@
import com.mapbox.navigation.core.RouteProgressDataProvider
import com.mapbox.navigation.core.directions.session.RouteRefresh
import com.mapbox.navigation.core.ev.EVRefreshDataProvider
+import com.mapbox.navigation.utils.internal.logD
import com.mapbox.navigation.utils.internal.logE
import com.mapbox.navigation.utils.internal.logI
+import com.mapbox.navigation.utils.internal.logW
import kotlinx.coroutines.CancellationException
import kotlinx.coroutines.CompletableDeferred
+import kotlinx.coroutines.Deferred
import kotlinx.coroutines.async
import kotlinx.coroutines.awaitAll
+import kotlinx.coroutines.cancelChildren
+import kotlinx.coroutines.channels.Channel
+import kotlinx.coroutines.channels.Channel.Factory.RENDEZVOUS
import kotlinx.coroutines.coroutineScope
import kotlinx.coroutines.delay
+import kotlinx.coroutines.selects.select
import kotlinx.coroutines.suspendCancellableCoroutine
import kotlinx.coroutines.withTimeoutOrNull
import java.util.Date
@@ -43,6 +50,8 @@
private val localDateProvider: () -> Date,
) {
+ private val immediateRefreshRequestsChannel = Channel<Unit>(RENDEZVOUS)
+
private var state: RouteRefreshStateResult? = null
set(value) {
if (field == value) return
@@ -92,6 +101,12 @@
}
}
+ fun refreshImmediately() {
+ if (immediateRefreshRequestsChannel.trySend(Unit).isFailure) {
+ logW { "immediate refresh is ignored as route refresh is in progress already" }
+ }
+ }
+
fun registerRouteRefreshStateObserver(observer: RouteRefreshStatesObserver) {
observers.add(observer)
state?.let { observer.onNewState(it) }
@@ -150,7 +165,8 @@
var timeUntilNextAttempt = async { delay(routeRefreshOptions.intervalMillis) }
try {
repeat(FAILED_ATTEMPTS_TO_INVALIDATE_EXPIRING_DATA) {
- timeUntilNextAttempt.await()
+ waitForIntervalOrImmediateRefreshSignal(timeUntilNextAttempt)
+
if (it == 0) {
onNewState(RouteRefreshExtra.REFRESH_STATE_STARTED)
}
@@ -180,6 +196,25 @@
)
}
+ // inspired by https://github.com/Kotlin/kotlinx.coroutines/issues/2867#issuecomment-998553524
+ private suspend fun waitForIntervalOrImmediateRefreshSignal(
+ timeUntilNextAttempt: Deferred<Unit>
+ ) {
+ coroutineScope {
+ select<Unit> {
+ timeUntilNextAttempt.onAwait {
+ logD { "starting refresh by interval" }
+ }
+ immediateRefreshRequestsChannel.onReceive {
+ logD { "starting immediate refresh" }
+ }
+ }.also {
+ timeUntilNextAttempt.cancel()
+ coroutineContext.cancelChildren()
+ }
+ }
+ }
+
private fun removeExpiringDataFromRoute(
route: NavigationRoute,
currentLegIndex: Int,
@@ -344,4 +379,4 @@
data class Success(val route: NavigationRoute) : RouteRefreshResult()
data class Fail(val error: NavigationRouterRefreshError) : RouteRefreshResult()
}
-}
+}
\ No newline at end of file
|
Just rebasing, don't pay attention ^ |
|
@VysotskiVadim In general, Im not against using coroutines here. But I found |
|
@dzinad , I'm not agains your implementation. But I'm a biased author of the old coroutines-based implementation and for me it seems so much simpler and understandable. Code is always more understandable for the authors as the code represents the way authors see their features. You see route refresh a different way, and I'm sure you find your implementation to be much understandable then the old one. That's why it's very interesting for me which way @LukasPaczos and @RingerJK find the more simple and understandable. I think their opinion won't be as biased as mine. |
dzinad
force-pushed
the
NAVAND-777-dd
branch
from
90a3ab4 to
a737c6b
Compare
| stateHolder.onSuccess() | ||
| } else { | ||
| stateHolder.onFailure(null) | ||
| plannedRefreshController.resume() |
There was a problem hiding this comment.
why it's not resume(d)() on success result?
There was a problem hiding this comment.
Because success result means new refreshed routes -> we don't have to resume refreshing the old ones. We'll get a notification with reason = REFRESH and relaunch planned refresh.
| } | ||
| } | ||
|
|
||
| override fun pause() { |
There was a problem hiding this comment.
the API is a bit tricky: if we have public (public internally) pause-resume API, it should contain a counter of invoked pause-resume, because invoking unred the hood from different sources
pause()
pause()
resume()actually does not work correctly.
There was a problem hiding this comment.
Not sure I'm getting you.
it should contain a counter of invoked pause-resume
It's not like this but it has a boolean flag to check if the state is correct (paused for resume and resumed for pause). If you invoke pause, pause, resume, you'll get 1 pause and 1 resume (the second pause will have no effect). I think it's a valid scheme. We use the same approach for starting a trip session, for example.
|
|
||
| fun postDelayed(timeout: Long, block: Runnable, cancellationCallback: () -> Unit) { | ||
| val job = scope.launch { | ||
| delay(timeout) |
There was a problem hiding this comment.
nit: if delay would be 0(or negative?): doesn't it invoked in sync? It might affect invokeOnCompletion logic
There was a problem hiding this comment.
It's OK. From the docs:
Registers handler that is synchronously invoked once on completion of this job. When the job is already complete, then the handler is immediately invoked with the job's exception or cancellation cause or null. Otherwise, the handler will be invoked once when this job is complete.
I'll add tests for this.
...igation-core/src/main/java/com/mapbox/navigation/core/routerefresh/RouteRefreshController.kt
Outdated
Show resolved
Hide resolved
| message: String? = null | ||
| ) { | ||
| val oldState = this.state?.state | ||
| if (oldState != state && RouteRefreshStateChanger.canChange(from = oldState, to = state)) { |
There was a problem hiding this comment.
@dzinad not sure that I follow this. Seems like the function might prevent the update state. Could you describe why does it need?
There was a problem hiding this comment.
Yes, it can.
It's just the generalization of what we have on main. But there we only switch to CANCELED if we are in STARTED state. Here I introduced the table for all the states.
ChangelogFeatures
Bug fixes and improvements
Known issues
|
|
@LukasPaczos I addressed your comments. |
| @@ -0,0 +1,18 @@ | |||
| - Added `RouteRefreshController` interface to manage route refreshes. Retrieve it via `MapboxNavigation#routeRefreshController`. | |||
There was a problem hiding this comment.
Does exposing RouteRefreshController makes our SDK easier to harder to change?
I found myself in a complex situations with reroute logic a few times. It was very hard to change its structure, because we exposed it to the users in our API. Maybe exposing MapboxNavigation#refreshRoute() leaves us more flexibility for future changes? 🤔
There was a problem hiding this comment.
It's just an interface that accumulates refresh-related API.
...igation-core/src/main/java/com/mapbox/navigation/core/routerefresh/RouteRefreshController.kt
Outdated
Show resolved
Hide resolved
| result.refreshedRoutesData | ||
| ) | ||
| stateHolder.onClearedExpired() | ||
| if (result.refreshedRoutesData != newRoutesData) { |
There was a problem hiding this comment.
@VysotskiVadim I can't move this check to ObserversManager because here I compare the original data with the data we got after removing the expired part. I don't think that the ObserversManager should know anything about that.
dzinad
force-pushed
the
NAVAND-777-dd
branch
from
bff9495 to
36d17f0
Compare
...c/androidTest/java/com/mapbox/navigation/instrumentation_tests/core/RouteRefreshStateTest.kt
Outdated
Show resolved
Hide resolved
libnavigation-core/src/main/java/com/mapbox/navigation/core/routerefresh/CancellableHandler.kt
Outdated
Show resolved
Hide resolved
...-core/src/main/java/com/mapbox/navigation/core/routerefresh/PlannedRouteRefreshController.kt
Outdated
Show resolved
Hide resolved
| - Added `RouteRefreshController#requestImmediateRouteRefresh` to trigger route refresh request immediately. | ||
| - Moved `MapboxNavigation#registerRouteRefreshStateObserver` to `RouteRefreshController#registerRouteRefreshStateObserver`. To migrate, change: | ||
| ```kotlin | ||
| mapboxNavigation.registerRouteRefreshStateObserver(observer) |
There was a problem hiding this comment.
Maybe it makes sense to leave mapboxNavigation.registerRouteRefreshStateObserver for the time being and mark as deprecated to avoid breaking the API? I know, it's experimental, but if the price of keeping it is low, why not to leave it for a few more releases to leave customers a time window to migrate.
There was a problem hiding this comment.
Idk, I'd like to avoid duplicating API. What's more, we will probably forget to delete it. And when it's not experimental anymore, it won't be possible.
I know we've already removed experimental API (for Drop-In UI) with migration guide in the CHANGELOG.
| every { | ||
| localDateProvider() | ||
| } returns parseISO8601DateToLocalTimeOrNull("2022-06-30T20:00:00Z")!! |
There was a problem hiding this comment.
Is that simple API worth using external mock library? 🙂
| every { | |
| localDateProvider() | |
| } returns parseISO8601DateToLocalTimeOrNull("2022-06-30T20:00:00Z")!! | |
| val testDateProvider = { parseISO8601DateToLocalTimeOrNull("2022-06-30T20:00:00Z")!! } |
There was a problem hiding this comment.
it's probably more of the matter or preferences though
There was a problem hiding this comment.
Well, here I set the time it should return in the test (because the test holds the context of what time we should return), while the object is created earlier. Since there is only one test, I can move the object creation into the test, but I'm not sure if it will make a lot of sense. If I add another test in the future, I may want different time.
|
@VysotskiVadim I'm gonna merge it into alpha. If you find there is something important I haven't fixed here, I can always follow-up in a separate PR. |
Description
The main idea is that we have 2 types of refresh now: planned (once in refreshInterval) and on-demand (the user triggers the refresh immediately). The planned refresh was present before.
RouteRefreshControllercoordinates these 2 types of refresh. Don't look at the diff, see it as a new file. It changed tremendously.PlannedRefreshControlleris responsible for planned refresh.ImmediateRefreshControlleris responsible for on-demand refresh.RouteRefresherexecutes one refresh request.RouteRefresherExecutormakes sure only one refresh request is executed at a time and if the queue is too big, only the current request and the last one from the queue are executed, the intermediate ones are ignored.CancellableHandleris responsible for posting tasks after a timeout and cancelling them on-demand, also notifying the tasks that they were cancelled.RouteRefreshStateHolderis responsible for receiving and keeping route refresh state updates and notifying observers registered viaMapboxNavigation#registerRouteRefreshStateObserver.The previous flow (with only planned refresh):
The new flow (on-demand + planned):