Skip to content

Spruce up async handling in OnNavigateAsync callback in Blazor router #23835

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
6 commits merged into from
Jul 15, 2020
Merged

Spruce up async handling in OnNavigateAsync callback in Blazor router #23835

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
eff9da5
Spruce up async handling in OnNavigateAsync
captainsafia Jul 9, 2020
7479d31
Apply suggestions from code review
captainsafia Jul 10, 2020
27f8e31
Ensure previous task awaited before starting next one
captainsafia Jul 10, 2020
6e12150
Apply suggestions from code review
captainsafia Jul 14, 2020
24bb293
Validate no exceptions throw on multiple invocations
captainsafia Jul 14, 2020
c2f1b37
Address feedback from peer review
captainsafia Jul 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/Components/Components/src/Properties/AssemblyInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,5 @@
[assembly: InternalsVisibleTo("Microsoft.AspNetCore.Components.Tests, PublicKey=0024000004800000940000000602000000240000525341310004000001000100f33a29044fa9d740c9b3213a93e57c84b472c84e0b8a0e1ae48e67a9f8f6de9d5f7f3d52ac23e48ac51801f1dc950abe901da34d2a9e3baadb141a17c77ef3c565dd5ee5054b91cf63bb3c6ab83f72ab3aafe93d0fc3c2348b764fafb0b1c0733de51459aeab46580384bf9d74c4e28164b7cde247f891ba07891c9d872ad2bb")]
[assembly: InternalsVisibleTo("Microsoft.AspNetCore.Components.Web.Tests, PublicKey=0024000004800000940000000602000000240000525341310004000001000100f33a29044fa9d740c9b3213a93e57c84b472c84e0b8a0e1ae48e67a9f8f6de9d5f7f3d52ac23e48ac51801f1dc950abe901da34d2a9e3baadb141a17c77ef3c565dd5ee5054b91cf63bb3c6ab83f72ab3aafe93d0fc3c2348b764fafb0b1c0733de51459aeab46580384bf9d74c4e28164b7cde247f891ba07891c9d872ad2bb")]
[assembly: InternalsVisibleTo("Microsoft.AspNetCore.Components.Web.Extensions.Tests, PublicKey=0024000004800000940000000602000000240000525341310004000001000100f33a29044fa9d740c9b3213a93e57c84b472c84e0b8a0e1ae48e67a9f8f6de9d5f7f3d52ac23e48ac51801f1dc950abe901da34d2a9e3baadb141a17c77ef3c565dd5ee5054b91cf63bb3c6ab83f72ab3aafe93d0fc3c2348b764fafb0b1c0733de51459aeab46580384bf9d74c4e28164b7cde247f891ba07891c9d872ad2bb")]

[assembly: InternalsVisibleTo("DynamicProxyGenAssembly2, PublicKey=0024000004800000940000000602000000240000525341310004000001000100c547cac37abd99c8db225ef2f6c8a3602f3b3606cc9891605d02baa56104f4cfc0734aa39b93bf7852f7d9266654753cc297e7d2edfe0bac1cdcf9f717241550e0a7b191195b7667bb4f64bcb8e2121380fd1d9d46ad2d92d2d15605093924cceaf74c4861eff62abf69b9291ed0a340e113be11e6a7d3113e92484cf7045cc7")]
52 changes: 39 additions & 13 deletions src/Components/Components/src/Routing/Router.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ static readonly ReadOnlyDictionary<string, object> _emptyParametersDictionary

private CancellationTokenSource _onNavigateCts;

private Task _previousOnNavigateTask = Task.CompletedTask;

private readonly HashSet<Assembly> _assemblies = new HashSet<Assembly>();

private bool _onNavigateCalled = false;
Expand Down Expand Up @@ -112,7 +114,8 @@ public async Task SetParametersAsync(ParameterView parameters)
if (!_onNavigateCalled)
{
_onNavigateCalled = true;
await RunOnNavigateAsync(NavigationManager.ToBaseRelativePath(_locationAbsolute));
await RunOnNavigateWithRefreshAsync(NavigationManager.ToBaseRelativePath(_locationAbsolute), isNavigationIntercepted: false);
return;
}

Refresh(isNavigationIntercepted: false);
Expand All @@ -122,7 +125,6 @@ public async Task SetParametersAsync(ParameterView parameters)
public void Dispose()
{
NavigationManager.LocationChanged -= OnLocationChanged;
_onNavigateCts?.Dispose();
}

private static string StringUntilAny(string str, char[] chars)
Expand All @@ -147,7 +149,7 @@ private void RefreshRouteTable()

}

private void Refresh(bool isNavigationIntercepted)
internal virtual void Refresh(bool isNavigationIntercepted)
{
RefreshRouteTable();

Expand Down Expand Up @@ -190,28 +192,31 @@ private void Refresh(bool isNavigationIntercepted)
}
}

