Spruce up async handling in OnNavigateAsync callback in Blazor router

[captainsafia]

This PR introduces some modifications to improve handling of async operations and cancellations in OnNavigateAsync. The changes included:

• Use Cancel instead of Dispose on the _onNavigateCts inside OnNavigateAsync. Dispose does not emit a cancellation and simple breaks down registrations on the token source.
• Remove the _onNavigate?.Dispose call in the Dispose method of the router. Disposing of the cancellation token source isn't meaningful in our context and introduces more headaches than benefit. The Dispose method on the CTS disposes of timers, wait handles, and linked tokens on a CTS. Since we don't use any of these in our code, we don't need to dispose. This way, we don't have to worry about the unintended consequences of trying to cancel a disposed token.
• Adds the TaskCreationOptions.RunContinuationsAsynchronously constructor parameter to the TaskCompletionSource that observes the cancellation token. Without this option, the cancellationTcs continuation will run inline when triggered. When we visit Route A, then Route B, the cancellation of the CTS that occurs in the second invocation of OnNavigateAsync will continue running the rest of A's OnNavigateAsync and triggering a Refresh.
• Checks to see if a task was completed or cancelled to avoid extraneously rendering a route's component if the OnNavigate was cancelled.
• Checks to see if the completion or cancellation of the previous task was fully finalized before starting the next OnNavigateAsync task.
• Adds unit tests for OnNavigateAsync and changes a few modifiers on Router to support testing.

Addresses #23789

[mrpmorris]

Did you see my comment on the last PR about naming it OnNavigatedAsync?

I'm not looking for a response, just want to ensure it wasn't missed.

[captainsafia]

Did you see my comment on the last PR about naming it OnNavigatedAsync?

I did see the comment but haven't thought much about OnNavigateAsync vs OnNavigatedAsync yet.

[ghost]

Hello @captainsafia!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

[javiercn]

I might be wrong here, but if we were to support multi-threading we would still be single threaded on the UI, since changing that would be a massive breaking change of epic proportions. We would likely switch to use a synchronization context in the same way we do on the server.

[SteveSandersonMS]

I'm sure this await previousOnNavigate is for a good reason, but it seems subtle. I can see that due to the chain of events triggered by L206, the previous task will immediately become cancelled completed, so the await on L209 shouldn't have to really wait.

So it makes me wonder why we couldn't just ignore previousOnNavigate, since we know it will have been cancelled completed and that its continuation is going to no-op (not refresh the UI).

Also I spent a while being confused about error handling here, because I thought that if previousOnNavigate had failed or been cancelled, we'd be blocking all subsequent navigation because we're awaiting it. Then later I realised that previousOnNavigate can never fail or be cancelled because it's entirely managed within RunOnNavigateWithRefreshAsync, which only ever sets it to completed.

So, it would be good to clarify in the comment why we want to await this, and why we know it will never fail or be cancelled :)

[halter73]

I recommended this when reviewing the the CTS usage. I didn't want to have to think about the possible consequences of the meat of RunOnNavigateWithRefreshAsync running concurrently of itself, so in my mind this simplified things. I cannot point to any specific bug this prevents though.

[captainsafia]

@halter73 Thanks for posting this response. I also updated the commenting around here in a follow-up PR to make ti clearer.

[SteveSandersonMS]

I'm unclear about this logic.

• If the developer's task is cancelled by them, is the intended behavior that we treat it the same as if it resolved as success? That is, we do perform the UI refresh. That appears to be the behavior here, and I suspect that's the best choice because we don't want to leave things in a "loading" state, and we don't have a way to truly cancel navigation and put the URL back to what it was before.
  • The other option I'd consider is maybe throwing if the developer's own task was cancelled (and _onNavigateCts.Token was not). Because maybe that signals their intent to cancel navigation, and we don't support that. Throwing would leave us the option to add support for navigation cancellation in the future, whereas if we don't, then we couldn't use task cancellation as the signal for it.
