Skip to content

Make OnNavigateAsync EventCallback and cancel previous navigation #25011

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
merged 2 commits into from
Aug 20, 2020
Merged

Make OnNavigateAsync EventCallback and cancel previous navigation #25011

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
39914ac
Make OnNavigateAsync EventCallback and cancel previous navigation
captainsafia Aug 18, 2020
7162fba
Add more tests
captainsafia Aug 19, 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
64 changes: 19 additions & 45 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 @@ -73,7 +73,7 @@ static readonly ReadOnlyDictionary<string, object> _emptyParametersDictionary
/// <summary>
/// Gets or sets a handler that should be called before navigating to a new page.
/// </summary>
[Parameter] public Func<NavigationContext, Task>? OnNavigateAsync { get; set; }
[Parameter] public EventCallback<NavigationContext> OnNavigateAsync { get; set; }

private RouteTable Routes { get; set; }

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

Refresh(isNavigationIntercepted: false);
Expand Down Expand Up @@ -206,9 +205,8 @@ internal virtual void Refresh(bool isNavigationIntercepted)
}
}

private async ValueTask<bool> RunOnNavigateAsync(string path, Task previousOnNavigate)
internal async ValueTask RunOnNavigateAsync(string path, bool isNavigationIntercepted)
{

// Cancel the CTS instead of disposing it, since disposing does not
// actually cancel and can cause unintended Object Disposed Exceptions.
// This effectivelly cancels the previously running task and completes it.
Expand All @@ -217,67 +215,43 @@ private async ValueTask<bool> RunOnNavigateAsync(string path, Task previousOnNav
// before starting the next one. This avoid race conditions where the cancellation
// for the previous task was set but not fully completed by the time we get to this
// invocation.
await previousOnNavigate;
await _previousOnNavigateTask;

if (OnNavigateAsync == null)
var tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
_previousOnNavigateTask = tcs.Task;

if (!OnNavigateAsync.HasDelegate)
{
return true;
Refresh(isNavigationIntercepted);
}

_onNavigateCts = new CancellationTokenSource();
var navigateContext = new NavigationContext(path, _onNavigateCts.Token);

var cancellationTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
navigateContext.CancellationToken.Register(state =>
((TaskCompletionSource)state).SetResult(), cancellationTcs);

try
{
if (Navigating != null)
{
_renderHandle.Render(Navigating);
}
await OnNavigateAsync(navigateContext);
return true;
}
catch (OperationCanceledException e)
{
if (e.CancellationToken != navigateContext.CancellationToken)
{
var rethrownException = new InvalidOperationException("OnNavigateAsync can only be cancelled via NavigateContext.CancellationToken.", e);
_renderHandle.Render(builder => ExceptionDispatchInfo.Throw(rethrownException));
}
// Task.WhenAny returns a Task<Task> so we need to await twice to unwrap the exception
var task = await Task.WhenAny(OnNavigateAsync.InvokeAsync(navigateContext), cancellationTcs.Task);
await task;
tcs.SetResult();
Refresh(isNavigationIntercepted);
}
catch (Exception e)
{
_renderHandle.Render(builder => ExceptionDispatchInfo.Throw(e));
}

return false;
}

internal async Task RunOnNavigateWithRefreshAsync(string path, bool isNavigationIntercepted)
{
// We cache the Task representing the previously invoked RunOnNavigateWithRefreshAsync
// that is stored. Then we create a new one that represents our current invocation and store it
// globally for the next invocation. This allows us to check inside `RunOnNavigateAsync` if the
// previous OnNavigateAsync task has fully completed before starting the next one.
var previousTask = _previousOnNavigateTask;
var tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
_previousOnNavigateTask = tcs.Task;

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

}

private void OnLocationChanged(object sender, LocationChangedEventArgs args)
{
_locationAbsolute = args.Location;
if (_renderHandle.IsInitialized && Routes != null)
{
_ = RunOnNavigateWithRefreshAsync(NavigationManager.ToBaseRelativePath(_locationAbsolute), args.IsNavigationIntercepted);
_ = RunOnNavigateAsync(NavigationManager.ToBaseRelativePath(_locationAbsolute), args.IsNavigationIntercepted);
}
}

Expand Down
102 changes: 26 additions & 76 deletions src/Components/Components/test/Routing/RouterTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,7 @@
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Logging.Abstractions;
using Moq;
using Xunit;
using Microsoft.AspNetCore.Components;

namespace Microsoft.AspNetCore.Components.Test.Routing
{
Expand Down Expand Up @@ -42,109 +40,61 @@ public async Task CanRunOnNavigateAsync()
{
// Arrange
var called = false;
async Task OnNavigateAsync(NavigationContext args)
Action<NavigationContext> OnNavigateAsync = async (NavigationContext args) =>
{
await Task.CompletedTask;
called = true;
}
_router.OnNavigateAsync = OnNavigateAsync;

// Act
await _renderer.Dispatcher.InvokeAsync(() => _router.RunOnNavigateWithRefreshAsync("http://example.com/jan", false));

// Assert
Assert.True(called);
}

[Fact]
public async Task CanHandleSingleFailedOnNavigateAsync()
Copy link
Member

@javiercn javiercn Aug 19, 2020

Choose a reason for hiding this comment

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

Why are we removing coverage for this? Shouldn't we still test what happens when an exception is thrown from the handler?

Copy link
Member Author

@captainsafia captainsafia Aug 19, 2020

Choose a reason for hiding this comment

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

We have an E2E test that validates this scenario here so I removed this.

Copy link
Member

@javiercn javiercn Aug 19, 2020

Choose a reason for hiding this comment

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

That's fair, for future reference I would suggest you consider keeping the unit test and the E2E test. They complement each other.

The unit test validates the component and helps catch regressions faster while the E2E test validates the integration. Figuring out a regression with a failing E2E test is much harder than with a unit test.

Copy link
Member Author

@captainsafia captainsafia Aug 19, 2020

Choose a reason for hiding this comment

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

Turns out that doing this in the E2E tests is the only option I have. There isn't a super nice way to create an EventCallback with the Router component instantiated with the tests (hence why we use new EventCallback(null, action). This exercises the execution of the async action but doesn't exercise the renderer.

Copy link
Member

@javiercn javiercn Aug 19, 2020

Choose a reason for hiding this comment

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

Hmm, I do know how to do this, but it requires a bit of changes. It involves:

  • Creating a parent component.
  • Attaching the instance to the renderer.
  • Setting properties on the parent component instance.
  • Triggering a render from the parent component.

There are a lot of samples in the Renderer tests, but it is a bit more involved.

Copy link
Member

@javiercn javiercn Aug 19, 2020

Choose a reason for hiding this comment

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

I'll let you use your best judgement here.

{
// Arrange
var called = false;
async Task OnNavigateAsync(NavigationContext args)
{
called = true;
await Task.CompletedTask;
throw new Exception("This is an uncaught exception.");
}
Copy link
Member

@javiercn javiercn Aug 19, 2020

Choose a reason for hiding this comment

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

Orthogonal to this change: We should test what happens when this throws synchronously and asynchronously. For example:

           Task OnNavigateAsync(NavigationContext args)
            {
                called = true;
                throw new Exception("This is an uncaught exception.");
                return Task.CompletedTask;
            }

Which throws synchronously VS the method above. (Which also throws synchronously, but the async machinery will transform it into a failed task).

For throwing asynchronously we actually need to go async

           async Task OnNavigateAsync(NavigationContext args)
            {
                called = true;
                await Task.Delay(1000); // Or better than this, a task completion source
                throw new Exception("This is an uncaught exception.");
            }

Copy link
Member Author

@captainsafia captainsafia Aug 19, 2020

Choose a reason for hiding this comment

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

Added an E2E test for this. E2E tests work a little bit better here because there isn't a good way to attach the event callback the router component in the unit tests.

_router.OnNavigateAsync = OnNavigateAsync;
};
_router.OnNavigateAsync = new EventCallback<NavigationContext>(null, OnNavigateAsync);

// Act
await _renderer.Dispatcher.InvokeAsync(() => _router.RunOnNavigateWithRefreshAsync("http://example.com/jan", false));
await _renderer.Dispatcher.InvokeAsync(() => _router.RunOnNavigateAsync("http://example.com/jan", false));

// Assert
Assert.True(called);
Assert.Single(_renderer.HandledExceptions);
var unhandledException = _renderer.HandledExceptions[0];
Assert.Equal("This is an uncaught exception.", unhandledException.Message);
}

[Fact]
public async Task CanceledFailedOnNavigateAsyncDoesNothing()
{
// Arrange
var onNavigateInvoked = 0;
async Task OnNavigateAsync(NavigationContext args)
Action<NavigationContext> OnNavigateAsync = async (NavigationContext args) =>
{
onNavigateInvoked += 1;
if (args.Path.EndsWith("jan"))
{
await Task.Delay(Timeout.Infinite, args.CancellationToken);
throw new Exception("This is an uncaught exception.");
}
}
var refreshCalled = false;
};
var refreshCalled = 0;
_renderer.OnUpdateDisplay = (renderBatch) =>
{
if (!refreshCalled)
{
refreshCalled = true;
return;
}
Assert.True(false, "OnUpdateDisplay called more than once.");
refreshCalled += 1;
return;
};
_router.OnNavigateAsync = OnNavigateAsync;
Copy link
Member

@javiercn javiercn Aug 19, 2020

Choose a reason for hiding this comment

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

It will be good to capture the new behavior of OnUpdateDisplay, I imagine that this has changed since we are removing it.

I would suggest we capture on an integer, the number of times it is being called and check against how many times we expect it to be called. That will help catch regressions if in the future we make a change to the implementation that results in a subtle change in the rendering sequence.

Copy link
Member Author

@captainsafia captainsafia Aug 19, 2020

Choose a reason for hiding this comment

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

Added this.

_router.OnNavigateAsync = new EventCallback<NavigationContext>(null, OnNavigateAsync);

// Act
var janTask = _renderer.Dispatcher.InvokeAsync(() => _router.RunOnNavigateWithRefreshAsync("http://example.com/jan", false));
var febTask = _renderer.Dispatcher.InvokeAsync(() => _router.RunOnNavigateWithRefreshAsync("http://example.com/feb", false));
var janTask = _renderer.Dispatcher.InvokeAsync(() => _router.RunOnNavigateAsync("http://example.com/jan", false));
var febTask = _renderer.Dispatcher.InvokeAsync(() => _router.RunOnNavigateAsync("http://example.com/feb", false));

await janTask;
await febTask;

// Assert that we render the second route component and don't throw an exception
Assert.Empty(_renderer.HandledExceptions);
Assert.Equal(2, onNavigateInvoked);
}

