Skip to content
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.

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

Allow cancellation of navigation events in Blazor #42638

Merged
merged 32 commits into from
Jul 22, 2022
Merged
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
a7ae3e9
Prototype for canceling internal navigations
MackinnonBuck Jul 5, 2022
05dc74b
New overlapping navigation behavior, changed APIs
MackinnonBuck Jul 7, 2022
c1f8602
Added APIs to block external navigations
MackinnonBuck Jul 8, 2022
0af6785
Updated API baselines, added XML docs.
MackinnonBuck Jul 8, 2022
298117a
Update NavigationManager.cs
MackinnonBuck Jul 8, 2022
08bc4f0
Merge branch 'main' into mbuck/navigation-cancellation
MackinnonBuck Jul 10, 2022
f8bfc12
Safeguard for overlapping navigations
MackinnonBuck Jul 10, 2022
ff6b245
PR feedback
MackinnonBuck Jul 12, 2022
1a7aca5
Cleanup
MackinnonBuck Jul 12, 2022
7086ddd
Create blazor.webview.js
MackinnonBuck Jul 12, 2022
7edfc97
Fixed deadlock, better exception handling
MackinnonBuck Jul 12, 2022
055bedf
Minor cleanup
MackinnonBuck Jul 12, 2022
0cea6cd
BlazorWebView support
MackinnonBuck Jul 13, 2022
804b451
Started E2E tests, fixed Blazor.navigateTo() bug
MackinnonBuck Jul 14, 2022
01c6484
PR feedback
MackinnonBuck Jul 14, 2022
ab6caad
Miscellaneous improvements
MackinnonBuck Jul 14, 2022
bb4d33b
Switch NavigationLock to use IDs
MackinnonBuck Jul 14, 2022
6f449bb
Update RoutingTest.cs
MackinnonBuck Jul 14, 2022
2fca4ce
Fixed history navigation test
MackinnonBuck Jul 15, 2022
3365322
Update NavigationManagerComponent.razor
MackinnonBuck Jul 15, 2022
54d7bca
PR feedback
MackinnonBuck Jul 18, 2022
72230e2
Debugging test failures in CI
MackinnonBuck Jul 18, 2022
4e3cca2
Updated E2E tests
MackinnonBuck Jul 18, 2022
9bfc11e
Fix potential trimming issue
MackinnonBuck Jul 18, 2022
7684aeb
History state entry support, more tests
MackinnonBuck Jul 18, 2022
8e2b8c0
Some PR feedback
MackinnonBuck Jul 19, 2022
cf17ac7
PR feedback + added NavigationManager unit tests
MackinnonBuck Jul 20, 2022
2771923
Updated E2E tests
MackinnonBuck Jul 20, 2022
22997be
Improved exception handling, more unit tests
MackinnonBuck Jul 20, 2022
8c017b8
Update TestCircuitHost.cs
MackinnonBuck Jul 20, 2022
61b99ed
Improved exception handling in handler callbacks
MackinnonBuck Jul 21, 2022
d155810
Fixed/verified Blazor Hybrid functionality
MackinnonBuck Jul 22, 2022
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
129 changes: 129 additions & 0 deletions src/Components/Components/src/NavigationManager.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Buffers;
using Microsoft.AspNetCore.Components.Routing;

namespace Microsoft.AspNetCore.Components;
Expand Down Expand Up @@ -31,6 +32,10 @@ public event EventHandler<LocationChangedEventArgs> LocationChanged

private EventHandler<LocationChangedEventArgs>? _locationChanged;

private readonly List<Func<LocationChangingContext, ValueTask>> _locationChangingHandlers = new();

private CancellationTokenSource? _locationChangingCts;

// For the baseUri it's worth storing as a System.Uri so we can do operations
// on that type. System.Uri gives us access to the original string anyway.
private Uri? _baseUri;
Expand Down Expand Up @@ -274,6 +279,130 @@ protected void NotifyLocationChanged(bool isInterceptedLink)
}
}

