-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
Error boundaries #30874
Changes from 54 commits
c0d3d59
952cac0
4b12fc2
cc44e53
5351089
4e7e3c5
e6c1c1c
0d6acd7
2b35fe8
42d1911
2f68770
80ebf30
83de667
2a5a7d3
0cd43ff
0a7a07f
8089c36
4514899
2de4259
415d8dd
9529163
bf04cf3
1db319e
0415115
a28c3ca
275ae5a
d319b9f
15ab863
990128e
c9c7c6f
2f597f4
29e4760
08d4595
7540d3a
34e6519
06da1ef
259f5d5
63d950b
ae4cdc4
76f41fe
f5be9f1
31def59
015b4e2
832f72a
50c9ddb
7df7d5a
0a61aaa
4955cc3
92e506b
8f18c40
d151976
d85c8d2
00e5e50
5eae0b1
9ace45d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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; | ||
|
||
/// <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: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not at all easy, as far as I know.
Two counterexamples:
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
} | ||
} | ||
} | ||
} | ||
} |
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: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} | ||
} |
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.