Skip to content

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

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

Merged
merged 1 commit into from
Jul 29, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
31 changes: 20 additions & 11 deletions src/Components/Components/src/Routing/Router.cs
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,19 @@ private void RefreshRouteTable()

internal virtual void Refresh(bool isNavigationIntercepted)
{
// If an `OnNavigateAsync` task is currently in progress, then wait
// for it to complete before rendering. Note: because _previousOnNavigateTask
// is initialized to a CompletedTask on initialization, this will still
// allow first-render to complete successfully.
if (_previousOnNavigateTask.Status != TaskStatus.RanToCompletion)
{
if (Navigating != null)
{
_renderHandle.Render(Navigating);
}
return;
}

RefreshRouteTable();

var locationPath = NavigationManager.ToBaseRelativePath(_locationAbsolute);
Expand Down Expand Up @@ -248,19 +261,15 @@ internal async Task RunOnNavigateWithRefreshAsync(string path, bool isNavigation
var previousTask = _previousOnNavigateTask;
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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

}

private void OnLocationChanged(object sender, LocationChangedEventArgs args)
Expand Down
23 changes: 23 additions & 0 deletions src/Components/test/E2ETest/Tests/RoutingTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -567,6 +567,29 @@ public void OnNavigate_CanRenderUIForExceptions()
Assert.NotNull(errorUiElem);
}

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

// Navigate to a route
SetUrlViaPushState("/WithParameters/name/Abc");

// Click the button to trigger a re-render
var button = app.FindElement(By.Id("trigger-rerender"));
button.Click();

// Assert that the parameter route didn't render
Browser.DoesNotExist(By.Id("test-info"));

// Navigate to another page to cancel the previous `OnNavigateAsync`
// task and trigger a re-render on its completion
SetUrlViaPushState("/LongPage1");

// Confirm that the route was rendered
Browser.Equal("This is a long page you can scroll.", () => app.FindElement(By.Id("test-info")).Text);
}

private long BrowserScrollY
{
get => (long)((IJavaScriptExecutor)Browser).ExecuteScript("return window.scrollY");
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
@using Microsoft.AspNetCore.Components.Routing

@using System.Threading

<button @onclick="TriggerRerender" id="trigger-rerender">Trigger Rerender</button>

<Router AppAssembly="@typeof(BasicTestApp.Program).Assembly" OnNavigateAsync="@OnNavigateAsync">
<Navigating>
<div style="padding: 20px;background-color:blue;color:white;" id="loading-banner">
Expand All @@ -21,7 +25,8 @@
{
{ "LongPage1", new Func<NavigationContext, Task>(TestLoadingPageShows) },
{ "LongPage2", new Func<NavigationContext, Task>(TestOnNavCancel) },
{ "Other", new Func<NavigationContext, Task>(TestOnNavException) }
{ "Other", new Func<NavigationContext, Task>(TestOnNavException) },
{"WithParameters/name/Abc", new Func<NavigationContext, Task>(TestRefreshHandling)}
};

private async Task OnNavigateAsync(NavigationContext args)
Expand Down Expand Up @@ -50,4 +55,14 @@
await Task.CompletedTask;
throw new Exception("This is an uncaught exception.");
}

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

private void TriggerRerender()
{
Console.WriteLine("Nothing to see here, just an even to trigger a re-render...");
}
}