[Fact]
public async Task CanHandleSingleCancelledOnNavigateAsync()
{
// Arrange
async Task OnNavigateAsync(NavigationContext args)
{
var tcs = new TaskCompletionSource<int>();
tcs.TrySetCanceled();
await tcs.Task;
}
_renderer.OnUpdateDisplay = (renderBatch) => Assert.True(false, "OnUpdateDisplay called more than once.");
_router.OnNavigateAsync = OnNavigateAsync;

// Act
await _renderer.Dispatcher.InvokeAsync(() => _router.RunOnNavigateWithRefreshAsync("http://example.com/jan", false));

// Assert
Assert.Single(_renderer.HandledExceptions);
var unhandledException = _renderer.HandledExceptions[0];
Assert.Equal("OnNavigateAsync can only be cancelled via NavigateContext.CancellationToken.", unhandledException.Message);
Assert.Equal(2, refreshCalled);
}

[Fact]
public async Task AlreadyCanceledOnNavigateAsyncDoesNothing()
{
// Arrange
var triggerCancel = new TaskCompletionSource();
async Task OnNavigateAsync(NavigationContext args)
Action<NavigationContext> OnNavigateAsync = async (NavigationContext args) =>
{
if (args.Path.EndsWith("jan"))
{
Expand All @@ -153,7 +103,7 @@ async Task OnNavigateAsync(NavigationContext args)
tcs.TrySetCanceled();
await tcs.Task;
}
}
};
var refreshCalled = false;
_renderer.OnUpdateDisplay = (renderBatch) =>
{
Expand All @@ -164,11 +114,11 @@ async Task OnNavigateAsync(NavigationContext args)
}
Assert.True(false, "OnUpdateDisplay called more than once.");
};
_router.OnNavigateAsync = OnNavigateAsync;
_router.OnNavigateAsync = new EventCallback<NavigationContext>(null, OnNavigateAsync);

