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

Error boundaries #30874

Merged
merged 55 commits into from
Apr 8, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
55 commits
Select commit Hold shift + click to select a range
c0d3d59
Initial experiments
SteveSandersonMS Mar 12, 2021
952cac0
Handle async event handler exceptions
SteveSandersonMS Mar 12, 2021
4b12fc2
Test cases for lifecycle methods
SteveSandersonMS Mar 12, 2021
cc44e53
Handle SetParametersAsync exceptions
SteveSandersonMS Mar 12, 2021
5351089
Handle IHandleAfterRender exceptions
SteveSandersonMS Mar 12, 2021
4e7e3c5
Handle exceptions during rendering
SteveSandersonMS Mar 12, 2021
e6c1c1c
Update notes about disposal
SteveSandersonMS Mar 12, 2021
0d6acd7
Add note about Attach
SteveSandersonMS Mar 12, 2021
2b35fe8
Handle exceptions during cascading value notifications
SteveSandersonMS Mar 12, 2021
42d1911
Update ErrorMaker.razor
SteveSandersonMS Mar 12, 2021
2f68770
Add note about InvokeAsync
SteveSandersonMS Mar 12, 2021
80ebf30
Clean up some notes
SteveSandersonMS Mar 12, 2021
83de667
Clarify what we do if the IErrorBoundary component itself has an error
SteveSandersonMS Mar 12, 2021
2a5a7d3
Force error boundaries to clear up their descendants on error, even b…
SteveSandersonMS Mar 12, 2021
0cd43ff
More thoughts on error loops
SteveSandersonMS Mar 12, 2021
0a7a07f
More thoughts
SteveSandersonMS Mar 12, 2021
8089c36
Clean up all the stuff about detailed errors and prerendering
SteveSandersonMS Mar 15, 2021
4514899
Add PrerenderingErrorBoundaryLogger
SteveSandersonMS Mar 15, 2021
2de4259
Let error boundaries catch their own inline errors
SteveSandersonMS Mar 16, 2021
415d8dd
Redesign around ErrorBoundaryBase abstract base class.
SteveSandersonMS Mar 16, 2021
9529163
Define IErrorBoundary as an internal interface for clarity
SteveSandersonMS Mar 17, 2021
bf04cf3
Minor fix
SteveSandersonMS Mar 18, 2021
1db319e
Handle logging errors
SteveSandersonMS Apr 6, 2021
0415115
Make it possible to customize the UI via subclassing ErrorBoundary
SteveSandersonMS Apr 6, 2021
a28c3ca
Better default UI
SteveSandersonMS Apr 6, 2021
275ae5a
Show you can subclass ErrorBoundary helpfully
SteveSandersonMS Apr 6, 2021
d319b9f
Rename AutoReset->AutoRecover
SteveSandersonMS Apr 6, 2021
15ab863
Remove use of linq
SteveSandersonMS Apr 6, 2021
990128e
Tidy up comments
SteveSandersonMS Apr 6, 2021
c9c7c6f
Clean up and expand test cases
SteveSandersonMS Apr 6, 2021
2f597f4
CR: Add general-purpose OnErrorAsync to base class
SteveSandersonMS Apr 6, 2021
29e4760
Remove AutoRecover feature because it can be done in user code now we…
SteveSandersonMS Apr 6, 2021
08d4595
Unit tests for component error boundary dispatch logic
SteveSandersonMS Apr 6, 2021
7540d3a
Make async flow clearer
SteveSandersonMS Apr 6, 2021
34e6519
Fix unrelated test code badness
SteveSandersonMS Apr 6, 2021
06da1ef
Add E2E test UI
SteveSandersonMS Apr 6, 2021
259f5d5
Begin scripting E2E cases
SteveSandersonMS Apr 6, 2021
63d950b
Add some E2E tests for more difficult cases
SteveSandersonMS Apr 7, 2021
ae4cdc4
Cope with receiving errors while already in an error state
SteveSandersonMS Apr 7, 2021
76f41fe
Add failing unit test for errors after disposal
SteveSandersonMS Apr 7, 2021
f5be9f1
Have renderer cope with errors after disposal
SteveSandersonMS Apr 7, 2021
31def59
Better handling of error loops
SteveSandersonMS Apr 7, 2021
015b4e2
Fix description
SteveSandersonMS Apr 7, 2021
832f72a
Tidyup
SteveSandersonMS Apr 7, 2021
50c9ddb
Revert some unwanted changes
SteveSandersonMS Apr 7, 2021
7df7d5a
More cleanup
SteveSandersonMS Apr 7, 2021
0a61aaa
Add default error boundary content/style to project templates
SteveSandersonMS Apr 7, 2021
4955cc3
Fix server E2E tests
SteveSandersonMS Apr 7, 2021
92e506b
Test for custom error boundary
SteveSandersonMS Apr 7, 2021
8f18c40
Test for error ignorer
SteveSandersonMS Apr 7, 2021
d151976
Test for inline errors
SteveSandersonMS Apr 7, 2021
d85c8d2
Test for errors after disposal
SteveSandersonMS Apr 7, 2021
00e5e50
Test for multiple concurrent errors
SteveSandersonMS Apr 7, 2021
5eae0b1
Stronger guidance to override OnErrorAsync
SteveSandersonMS Apr 7, 2021
9ace45d
CR: Improve unit test
SteveSandersonMS Apr 8, 2021
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
126 changes: 126 additions & 0 deletions src/Components/Components/src/ErrorBoundaryBase.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
// 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.Runtime.ExceptionServices;
using System.Threading.Tasks;

