Don't render route component if OnNavigateAsync task in-progress

[captainsafia]

This PR adds a check to the Refresh method in the Router class to ensure that a route's component isn't rendered while an OnNavigateAsync method is running.

We check the state of the _previousOnNavigateTask global which stores a references to the currently running navigation task and only render if it ran to completion.

Addresses #24211

[SteveSandersonMS]

I was a bit confused by the removal of the try/finally here since it looks like if the RunOnNavigateAsync task fails, then the router would now get stuck permanently, because tcs would never get marked as completed, and all subsequent navigation tasks will queue up behind it.

However on close inspection it looks like we're saying RunOnNavigateAsync won't fail, because the only place where we call user code from it is wrapped in a try/catch. If that's the reasoning then totally fine! Please let me know if I'm misreading it.

[SteveSandersonMS]

TBH overall I feel that the logic in Router has got a bit out of hand and is really hard to reason about. For example, it seems as if the removal of Task.WhenAny here means that we now fully rely on the developer's callback bailing out if the cancellation token fires. If their callback chooses to ignore the token (which will usually be the case) then we'd continue to wait for it before performing subsequent navigation, which would be a bug. It's completely possible I'm misreading this, hence the concern about readability. Similarly, in this test we are checking for a flag called refreshCalled, but there's no code that ever sets that flag to true so it's unclear what the test shows.

Perhaps as part of #24215 we could take a fresh run at the whole flow inside Router and find a different way of expressing the queuing/cancellation of navigation tasks.

BTW I totally understand how we got here through a long chain of evolving changes to requirements and feedback-upon-feedback. Combined with the inherent difficulty of reasoning about concurrent tasks that makes testing hard and review feedback pretty random. It's still great that lazy loading will be in preview 8 and no criticism that we might want to take a fresh look at this one aspect of implementation!

[SteveSandersonMS]

Thanks for adding this fix! I know you're out today but hopefully @mkArtakMSFT can take care of merging it into the preview 8 branch.

[SteveSandersonMS]

@mkArtakMSFT Previously I was pushing to include this in preview 8 via ask mode, but after more consideration I think it's not necessary. This change fixes an issue that only affects people who are doing lazy loading (which is only people trying out the feature, since it's new) and even then only when they have a nonstandard routing setup where the host page might rerender independently of routing.

So in fact I'd be happy for this just to merge into master for RC1. @captainsafia I know you're out at the moment but once you're back, if you have a different opinion, please let us know!

[captainsafia]

So in fact I'd be happy for this just to merge into master for RC1. @captainsafia I know you're out at the moment but once you're back, if you have a different opinion, please let us know!

I'm fine with moving this to rc1. I'll update the target branch for this PR accordingly.

TBH overall I feel that the logic in Router has got a bit out of hand and is really hard to reason about. For example, it seems as if the removal of Task.WhenAny here means that we now fully rely on the developer's callback bailing out if the cancellation token fires.

This is correct.

If their callback chooses to ignore the token (which will usually be the case) then we'd continue to wait for it before performing subsequent navigation, which would be a bug.

My anticipation is that (given proper documentation and awareness for this), it won't happen too often. With this design, we are switching to a cooperative model of cancellation. This comes with a few benefits:

• Less complex cancellation handling on our end
• Gives user the power to throw the cancellation a point that makes sense in their callback
• Follows existing best practices in .NET

The downside that did come up is the one you mentioned about users being more likely to write bug-ridden code. Ultimately, I felt more comfortable with this model since we anticipate that users leveraging lazy-loading will be more advanced users and I don't anticipate that bugs caused by not throwing the cancellation will be as depilating for them.