// Act (start the operations then await them)
var jan = _renderer.Dispatcher.InvokeAsync(() => _router.RunOnNavigateWithRefreshAsync("http://example.com/jan", false));
var feb = _renderer.Dispatcher.InvokeAsync(() => _router.RunOnNavigateWithRefreshAsync("http://example.com/feb", false));
var jan = _renderer.Dispatcher.InvokeAsync(() => _router.RunOnNavigateAsync("http://example.com/jan", false));
var feb = _renderer.Dispatcher.InvokeAsync(() => _router.RunOnNavigateAsync("http://example.com/feb", false));
triggerCancel.TrySetResult();

await jan;
Expand All @@ -180,16 +130,16 @@ public void CanCancelPreviousOnNavigateAsync()
{
// Arrange
var cancelled = "";
async Task OnNavigateAsync(NavigationContext args)
Action<NavigationContext> OnNavigateAsync = async (NavigationContext args) =>
{
await Task.CompletedTask;
args.CancellationToken.Register(() => cancelled = args.Path);
};
_router.OnNavigateAsync = OnNavigateAsync;
_router.OnNavigateAsync = new EventCallback<NavigationContext>(null, OnNavigateAsync);

// Act
_ = _router.RunOnNavigateWithRefreshAsync("jan", false);
_ = _router.RunOnNavigateWithRefreshAsync("feb", false);
_ = _router.RunOnNavigateAsync("jan", false);
_ = _router.RunOnNavigateAsync("feb", false);

// Assert
var expected = "jan";
Expand All @@ -200,7 +150,7 @@ async Task OnNavigateAsync(NavigationContext args)
public async Task RefreshesOnceOnCancelledOnNavigateAsync()
{
// Arrange
async Task OnNavigateAsync(NavigationContext args)
Action<NavigationContext> OnNavigateAsync = async (NavigationContext args) =>
{
if (args.Path.EndsWith("jan"))
{
Expand All @@ -217,11 +167,11 @@ async Task OnNavigateAsync(NavigationContext args)
}
Assert.True(false, "OnUpdateDisplay called more than once.");
};
_router.OnNavigateAsync = OnNavigateAsync;
_router.OnNavigateAsync = new EventCallback<NavigationContext>(null, OnNavigateAsync);

// Act
var jan = _renderer.Dispatcher.InvokeAsync(() => _router.RunOnNavigateWithRefreshAsync("http://example.com/jan", false));
var feb = _renderer.Dispatcher.InvokeAsync(() => _router.RunOnNavigateWithRefreshAsync("http://example.com/feb", false));
var jan = _renderer.Dispatcher.InvokeAsync(() => _router.RunOnNavigateAsync("http://example.com/jan", false));
var feb = _renderer.Dispatcher.InvokeAsync(() => _router.RunOnNavigateAsync("http://example.com/feb", false));

await jan;
await feb;
Expand Down
14 changes: 12 additions & 2 deletions src/Components/test/E2ETest/Tests/RoutingTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -570,14 +570,24 @@ public void OnNavigate_CanRenderUIForExceptions()
{
var app = Browser.MountTestComponent<TestRouterWithOnNavigate>();

// Navigating from one page to another should
// cancel the previous OnNavigate Task
SetUrlViaPushState("/Other");

var errorUiElem = Browser.Exists(By.Id("blazor-error-ui"), TimeSpan.FromSeconds(10));
Assert.NotNull(errorUiElem);
}

[Fact]
public void OnNavigate_CanRenderUIForSyncExceptions()
{
var app = Browser.MountTestComponent<TestRouterWithOnNavigate>();

// Should capture exception from synchronously thrown
SetUrlViaPushState("/WithLazyAssembly");

var errorUiElem = Browser.Exists(By.Id("blazor-error-ui"), TimeSpan.FromSeconds(10));
Assert.NotNull(errorUiElem);
}

[Fact]
public void OnNavigate_DoesNotRenderWhileOnNavigateExecuting()
{
Expand Down
13 changes: 12 additions & 1 deletion src/Components/test/testassets/BasicTestApp/RouterTest/TestRouterWithOnNavigate.razor
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,15 @@
{ "LongPage1", new Func<NavigationContext, Task>(TestLoadingPageShows) },
{ "LongPage2", new Func<NavigationContext, Task>(TestOnNavCancel) },
{ "Other", new Func<NavigationContext, Task>(TestOnNavException) },
{"WithParameters/name/Abc", new Func<NavigationContext, Task>(TestRefreshHandling)}
{ "WithLazyAssembly", new Func<NavigationContext, Task>(TestOnNavException) },
{ "WithParameters/name/Abc", new Func<NavigationContext, Task>(TestRefreshHandling) }
};

protected override void OnAfterRender(bool firstRender)
{
Console.WriteLine("Render triggered...");
}

private async Task OnNavigateAsync(NavigationContext args)
{
Console.WriteLine($"Running OnNavigate for {args.Path}...");
Expand Down Expand Up @@ -56,6 +62,11 @@
throw new Exception("This is an uncaught exception.");
}

public static Task TestOnNavSyncException(NavigationContext args)
{
throw new Exception("This is an uncaught exception.");
}

public static async Task TestRefreshHandling(NavigationContext args)
{
await Task.Delay(Timeout.Infinite, args.CancellationToken);
Expand Down