namespace Microsoft.AspNetCore.Components
{
/// <summary>
/// A base class for error boundary components.
/// </summary>
public abstract class ErrorBoundaryBase : ComponentBase, IErrorBoundary
{
private int _errorCount;

/// <summary>
/// The content to be displayed when there is no error.
/// </summary>
[Parameter] public RenderFragment? ChildContent { get; set; }

/// <summary>
/// The content to be displayed when there is an error.
/// </summary>
[Parameter] public RenderFragment<Exception>? ErrorContent { get; set; }

/// <summary>
/// The maximum number of errors that can be handled. If more errors are received,
/// they will be treated as fatal. Calling <see cref="Recover"/> resets the count.
/// </summary>
[Parameter] public int MaximumErrorCount { get; set; } = 100;
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this to be public if we're only using it to determine whether logic in ErrorContent is causing an infinite stream of exceptions? It's not clear to me why we would want users to be able to customize it if we can set a sensible default.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's certainly open for debate. A developer might, for example, have a list with 101 items and want to be able to tolerate the case where every item fails independently, so they might have a reason to increase this number.

The tradeoff with bigger values is that there's more processing and more log messages generated before we eventually stop the process. I'm not sure there would be a single value that would be ideal for all cases.


/// <summary>
/// Gets the current exception, or null if there is no exception.
/// </summary>
protected Exception? CurrentException { get; private set; }

/// <summary>
/// Resets the error boundary to a non-errored state. If the error boundary is not
/// already in an errored state, the call has no effect.
/// </summary>
public void Recover()
{
if (CurrentException is not null)
{
_errorCount = 0;
CurrentException = null;
StateHasChanged();
}
}

/// <summary>
/// Invoked by the base class when an error is being handled. Typically, derived classes
/// should log the exception from this method.
/// </summary>
/// <param name="exception">The <see cref="Exception"/> being handled.</param>
protected abstract Task OnErrorAsync(Exception exception);

void IErrorBoundary.HandleException(Exception exception)
{
if (exception is null)
{
// This would be a framework bug if it happened. It should not be possible.
throw new ArgumentNullException(nameof(exception));
}

// If rendering the error content itself causes an error, then re-rendering on error risks creating an
// infinite error loop. Unfortunately it's very hard to distinguish whether the error source is "child content"
// or "error content", since the exceptions can be received asynchronously, arbitrarily long after we switched
// between normal and errored states. Without creating a very intricate coupling between ErrorBoundaryBase and
// Renderer internals, the obvious options are either:
Copy link
Member

Choose a reason for hiding this comment

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

We could "easily" determine this within the renderer and pass that info to the handler, isn't it?

The callsite for HandleException knows the handler and the source, so it can just compare them for reference equality and pass that information to handle exception (I think that's likely useful).

If we were todo that, then do we still need a limit on the number of exceptions? We could choose to say something like:

  • If the exception is not within the ErrorBoundary component handle it, otherwise throw?

Copy link
Member Author

@SteveSandersonMS SteveSandersonMS Apr 8, 2021

Choose a reason for hiding this comment

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

It's not at all easy, as far as I know.

If the exception is not within the ErrorBoundary component handle it, otherwise throw?

Two counterexamples:

  1. If the ErrorBoundary is not yet in an errored state (or has recovered), then the exceptions will be within it, and should not be fatal, so that would be a false positive
  2. Even if the exception appears not to be from within the ErrorBoundary, that might be because some intermediate child has since been disposed, and in that case it might still be from the error content, so that would be a false negative

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough, I don't have a deep enough intuition about this, so I'll trust your judgment (and the tests) here.

//
// [a] Don't re-render if we're already in an error state. This is problematic because the renderer needs to
// discard the error boundary's subtree on every error, in case a custom error boundary fails to do so, and
// hence we'd be left with a blank UI if we didn't re-render.
// [b] Do re-render each time, and trust the developer not to cause errors from their error content.
//
// As a middle ground, we try to detect excessive numbers of errors arriving in between recoveries, and treat
// an excess as fatal. This also helps to expose the case where a child continues to throw (e.g., on a timer),
// which would be very inefficient.
if (++_errorCount > MaximumErrorCount)
{
ExceptionDispatchInfo.Capture(exception).Throw();
}

// Notify the subclass so it can begin any async operation even before we render, because (for example)
// we want logs to be written before rendering in case the rendering throws. But there's no reason to
// wait for the async operation to complete before we render.
var onErrorTask = OnErrorAsync(exception);
if (!onErrorTask.IsCompletedSuccessfully)
{
_ = HandleOnErrorExceptions(onErrorTask);
}

CurrentException = exception;
StateHasChanged();
}

private async Task HandleOnErrorExceptions(Task onExceptionTask)
{
if (onExceptionTask.IsFaulted)
{
// Synchronous error handling exceptions can simply be fatal to the circuit
ExceptionDispatchInfo.Capture(onExceptionTask.Exception!).Throw();
}
else
{
// Async exceptions are tricky because there's no natural way to bring them back
// onto the sync context within their original circuit. The closest approximation
// we have is trying to rethrow via rendering. If, in the future, we add an API for
// directly dispatching an exception from ComponentBase, we should use that here.
try
{
await onExceptionTask;
}
catch (Exception exception)
{
CurrentException = exception;
ChildContent = _ => ExceptionDispatchInfo.Capture(exception).Throw();
ErrorContent = _ => _ => ExceptionDispatchInfo.Capture(exception).Throw();
StateHasChanged();
}
}
}
}
}
23 changes: 23 additions & 0 deletions src/Components/Components/src/IErrorBoundary.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
// 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;

namespace Microsoft.AspNetCore.Components
{
// Purpose of this interface, instead of just using ErrorBoundaryBase directly:
Copy link
Member

Choose a reason for hiding this comment

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

I like the code comments you've added here and elsewhere. Good stuff!

//
// [1] It keeps clear what is fundamental to an error boundary from the Renderer's perspective.
// Anything more specific than this is just a useful pattern inside ErrorBoundaryBase.
// [2] It improves linkability. If an application isn't using error boundaries, then all of
// ErrorBoundaryBase and its dependencies can be linked out, leaving only this interface.
//
// If we wanted, we could make this public, but it could lead to common antipatterns such as
// routinely marking all components as error boundaries (e.g., in a common base class) in an
// attempt to create "On Error Resume Next"-type behaviors.

internal interface IErrorBoundary
{
void HandleException(Exception error);
}
}
11 changes: 11 additions & 0 deletions src/Components/Components/src/PublicAPI.Unshipped.txt
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,16 @@ Microsoft.AspNetCore.Components.ComponentApplicationState.PersistAsJson<TValue>(
Microsoft.AspNetCore.Components.ComponentApplicationState.PersistState(string! key, byte[]! value) -> void
Microsoft.AspNetCore.Components.ComponentApplicationState.TryTakeAsJson<TValue>(string! key, out TValue? instance) -> bool
Microsoft.AspNetCore.Components.ComponentApplicationState.TryTakePersistedState(string! key, out byte[]? value) -> bool
Microsoft.AspNetCore.Components.ErrorBoundaryBase
Microsoft.AspNetCore.Components.ErrorBoundaryBase.ChildContent.get -> Microsoft.AspNetCore.Components.RenderFragment?
Microsoft.AspNetCore.Components.ErrorBoundaryBase.ChildContent.set -> void
Microsoft.AspNetCore.Components.ErrorBoundaryBase.CurrentException.get -> System.Exception?
Microsoft.AspNetCore.Components.ErrorBoundaryBase.ErrorBoundaryBase() -> void
Microsoft.AspNetCore.Components.ErrorBoundaryBase.ErrorContent.get -> Microsoft.AspNetCore.Components.RenderFragment<System.Exception!>?
Microsoft.AspNetCore.Components.ErrorBoundaryBase.ErrorContent.set -> void
Microsoft.AspNetCore.Components.ErrorBoundaryBase.MaximumErrorCount.get -> int
Microsoft.AspNetCore.Components.ErrorBoundaryBase.MaximumErrorCount.set -> void
Microsoft.AspNetCore.Components.ErrorBoundaryBase.Recover() -> void
Microsoft.AspNetCore.Components.Lifetime.ComponentApplicationLifetime
Microsoft.AspNetCore.Components.Lifetime.ComponentApplicationLifetime.ComponentApplicationLifetime(Microsoft.Extensions.Logging.ILogger<Microsoft.AspNetCore.Components.Lifetime.ComponentApplicationLifetime!>! logger) -> void
Microsoft.AspNetCore.Components.Lifetime.ComponentApplicationLifetime.PersistStateAsync(Microsoft.AspNetCore.Components.Lifetime.IComponentApplicationStateStore! store, Microsoft.AspNetCore.Components.RenderTree.Renderer! renderer) -> System.Threading.Tasks.Task!
Expand All @@ -33,6 +43,7 @@ Microsoft.AspNetCore.Components.CascadingTypeParameterAttribute
Microsoft.AspNetCore.Components.CascadingTypeParameterAttribute.CascadingTypeParameterAttribute(string! name) -> void
Microsoft.AspNetCore.Components.CascadingTypeParameterAttribute.Name.get -> string!
Microsoft.AspNetCore.Components.RenderTree.Renderer.GetEventArgsType(ulong eventHandlerId) -> System.Type!
abstract Microsoft.AspNetCore.Components.ErrorBoundaryBase.OnErrorAsync(System.Exception! exception) -> System.Threading.Tasks.Task!
override Microsoft.AspNetCore.Components.LayoutComponentBase.SetParametersAsync(Microsoft.AspNetCore.Components.ParameterView parameters) -> System.Threading.Tasks.Task!
static Microsoft.AspNetCore.Components.ParameterView.FromDictionary(System.Collections.Generic.IDictionary<string!, object?>! parameters) -> Microsoft.AspNetCore.Components.ParameterView
virtual Microsoft.AspNetCore.Components.RenderTree.Renderer.DispatchEventAsync(ulong eventHandlerId, Microsoft.AspNetCore.Components.RenderTree.EventFieldInfo? fieldInfo, System.EventArgs! eventArgs) -> System.Threading.Tasks.Task!
Expand Down
Loading