/// <summary>
/// Notifies the registered handlers of the current location change.
/// </summary>
/// <param name="uri">The destination URI. This can be absolute, or relative to the base URI.</param>
/// <param name="state">The state associated with the target history entry.</param>
/// <param name="isNavigationIntercepted">Whether this navigation was intercepted from a link.</param>
/// <returns>A <see cref="ValueTask{TResult}"/> representing the completion of the operation. If the result is <see langword="true"/>, the navigation should continue.</returns>
protected async ValueTask<bool> NotifyLocationChangingAsync(string uri, string? state, bool isNavigationIntercepted)
{
_locationChangingCts?.Cancel();
_locationChangingCts = null;

var handlerCount = _locationChangingHandlers.Count;

if (handlerCount == 0)
{
return true;
}

_locationChangingCts = new();

var context = new LocationChangingContext(uri, state, isNavigationIntercepted, _locationChangingCts);
javiercn marked this conversation as resolved.
Show resolved Hide resolved
var cancellationToken = _locationChangingCts.Token;

try
{
if (handlerCount == 1)
{
var handlerTask = _locationChangingHandlers[0](context);

if (cancellationToken.IsCancellationRequested)
{
return false;
}

if (!handlerTask.IsCompletedSuccessfully)
{
await handlerTask.AsTask().WaitAsync(cancellationToken);
}
}
else
{
var locationChangingHandlersCopy = ArrayPool<Func<LocationChangingContext, ValueTask>>.Shared.Rent(handlerCount);

try
{
_locationChangingHandlers.CopyTo(locationChangingHandlersCopy);

var locationChangingTasks = new Task[handlerCount];

for (var i = 0; i < handlerCount; i++)
{
var handlerTask = locationChangingHandlersCopy[i](context);

if (cancellationToken.IsCancellationRequested)
{
return false;
}

locationChangingTasks[i] = handlerTask.AsTask();
javiercn marked this conversation as resolved.
Show resolved Hide resolved
}

await Task.WhenAll(locationChangingTasks).WaitAsync(cancellationToken);
}
finally
{
ArrayPool<Func<LocationChangingContext, ValueTask>>.Shared.Return(locationChangingHandlersCopy);
}
}

return !cancellationToken.IsCancellationRequested;
}
catch (TaskCanceledException ex)
{
if (ex.CancellationToken == cancellationToken)
{
return false;
}

throw;
}
}

/// <summary>
/// Sets whether navigation is currently locked. If it is, then implementations should not update <see cref="Uri"/> and call
/// <see cref="NotifyLocationChanged(bool)"/> until they have first confirmed the navigation by calling
/// <see cref="NotifyLocationChangingAsync(string, string?, bool)"/>.
/// </summary>
/// <param name="value">Whether navigation is currently locked.</param>
protected virtual void SetNavigationLockState(bool value)
=> throw new NotSupportedException($"To support navigation locks, {GetType().Name} must override {nameof(SetNavigationLockState)}");

/// <summary>
/// Adds a handler to process incoming navigation events.
/// </summary>
/// <param name="locationChangingHandler">The handler to process incoming navigation events.</param>
public void AddLocationChangingHandler(Func<LocationChangingContext, ValueTask> locationChangingHandler)
{
AssertInitialized();

var isFirstHandler = _locationChangingHandlers.Count == 0;

_locationChangingHandlers.Add(locationChangingHandler);

if (isFirstHandler)
{
SetNavigationLockState(true);
}
}

/// <summary>
/// Removes a previously-added location changing handler.
/// </summary>
/// <param name="locationChangingHandler">The handler to remove.</param>
public void RemoveLocationChangingHandler(Func<LocationChangingContext, ValueTask> locationChangingHandler)
{
AssertInitialized();

if (_locationChangingHandlers.Remove(locationChangingHandler) && _locationChangingHandlers.Count == 0)
{
SetNavigationLockState(false);
}
}