private async Task RunOnNavigateAsync(string path)
private async ValueTask<bool> RunOnNavigateAsync(string path, Task previousOnNavigate)
{
// If this router instance does not provide an OnNavigateAsync parameter
// then we render the component associated with the route as per usual.
if (!OnNavigateAsync.HasDelegate)
{
return;
return true;
}

// If we've already invoked a task and stored its CTS, then
// cancel the existing task.
_onNavigateCts?.Dispose();
// cancel that existing CTS.
_onNavigateCts?.Cancel();
// Then make sure that the task has been completed cancelled or
// completed before continuing with the execution of this current task.
await previousOnNavigate;
Copy link
Member

@SteveSandersonMS SteveSandersonMS Jul 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// Create a new cancellation token source for this instance
_onNavigateCts = new CancellationTokenSource();
var navigateContext = new NavigationContext(path, _onNavigateCts.Token);

// Create a cancellation task based on the cancellation token
// associated with the current running task.
var cancellationTaskSource = new TaskCompletionSource();
var cancellationTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
navigateContext.CancellationToken.Register(state =>
((TaskCompletionSource)state).SetResult(), cancellationTaskSource);
((TaskCompletionSource)state).SetResult(), cancellationTcs);

var task = OnNavigateAsync.InvokeAsync(navigateContext);

Expand All @@ -221,13 +226,34 @@ private async Task RunOnNavigateAsync(string path)
_renderHandle.Render(Navigating);
}

await Task.WhenAny(task, cancellationTaskSource.Task);
var completedTask = await Task.WhenAny(task, cancellationTcs.Task);
return task == completedTask;
Comment on lines +229 to +230
Copy link
Member

@SteveSandersonMS SteveSandersonMS Jul 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@captainsafia captainsafia Jul 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

}

private async Task RunOnNavigateWithRefreshAsync(string path, bool isNavigationIntercepted)
internal async Task RunOnNavigateWithRefreshAsync(string path, bool isNavigationIntercepted)
{
await RunOnNavigateAsync(path);
Refresh(isNavigationIntercepted);
// We cache the Task representing the previously invoked RunOnNavigateWithRefreshAsync
// that is stored
var previousTask = _previousOnNavigateTask;
// Then we create a new one that represents our current invocation and store it
// globally for the next invocation. Note to the developer, if the WASM runtime
// support multi-threading then we'll need to implement the appropriate locks
// here to ensure that the cached previous task is overwritten incorrectly.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

var tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
_previousOnNavigateTask = tcs.Task;
try
{
// And pass an indicator for the previous task to the currently running one.
var shouldRefresh = await RunOnNavigateAsync(path, previousTask);
if (shouldRefresh)
{
Refresh(isNavigationIntercepted);
}
}
finally
{
tcs.SetResult();
}
}

private void OnLocationChanged(object sender, LocationChangedEventArgs args)
Expand Down
104 changes: 104 additions & 0 deletions src/Components/Components/test/Routing/RouterTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Reflection;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Components;
using Microsoft.AspNetCore.Components.Routing;
using Microsoft.AspNetCore.Components.Test.Helpers;
using Microsoft.Extensions.DependencyModel;
using Moq;
using Xunit;

namespace Microsoft.AspNetCore.Components.Test.Routing
{
public class RouterTest
{
[Fact]
public async Task CanRunOnNavigateAsync()
{
// Arrange
var router = CreateMockRouter();
var called = false;
async Task OnNavigateAsync(NavigationContext args)
{
await Task.CompletedTask;
called = true;
}
router.Object.OnNavigateAsync = new EventCallbackFactory().Create<NavigationContext>(router, OnNavigateAsync);

// Act
await router.Object.RunOnNavigateWithRefreshAsync("http://example.com/jan", false);

// Assert
Assert.True(called);
}

[Fact]
public async Task CanCancelPreviousOnNavigateAsync()
{
// Arrange
var router = CreateMockRouter();
var cancelled = "";
async Task OnNavigateAsync(NavigationContext args)
{
await Task.CompletedTask;
args.CancellationToken.Register(() => cancelled = args.Path);
};
router.Object.OnNavigateAsync = new EventCallbackFactory().Create<NavigationContext>(router, OnNavigateAsync);

// Act
await router.Object.RunOnNavigateWithRefreshAsync("jan", false);
await router.Object.RunOnNavigateWithRefreshAsync("feb", false);

// Assert
var expected = "jan";
Assert.Equal(cancelled, expected);
}

[Fact]
public async Task RefreshesOnceOnCancelledOnNavigateAsync()
{
// Arrange
var router = CreateMockRouter();
async Task OnNavigateAsync(NavigationContext args)
{
if (args.Path.EndsWith("jan"))
{
await Task.Delay(Timeout.Infinite);
}
};
router.Object.OnNavigateAsync = new EventCallbackFactory().Create<NavigationContext>(router, OnNavigateAsync);

// Act
var janTask = router.Object.RunOnNavigateWithRefreshAsync("jan", false);
var febTask = router.Object.RunOnNavigateWithRefreshAsync("feb", false);

var janTaskException = await Record.ExceptionAsync(() => janTask);
var febTaskException = await Record.ExceptionAsync(() => febTask);

// Assert neither exceution threw an exception
Assert.Null(janTaskException);
Assert.Null(febTaskException);
// Assert refresh should've only been called once for the second route
router.Verify(x => x.Refresh(false), Times.Once());
}

Copy link
Member

@SteveSandersonMS SteveSandersonMS Jul 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

private Mock<Router> CreateMockRouter()
{
var router = new Mock<Router>() { CallBase = true };
router.Setup(x => x.Refresh(It.IsAny<bool>())).Verifiable();
return router;
}

[Route("jan")]
private class JanComponent : ComponentBase { }

[Route("feb")]
private class FebComponent : ComponentBase { }
}
}