• But, how do we know it was cancelled by them? Inside their OnNavigateAsync callback, they may well be using the cancellation token we gave them. So if that token fires, their returned task may be immediately cancelled as a result. If that happens it seems like we have a race condition: either task or cancellationTcs.Task might happen to resolve first, since they were both triggered by the same underlying _onNavigateCts.Token. It makes me wonder if L230 really should say return !_onNavigateCts.Token.IsCancellationRequested or something like that.
• Also, if the developer's task resolves as faulted, then the logic here will disregard that fault and treat it the same as completing successfully. It seems like instead we should be throwing the exception upstream. I know that we don't yet have a way to truly pass it back into the renderer, but as it stands we're discarding the exception in two places (here on L230 and also on L264). Shouldn't we be throwing here so the fault goes to one place (L264) where we can do something useful with it later?

[captainsafia]

If the developer's task is cancelled by them, is the intended behavior that we treat it the same as if it resolved as success?

This is a correct. If the user cancels the task themselves, we'll still render the route's component. The only time we don't render the route's component is if the OnNavigateAsync was cancelled because another one was initiated. This effectively stops us from double rendering when a user quickly navigates between two routes that both have an OnNavigateAsync.

The other option I'd consider is maybe throwing if the developer's own task was cancelled (and _onNavigateCts.Token was not). Because maybe that signals their intent to cancel navigation, and we don't support that. Throwing would leave us the option to add support for navigation cancellation in the future, whereas if we don't, then we couldn't use task cancellation as the signal for it.

It should be pretty easy to check this. We would have to do something like if (completedTask.IsCancelled && !_onNavigateCts.Token.IsCancellationRequested) { throw InvalidOperationException(...); }.

But, how do we know it was cancelled by them?

We can do task.IsCancelled to validate this.

Inside their OnNavigateAsync callback, they may well be using the cancellation token we gave them. So if that token fires, their returned task may be immediately cancelled as a result. If that happens it seems like we have a race condition: either task or cancellationTcs.Task might happen to resolve first, since they were both triggered by the same underlying _onNavigateCts.Token. It makes me wonder if L230 really should say return !_onNavigateCts.Token.IsCancellationRequested or something like that.

Maybe the more full-proof check is an adaptation of the one above (completedTask.IsCancelled || _onNavigateCts.Token.IsCancellationRequested) -- so if the task is cancelled by the user or via the token, then don't bother rendering the component's route.

Also, if the developer's task resolves as faulted, then the logic here will disregard that fault and treat it the same as completing successfully. It seems like instead we should be throwing the exception upstream. I know that we don't yet have a way to truly pass it back into the renderer, but as it stands we're discarding the exception in two places (here on L230 and also on L264). Shouldn't we be throwing here so the fault goes to one place (L264) where we can do something useful with it later?

The whole "do something when a task throws an exception) is addressed in my other PR. You make a good point about the logic that throws the exception back to the renderer being the "OnNavigateAsyncWithRefresh" method instead of the RunOnNavigateAsync method.

Edit: Actually, we can keep the IsFaulted check in the same place that it is in the other PR (inside RunOnNavigateAsync).

[SteveSandersonMS]

It looks like you have well covered the cases where cancellation occurs due to a second navigation.

I'd love to see some additional test cases covering scenarios where the OnNavigateAsync task itself is what completes first, in success, failure, and cancelled status. For example:

• OnNavigateAsync task fails
  • While it's the activate navigation (I presume the desired behavior for RunOnNavigateAsync is to throw, but it doesn't currently)
  • After some other navigation has started (I presume the intended behavior is to ignore it, since we're not waiting for that task any more)
• OnNavigateAsync task is cancelled
  • While it's the activate navigation (see discussion above about how we might choose to handle that)
  • After some other navigation has started (I presume the intended behavior is to ignore it, since we're not waiting for that task any more)

[captainsafia]

I'd love to see some additional test cases covering scenarios where the OnNavigateAsync task itself is what completes first, in success, failure, and cancelled status.

These are good ideas. I'll add these scenarios as unit tests in the other PR.

[SteveSandersonMS]

@captainsafia Sorry these comments are after you merged. It was totally reasonable to merge at the time you did. But it would be great if we could follow up one the remaining points! Hopefully we might be able to simplify/eliminate the workload you were taking on in #23947 to make up for it :)