private void AssertInitialized()
{
if (!_isInitialized)
Expand Down
10 changes: 10 additions & 0 deletions src/Components/Components/src/PublicAPI.Unshipped.txt
Original file line number Diff line number Diff line change
@@ -1,10 +1,19 @@
#nullable enable
Microsoft.AspNetCore.Components.NavigationManager.AddLocationChangingHandler(System.Func<Microsoft.AspNetCore.Components.Routing.LocationChangingContext!, System.Threading.Tasks.ValueTask>! locationChangingHandler) -> void
Microsoft.AspNetCore.Components.NavigationManager.HistoryEntryState.get -> string?
Microsoft.AspNetCore.Components.NavigationManager.HistoryEntryState.set -> void
Microsoft.AspNetCore.Components.NavigationManager.NotifyLocationChangingAsync(string! uri, string? state, bool isNavigationIntercepted) -> System.Threading.Tasks.ValueTask<bool>
Microsoft.AspNetCore.Components.NavigationManager.RemoveLocationChangingHandler(System.Func<Microsoft.AspNetCore.Components.Routing.LocationChangingContext!, System.Threading.Tasks.ValueTask>! locationChangingHandler) -> void
Microsoft.AspNetCore.Components.NavigationOptions.HistoryEntryState.get -> string?
Microsoft.AspNetCore.Components.NavigationOptions.HistoryEntryState.init -> void
Microsoft.AspNetCore.Components.Rendering.RenderTreeBuilder.AddContent(int sequence, Microsoft.AspNetCore.Components.MarkupString? markupContent) -> void
Microsoft.AspNetCore.Components.Routing.LocationChangedEventArgs.HistoryEntryState.get -> string?
Microsoft.AspNetCore.Components.Routing.LocationChangingContext
Microsoft.AspNetCore.Components.Routing.LocationChangingContext.CancellationToken.get -> System.Threading.CancellationToken
Microsoft.AspNetCore.Components.Routing.LocationChangingContext.HistoryEntryState.get -> string?
Microsoft.AspNetCore.Components.Routing.LocationChangingContext.IsNavigationIntercepted.get -> bool
Microsoft.AspNetCore.Components.Routing.LocationChangingContext.PreventNavigation() -> void
Microsoft.AspNetCore.Components.Routing.LocationChangingContext.TargetLocation.get -> string!
static Microsoft.AspNetCore.Components.CompilerServices.RuntimeHelpers.CreateInferredEventCallback<T>(object! receiver, Microsoft.AspNetCore.Components.EventCallback<T> callback, T value) -> Microsoft.AspNetCore.Components.EventCallback<T>
static Microsoft.AspNetCore.Components.CompilerServices.RuntimeHelpers.InvokeAsynchronousDelegate(System.Action! callback) -> System.Threading.Tasks.Task!
static Microsoft.AspNetCore.Components.CompilerServices.RuntimeHelpers.InvokeAsynchronousDelegate(System.Func<System.Threading.Tasks.Task!>! callback) -> System.Threading.Tasks.Task!
Expand Down Expand Up @@ -41,3 +50,4 @@ static Microsoft.AspNetCore.Components.EventCallbackFactoryBinderExtensions.Crea
static Microsoft.AspNetCore.Components.EventCallbackFactoryBinderExtensions.CreateBinder(this Microsoft.AspNetCore.Components.EventCallbackFactory! factory, object! receiver, Microsoft.AspNetCore.Components.EventCallback<short?> setter, short? existingValue, System.Globalization.CultureInfo? culture = null) -> Microsoft.AspNetCore.Components.EventCallback<Microsoft.AspNetCore.Components.ChangeEventArgs!>
static Microsoft.AspNetCore.Components.EventCallbackFactoryBinderExtensions.CreateBinder(this Microsoft.AspNetCore.Components.EventCallbackFactory! factory, object! receiver, Microsoft.AspNetCore.Components.EventCallback<string?> setter, string! existingValue, System.Globalization.CultureInfo? culture = null) -> Microsoft.AspNetCore.Components.EventCallback<Microsoft.AspNetCore.Components.ChangeEventArgs!>
static Microsoft.AspNetCore.Components.EventCallbackFactoryBinderExtensions.CreateBinder<T>(this Microsoft.AspNetCore.Components.EventCallbackFactory! factory, object! receiver, Microsoft.AspNetCore.Components.EventCallback<T> setter, T existingValue, System.Globalization.CultureInfo? culture = null) -> Microsoft.AspNetCore.Components.EventCallback<Microsoft.AspNetCore.Components.ChangeEventArgs!>
virtual Microsoft.AspNetCore.Components.NavigationManager.SetNavigationLockState(bool value) -> void
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

namespace Microsoft.AspNetCore.Components.Routing;

/// <summary>
/// Contains context for a change to the browser's current location.
/// </summary>
public class LocationChangingContext
{
private readonly CancellationTokenSource _cts;

internal LocationChangingContext(string targetLocation, string? historyEntryState, bool isNavigationIntercepted, CancellationTokenSource cts)
{
TargetLocation = targetLocation;
HistoryEntryState = historyEntryState;
IsNavigationIntercepted = isNavigationIntercepted;

_cts = cts;
}

/// <summary>
/// Gets the target location.
/// </summary>
public string TargetLocation { get; }

/// <summary>
/// Gets the state associated with the target history entry.
/// </summary>
public string? HistoryEntryState { get; }

/// <summary>
/// Gets whether this navigation was intercepted from a link.
/// </summary>
public bool IsNavigationIntercepted { get; }

/// <summary>
/// Gets a <see cref="System.Threading.CancellationToken"/> that can be used to determine if this navigation was canceled
/// (for example, because the user has triggered a different navigation).
/// </summary>
public CancellationToken CancellationToken => _cts.Token;

/// <summary>
/// Prevents this navigation from continuing.
/// </summary>
public void PreventNavigation()
{
_cts.Cancel();
}
}
28 changes: 28 additions & 0 deletions src/Components/Server/src/Circuits/CircuitHost.cs
Original file line number Diff line number Diff line change
Expand Up @@ -536,6 +536,31 @@ await Renderer.Dispatcher.InvokeAsync(() =>
}
}

