Fix routeProgress get out of sync with NavNative status

[S2Ler]

• Probably addresses some symptoms of Parallel copies of route in Router can get out of sync #3343

• Fixes Fix and enable NavigationServiceTests.  #3375

• Fixes Crash: RouteController.updateIndexes(status:progress:)  #3298

• Fixes Restore disabled test #3128

• Fixes [Bug]: Crashed navigation session. Precondition failed: The stepIndex is higher than steps count #3461

• NavigationService.updateRoute, RouteController.updateRoute is no
  longer synchronous. This change is needed because NavNative
  implementation is asynchronous, and we need to know when this operation
  completes so that we can update local properties with a new route.
  Otherwise, we can end up in a situation when the SDK state doesn't match
  with the nav status callback received from NavNative.

• userIsOnRoute(_:status:) incorrectly assumes that the invalid route
  state is an off-route event. The invalid route state is reported when
  there is no route set in a native navigator, that happens during passive
  navigation.

• It is no longer possible to set RouteController.routeProgress
  property synchronously for the same reason as why
  RouteController.updateRoute is asynchronous.

• updateNavigator incorrectly referenced old routeProgress in the
  parameters to NavNative's setRoute method.

• RouteProgress.legIndexHandler removed as it is no longer needed.
  RouteProgress.legIndex should be updated only after NavNative status
  callback, otherwise they will be out-of-sync.
  RouteController.advanceLegIndex is the correct method to change
  legIndex, then it will be updated in RouteProgress.

• NavigationEventsManagerTestDoubles.flush method was adjusted to
  work closer to real world. Without clearing enqueuedEvents, if flus is
  called two times, flushed events will contain two duplicates.

• A lot of RouteController dependent tests are fixed, some restored.

Next:

• Update comments.
• Update documentation.
• Update inline documentation.
• Update changelog.

[1ec5]

Wow, this is quite a PR. Can you say a bit more about what fixed #3343 and #3298?

[S2Ler]

Wow, this is quite a PR. Can you say a bit more about what fixed #3343 and #3298?

• Parallel copies of route in Router can get out of sync #3343 I read this issue once again and it won't fix this particular issue, but probably fixes some symptoms.
• Crash: RouteController.updateIndexes(status:progress:)  #3298 I've see similar crashes in test. That helped me to find the route causes of these crashes. There are two main issues addressed:

  • mapbox-navigation-ios/Sources/MapboxCoreNavigation/RouteController.swift

    Lines 186 to 202 in 5382dc0

    	private func updateNavigator(with progress: RouteProgress) {
    	let encoder = JSONEncoder()
    	encoder.userInfo[.options] = progress.routeOptions
    	guard let routeData = try? encoder.encode(progress.route),
    	let routeJSONString = String(data: routeData, encoding: .utf8) else {
    	return
    	}
    	let routeRequest = Directions().url(forCalculating: routeProgress.routeOptions).absoluteString
    	navigator.setRouteForRouteResponse(routeJSONString,
    	route: 0,
    	leg: UInt32(routeProgress.legIndex),
    	routeRequest: routeRequest) { result in
    	// No-op
    	}
    	}

    incorrectly referenced old routeProgress in the parameters to NavNative's setRoute method.

    mapbox-navigation-ios/Sources/MapboxCoreNavigation/RouteController.swift

    Line 194 in 5382dc0

    let routeRequest = Directions().url(forCalculating: routeProgress.routeOptions).absoluteString

    mapbox-navigation-ios/Sources/MapboxCoreNavigation/RouteController.swift

    Line 198 in 5382dc0

    leg: UInt32(routeProgress.legIndex),

  • During reroute we synchronously modify local routeProgress and then we receive a navigator status from the old route and crash on index inconsistencies.

[bamx23]

Should we discuss this and remove the TODO?

[S2Ler]

It is here for a long time, who can verify it?

[1ec5]

In a bring-your-own-route workflow, the developer would have to supply a RouteOptions that would simulate the RouteResponse converted from the MapMatchingResponse. The workflow also currently requires the developer to preempt rerouting and set the new route manually (using the method that’s becoming asynchronous in this PR). #3464 tracks streamlining this workflow. We had been getting lucky, because #3191 had been morphing MapMatchingOptions into RouteOptions. But now that that issue is fixed, it may be a little easier for us to get into this situation.

[S2Ler]

@1ec5 You seem to have a good grasp of this issue. Can you suggest what shall we do when we hit this code path?

[1ec5]

Whew! I mostly focused on whatever had outstanding questions, as well as the public API, but I didn’t look very closely at the test changes.

[1ec5]

In a bring-your-own-route workflow, the developer would have to supply a RouteOptions that would simulate the RouteResponse converted from the MapMatchingResponse. The workflow also currently requires the developer to preempt rerouting and set the new route manually (using the method that’s becoming asynchronous in this PR). #3464 tracks streamlining this workflow. We had been getting lucky, because #3191 had been morphing MapMatchingOptions into RouteOptions. But now that that issue is fixed, it may be a little easier for us to get into this situation.