From 39914ac403391d6ab43b7b6d6836c913bd9d09d6 Mon Sep 17 00:00:00 2001 From: Safia Abdalla Date: Tue, 18 Aug 2020 11:40:48 -0700 Subject: [PATCH 1/2] Make OnNavigateAsync EventCallback and cancel previous navigation --- .../Components/src/Routing/Router.cs | 64 ++++-------- .../Components/test/Routing/RouterTest.cs | 97 ++++--------------- 2 files changed, 40 insertions(+), 121 deletions(-) diff --git a/src/Components/Components/src/Routing/Router.cs b/src/Components/Components/src/Routing/Router.cs index 8470101904a9..569a7061b182 100644 --- a/src/Components/Components/src/Routing/Router.cs +++ b/src/Components/Components/src/Routing/Router.cs @@ -73,7 +73,7 @@ static readonly ReadOnlyDictionary _emptyParametersDictionary /// /// Gets or sets a handler that should be called before navigating to a new page. /// - [Parameter] public Func? OnNavigateAsync { get; set; } + [Parameter] public EventCallback OnNavigateAsync { get; set; } private RouteTable Routes { get; set; } @@ -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); @@ -206,9 +205,8 @@ internal virtual void Refresh(bool isNavigationIntercepted) } } - private async ValueTask 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. @@ -217,59 +215,35 @@ private async ValueTask 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 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) @@ -277,7 +251,7 @@ 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); } } diff --git a/src/Components/Components/test/Routing/RouterTest.cs b/src/Components/Components/test/Routing/RouterTest.cs index 62569d5c569c..557a6f0d9deb 100644 --- a/src/Components/Components/test/Routing/RouterTest.cs +++ b/src/Components/Components/test/Routing/RouterTest.cs @@ -42,41 +42,18 @@ public async Task CanRunOnNavigateAsync() { // Arrange var called = false; - async Task OnNavigateAsync(NavigationContext args) + Action 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() - { - // Arrange - var called = false; - async Task OnNavigateAsync(NavigationContext args) - { - called = true; - await Task.CompletedTask; - throw new Exception("This is an uncaught exception."); - } - _router.OnNavigateAsync = OnNavigateAsync; + }; + _router.OnNavigateAsync = new EventCallback(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] @@ -84,7 +61,7 @@ public async Task CanceledFailedOnNavigateAsyncDoesNothing() { // Arrange var onNavigateInvoked = 0; - async Task OnNavigateAsync(NavigationContext args) + Action OnNavigateAsync = async (NavigationContext args) => { onNavigateInvoked += 1; if (args.Path.EndsWith("jan")) @@ -92,22 +69,12 @@ async Task OnNavigateAsync(NavigationContext args) await Task.Delay(Timeout.Infinite, args.CancellationToken); throw new Exception("This is an uncaught exception."); } - } - var refreshCalled = false; - _renderer.OnUpdateDisplay = (renderBatch) => - { - if (!refreshCalled) - { - refreshCalled = true; - return; - } - Assert.True(false, "OnUpdateDisplay called more than once."); }; - _router.OnNavigateAsync = OnNavigateAsync; + _router.OnNavigateAsync = new EventCallback(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; @@ -117,34 +84,12 @@ async Task OnNavigateAsync(NavigationContext args) Assert.Equal(2, onNavigateInvoked); } - [Fact] - public async Task CanHandleSingleCancelledOnNavigateAsync() - { - // Arrange - async Task OnNavigateAsync(NavigationContext args) - { - var tcs = new TaskCompletionSource(); - 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); - } - [Fact] public async Task AlreadyCanceledOnNavigateAsyncDoesNothing() { // Arrange var triggerCancel = new TaskCompletionSource(); - async Task OnNavigateAsync(NavigationContext args) + Action OnNavigateAsync = async (NavigationContext args) => { if (args.Path.EndsWith("jan")) { @@ -153,7 +98,7 @@ async Task OnNavigateAsync(NavigationContext args) tcs.TrySetCanceled(); await tcs.Task; } - } + }; var refreshCalled = false; _renderer.OnUpdateDisplay = (renderBatch) => { @@ -164,11 +109,11 @@ async Task OnNavigateAsync(NavigationContext args) } Assert.True(false, "OnUpdateDisplay called more than once."); }; - _router.OnNavigateAsync = OnNavigateAsync; + _router.OnNavigateAsync = new EventCallback(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; @@ -180,16 +125,16 @@ public void CanCancelPreviousOnNavigateAsync() { // Arrange var cancelled = ""; - async Task OnNavigateAsync(NavigationContext args) + Action OnNavigateAsync = async (NavigationContext args) => { await Task.CompletedTask; args.CancellationToken.Register(() => cancelled = args.Path); }; - _router.OnNavigateAsync = OnNavigateAsync; + _router.OnNavigateAsync = new EventCallback(null, OnNavigateAsync); // Act - _ = _router.RunOnNavigateWithRefreshAsync("jan", false); - _ = _router.RunOnNavigateWithRefreshAsync("feb", false); + _ = _router.RunOnNavigateAsync("jan", false); + _ = _router.RunOnNavigateAsync("feb", false); // Assert var expected = "jan"; @@ -200,7 +145,7 @@ async Task OnNavigateAsync(NavigationContext args) public async Task RefreshesOnceOnCancelledOnNavigateAsync() { // Arrange - async Task OnNavigateAsync(NavigationContext args) + Action OnNavigateAsync = async (NavigationContext args) => { if (args.Path.EndsWith("jan")) { @@ -217,11 +162,11 @@ async Task OnNavigateAsync(NavigationContext args) } Assert.True(false, "OnUpdateDisplay called more than once."); }; - _router.OnNavigateAsync = OnNavigateAsync; + _router.OnNavigateAsync = new EventCallback(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; From 7162fba4387853b7bb1ddfb63720bb068b2821ba Mon Sep 17 00:00:00 2001 From: Safia Abdalla Date: Wed, 19 Aug 2020 14:14:17 -0700 Subject: [PATCH 2/2] Add more tests --- .../Components/test/Routing/RouterTest.cs | 9 +++++++-- src/Components/test/E2ETest/Tests/RoutingTest.cs | 14 ++++++++++++-- .../RouterTest/TestRouterWithOnNavigate.razor | 13 ++++++++++++- 3 files changed, 31 insertions(+), 5 deletions(-) diff --git a/src/Components/Components/test/Routing/RouterTest.cs b/src/Components/Components/test/Routing/RouterTest.cs index 557a6f0d9deb..29da11476bf9 100644 --- a/src/Components/Components/test/Routing/RouterTest.cs +++ b/src/Components/Components/test/Routing/RouterTest.cs @@ -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 { @@ -70,6 +68,12 @@ public async Task CanceledFailedOnNavigateAsyncDoesNothing() throw new Exception("This is an uncaught exception."); } }; + var refreshCalled = 0; + _renderer.OnUpdateDisplay = (renderBatch) => + { + refreshCalled += 1; + return; + }; _router.OnNavigateAsync = new EventCallback(null, OnNavigateAsync); // Act @@ -82,6 +86,7 @@ public async Task CanceledFailedOnNavigateAsyncDoesNothing() // Assert that we render the second route component and don't throw an exception Assert.Empty(_renderer.HandledExceptions); Assert.Equal(2, onNavigateInvoked); + Assert.Equal(2, refreshCalled); } [Fact] diff --git a/src/Components/test/E2ETest/Tests/RoutingTest.cs b/src/Components/test/E2ETest/Tests/RoutingTest.cs index 28fe0a1727e3..d1642864dbee 100644 --- a/src/Components/test/E2ETest/Tests/RoutingTest.cs +++ b/src/Components/test/E2ETest/Tests/RoutingTest.cs @@ -570,14 +570,24 @@ public void OnNavigate_CanRenderUIForExceptions() { var app = Browser.MountTestComponent(); - // 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(); + + // 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() { diff --git a/src/Components/test/testassets/BasicTestApp/RouterTest/TestRouterWithOnNavigate.razor b/src/Components/test/testassets/BasicTestApp/RouterTest/TestRouterWithOnNavigate.razor index 7b0c289b56e6..933512d2bf5b 100644 --- a/src/Components/test/testassets/BasicTestApp/RouterTest/TestRouterWithOnNavigate.razor +++ b/src/Components/test/testassets/BasicTestApp/RouterTest/TestRouterWithOnNavigate.razor @@ -26,9 +26,15 @@ { "LongPage1", new Func(TestLoadingPageShows) }, { "LongPage2", new Func(TestOnNavCancel) }, { "Other", new Func(TestOnNavException) }, - {"WithParameters/name/Abc", new Func(TestRefreshHandling)} + { "WithLazyAssembly", new Func(TestOnNavException) }, + { "WithParameters/name/Abc", new Func(TestRefreshHandling) } }; + protected override void OnAfterRender(bool firstRender) + { + Console.WriteLine("Render triggered..."); + } + private async Task OnNavigateAsync(NavigationContext args) { Console.WriteLine($"Running OnNavigate for {args.Path}..."); @@ -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);