public async Task OnLocationChangingAsync(int callId, string uri, string? state, bool intercepted)
{
AssertInitialized();
AssertNotDisposed();

try
{
var shouldContinueNavigation = await Renderer.Dispatcher.InvokeAsync(async () =>
{
Log.LocationChanging(_logger, uri, CircuitId);
var navigationManager = (RemoteNavigationManager)Services.GetRequiredService<NavigationManager>();
return await navigationManager.HandleLocationChangingAsync(uri, state, intercepted);
});

await Client.SendAsync("JS.EndLocationChanging", callId, shouldContinueNavigation);
}
catch (Exception ex)
{
// Exceptions thrown by location changing handlers should be treated like unhandled exceptions in user-code.
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 wonder if we should really be treating exceptions thrown in location changing handlers as non-fatal? In the current WebAssembly implementation of this feature, the navigation is canceled if an exception is thrown in the location changing handler. Should we treat it as a critical failure there instead? And, if we decide to make this case non-fatal, should we let the navigation continue if no other handlers cancel it?

Copy link
Member

Choose a reason for hiding this comment

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

I'd err on the side of treating unhandled exceptions as fatal unless there's a definite use case in which developers can't be expected to avoid unhandled exceptions. In this case I expect a developer could use try/catch inside their handler if they are doing something that might fail.

Generally in WebAssembly we've had a more laissez-faire attitude because we're inheriting the JS semantics around errors being something you log but may be able to continue afterwards. On Server, it's much more important to have clearly defined rules about when you can and can't continue after an exception.

Should we treat it as a critical failure there instead?

If possible, that would be nice. But as long as something ends up in the JS console giving the exception details, that's enough. It just has to be reasonable for the developer to see what went wrong.

Log.LocationChangeFailedInCircuit(_logger, uri, CircuitId, ex);
await TryNotifyClientErrorAsync(Client, GetClientErrorMessage(ex, $"Location changing to '{uri}' failed."));
UnhandledException?.Invoke(this, new UnhandledExceptionEventArgs(ex, isTerminating: false));
}
}

public void SetCircuitUser(ClaimsPrincipal user)
{
// This can be called before the circuit is initialized.
Expand Down Expand Up @@ -730,6 +755,9 @@ public static void CircuitHandlerFailed(ILogger logger, CircuitHandler handler,
[LoggerMessage(210, LogLevel.Debug, "Location change to '{URI}' in circuit '{CircuitId}' failed.", EventName = "LocationChangeFailed")]
public static partial void LocationChangeFailed(ILogger logger, string uri, CircuitId circuitId, Exception exception);

[LoggerMessage(211, LogLevel.Debug, "Location is about to change to {URI} in ciruit '{CircuitId}'.", EventName = "LocationChanging")]
public static partial void LocationChanging(ILogger logger, string uri, CircuitId circuitId);

[LoggerMessage(212, LogLevel.Debug, "Failed to complete render batch '{RenderId}' in circuit host '{CircuitId}'.", EventName = "OnRenderCompletedFailed")]
public static partial void OnRenderCompletedFailed(ILogger logger, long renderId, CircuitId circuitId, Exception e);

Expand Down
55 changes: 54 additions & 1 deletion src/Components/Server/src/Circuits/RemoteNavigationManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ internal sealed partial class RemoteNavigationManager : NavigationManager, IHost
{
private readonly ILogger<RemoteNavigationManager> _logger;
private IJSRuntime _jsRuntime;
private bool? _navigationLockStateBeforeJsRuntimeAttached;

/// <summary>
/// Creates a new <see cref="RemoteNavigationManager"/> instance.
Expand Down Expand Up @@ -54,6 +55,12 @@ public void AttachJsRuntime(IJSRuntime jsRuntime)
}

_jsRuntime = jsRuntime;

if (_navigationLockStateBeforeJsRuntimeAttached.HasValue)
{
SetHasLocationChangingListeners(_navigationLockStateBeforeJsRuntimeAttached.Value);
_navigationLockStateBeforeJsRuntimeAttached = null;
}
}

public void NotifyLocationChanged(string uri, string state, bool intercepted)
Expand All @@ -65,6 +72,11 @@ public void NotifyLocationChanged(string uri, string state, bool intercepted)
NotifyLocationChanged(intercepted);
}

public async ValueTask<bool> HandleLocationChangingAsync(string uri, string? state, bool intercepted)
{
return await NotifyLocationChangingAsync(uri, state, intercepted);
}

/// <inheritdoc />
[DynamicDependency(DynamicallyAccessedMemberTypes.PublicProperties, typeof(NavigationOptions))]
protected override void NavigateToCore(string uri, NavigationOptions options)
Expand All @@ -77,9 +89,44 @@ protected override void NavigateToCore(string uri, NavigationOptions options)
throw new NavigationException(absoluteUriString);
}

_jsRuntime.InvokeVoidAsync(Interop.NavigateTo, uri, options).Preserve();
_ = PerformNavigationAsync();

async Task PerformNavigationAsync()
{
try
{
var shouldContinueNavigation = await NotifyLocationChangingAsync(uri, options.HistoryEntryState, false);

if (shouldContinueNavigation)
{
await _jsRuntime.InvokeVoidAsync(Interop.NavigateTo, uri, options);
}
else
{
Log.NavigationCanceled(_logger, uri);
}
}
catch (Exception ex)
{
Log.NavigationFailed(_logger, uri, ex);
javiercn marked this conversation as resolved.
Show resolved Hide resolved
}
}
}

protected override void SetNavigationLockState(bool value)
{
if (_jsRuntime is null)
{
_navigationLockStateBeforeJsRuntimeAttached = value;
return;
}

SetHasLocationChangingListeners(value);
}

private void SetHasLocationChangingListeners(bool value)
=> _jsRuntime.InvokeVoidAsync(Interop.SetHasLocationChangingListeners, value).Preserve();

private static partial class Log
{
[LoggerMessage(1, LogLevel.Debug, "Requesting navigation to URI {Uri} with forceLoad={ForceLoad}, replace={Replace}", EventName = "RequestingNavigation")]
Expand All @@ -90,5 +137,11 @@ public static void RequestingNavigation(ILogger logger, string uri, NavigationOp

[LoggerMessage(2, LogLevel.Debug, "Received notification that the URI has changed to {Uri} with isIntercepted={IsIntercepted}", EventName = "ReceivedLocationChangedNotification")]
public static partial void ReceivedLocationChangedNotification(ILogger logger, string uri, bool isIntercepted);

[LoggerMessage(3, LogLevel.Debug, "Navigation canceled when changing the location to {Uri}", EventName = "NavigationCanceled")]
public static partial void NavigationCanceled(ILogger logger, string uri);

[LoggerMessage(4, LogLevel.Error, "Navigation failed when changing the location to {Uri}", EventName = "NavigationFailed")]
public static partial void NavigationFailed(ILogger logger, string uri, Exception exception);
}
}
11 changes: 11 additions & 0 deletions src/Components/Server/src/ComponentHub.cs
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,17 @@ public async ValueTask OnLocationChanged(string uri, string? state, bool interce
_ = circuitHost.OnLocationChangedAsync(uri, state, intercepted);
}

public async ValueTask OnLocationChanging(int callId, string uri, string? state, bool intercepted)
{
var circuitHost = await GetActiveCircuitAsync();
if (circuitHost == null)
{
return;
}

_ = circuitHost.OnLocationChangingAsync(callId, uri, state, intercepted);
}

// We store the CircuitHost through a *handle* here because Context.Items is tied to the lifetime
// of the connection. It's possible that a misbehaving client could cause disposal of a CircuitHost
// but keep a connection open indefinitely, preventing GC of the Circuit and related application state.
Expand Down
11 changes: 11 additions & 0 deletions src/Components/Server/test/Circuits/ComponentHubTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,17 @@ public async Task CannotInvokeOnLocationChangedBeforeInitialization()
mockClientProxy.Verify(m => m.SendCoreAsync("JS.Error", new[] { errorMessage }, It.IsAny<CancellationToken>()), Times.Once());
}

[Fact]
public async Task CannotInvokeOnLocationChangingBeforeInitialization()
{
var (mockClientProxy, hub) = InitializeComponentHub();

await hub.OnLocationChanging(0, "https://localhost:5000/subdir/page", null, false);

var errorMessage = "Circuit not initialized.";
mockClientProxy.Verify(m => m.SendCoreAsync("JS.Error", new[] { errorMessage }, It.IsAny<CancellationToken>()), Times.Once());
}

private static (Mock<ISingleClientProxy>, ComponentHub) InitializeComponentHub()
{
var ephemeralDataProtectionProvider = new EphemeralDataProtectionProvider();
Expand Down
Loading