From c0d3d59cdfd42eb230692ab0342312a4279714c0 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Fri, 12 Mar 2021 10:46:44 +0000 Subject: [PATCH 01/55] Initial experiments --- .../Components/src/IErrorBoundary.cs | 12 ++ .../Components/src/PublicAPI.Unshipped.txt | 2 + .../Components/src/RenderTree/Renderer.cs | 61 +++++++++- .../BlazorServerApp/Pages/ErrorMaker.razor | 22 ++++ .../BlazorServerApp/Pages/FetchData.razor | 46 -------- .../BlazorServerApp/Shared/MainLayout.razor | 4 +- .../BlazorServerApp/Shared/NavMenu.razor | 4 +- .../Web/src/PublicAPI.Unshipped.txt | 11 ++ src/Components/Web/src/Web/ErrorBoundary.cs | 111 ++++++++++++++++++ 9 files changed, 223 insertions(+), 50 deletions(-) create mode 100644 src/Components/Components/src/IErrorBoundary.cs create mode 100644 src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor delete mode 100644 src/Components/Samples/BlazorServerApp/Pages/FetchData.razor create mode 100644 src/Components/Web/src/Web/ErrorBoundary.cs diff --git a/src/Components/Components/src/IErrorBoundary.cs b/src/Components/Components/src/IErrorBoundary.cs new file mode 100644 index 000000000000..b648239706cb --- /dev/null +++ b/src/Components/Components/src/IErrorBoundary.cs @@ -0,0 +1,12 @@ +// 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 +{ + public interface IErrorBoundary + { + void HandleException(Exception exception); + } +} diff --git a/src/Components/Components/src/PublicAPI.Unshipped.txt b/src/Components/Components/src/PublicAPI.Unshipped.txt index 459cce22c1ad..ada7a56545e0 100644 --- a/src/Components/Components/src/PublicAPI.Unshipped.txt +++ b/src/Components/Components/src/PublicAPI.Unshipped.txt @@ -9,6 +9,8 @@ Microsoft.AspNetCore.Components.ComponentApplicationState.PersistAsJson( Microsoft.AspNetCore.Components.ComponentApplicationState.PersistState(string! key, byte[]! value) -> void Microsoft.AspNetCore.Components.ComponentApplicationState.TryTakeAsJson(string! key, out TValue? instance) -> bool Microsoft.AspNetCore.Components.ComponentApplicationState.TryTakePersistedState(string! key, out byte[]? value) -> bool +Microsoft.AspNetCore.Components.IErrorBoundary +Microsoft.AspNetCore.Components.IErrorBoundary.HandleException(System.Exception! exception) -> void Microsoft.AspNetCore.Components.Lifetime.ComponentApplicationLifetime Microsoft.AspNetCore.Components.Lifetime.ComponentApplicationLifetime.ComponentApplicationLifetime(Microsoft.Extensions.Logging.ILogger! logger) -> void Microsoft.AspNetCore.Components.Lifetime.ComponentApplicationLifetime.PersistStateAsync(Microsoft.AspNetCore.Components.Lifetime.IComponentApplicationStateStore! store, Microsoft.AspNetCore.Components.RenderTree.Renderer! renderer) -> System.Threading.Tasks.Task! diff --git a/src/Components/Components/src/RenderTree/Renderer.cs b/src/Components/Components/src/RenderTree/Renderer.cs index ddcbf714b453..aaa760cb68c1 100644 --- a/src/Components/Components/src/RenderTree/Renderer.cs +++ b/src/Components/Components/src/RenderTree/Renderer.cs @@ -7,6 +7,7 @@ using System.Collections.Generic; using System.Diagnostics; using System.Diagnostics.CodeAnalysis; +using System.Linq; using System.Threading; using System.Threading.Tasks; using Microsoft.AspNetCore.Components.HotReload; @@ -335,7 +336,28 @@ public virtual Task DispatchEventAsync(ulong eventHandlerId, EventFieldInfo? fie } catch (Exception e) { - HandleException(e); + // Exceptions in event handlers don't compromise the stability of the framework, since the + // framework wasn't processing any internal state that might have been left corrupted. The + // risk is that user code is now left in some invalid state. So: + // + // * If we can identify a strong enough candidate for which component subtree is at risk + // of being affected, we'll shut down that subtree. The conditions for this are: + // * The event callback was dispatched to an IComponent + // * ... which is still live in the hierarchy + // * ... and is within an error boundary + // * Otherwise, we're in a more ambiguous case, such as event callbacks being dispatched + // to non-components. In this case, we'll treat it as fatal for the entire renderer. + // + // This is pretty subjective. Even in the "safe" case, it's possible that some non-component + // (domain model) state has been left corrupted. But we have to let developers be responsible + // for keeping their non-component state valid otherwise we couldn't have any error recovery. + var recovered = callback.Receiver is IComponent componentReceiver + && TrySendExceptionToErrorBoundary(componentReceiver, e); + if (!recovered) + { + HandleException(e); + } + return Task.CompletedTask; } finally @@ -353,6 +375,43 @@ public virtual Task DispatchEventAsync(ulong eventHandlerId, EventFieldInfo? fie return result; } + private bool TrySendExceptionToErrorBoundary(IComponent errorSource, Exception error) + { + if (!_isBatchInProgress) + { + // This should never happen, but if we have a bug, we want to know + throw new InvalidOperationException($"{nameof(TrySendExceptionToErrorBoundary)} can only be called when a batch is in progress."); + } + + // This linear search will be slow, but only happens during exception handling, so is likely OK. + // If it becomes problematic, we can maintain a lookup from component instance to ID. + var componentState = _componentStateById.FirstOrDefault(kvp => kvp.Value.Component == errorSource).Value; + + // Find the closest ancestor that's an immediate child of an error boundary. This is the component + // that will be terminated. + while (componentState is not null) + { + // TODO: Consider having an IErrorBoundary interface to support custom implementations + if (componentState.ParentComponentState?.Component is IErrorBoundary errorBoundary) + { + // TODO: Is it OK that we're trusting the IErrorBoundary to remove its previous child + // content? What if it fails to do so? We could proactively dispose componentState.Component + // within the current batch here if we wanted, but that does complicate things. Until now, + // we only dispose components during diffing, and there's an error if diffing tries to queue a + // disposal for something that's already disposed. + // Overall I think it's probably OK to let the IErrorBoundary make its own choice. Almost + // everyone will have the default behaviors, and even if someone leaves the broken component + // in place, we will already have done some level of recovery close to the call site. + errorBoundary.HandleException(error); + return true; + } + + componentState = componentState.ParentComponentState; + } + + return false; + } + /// /// Gets the event arguments type for the specified event handler. /// diff --git a/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor b/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor new file mode 100644 index 000000000000..50b1f2defa6f --- /dev/null +++ b/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor @@ -0,0 +1,22 @@ +@page "/errormaker" + +
+ Error maker + +
+ Event handlers + + +
+
+ +@code { + void EventHandlerErrorSync() + => throw new InvalidTimeZoneException("Synchronous error from event handler"); + + async Task EventHandlerErrorAsync() + { + await Task.Yield(); + throw new InvalidTimeZoneException("Asynchronous error from event handler"); + } +} diff --git a/src/Components/Samples/BlazorServerApp/Pages/FetchData.razor b/src/Components/Samples/BlazorServerApp/Pages/FetchData.razor deleted file mode 100644 index aefc6d9cc01b..000000000000 --- a/src/Components/Samples/BlazorServerApp/Pages/FetchData.razor +++ /dev/null @@ -1,46 +0,0 @@ -@page "/fetchdata" - -@using BlazorServerApp.Data -@inject WeatherForecastService ForecastService - -

Weather forecast

- -

This component demonstrates fetching data from a service.

- -@if (forecasts == null) -{ -

Loading...

-} -else -{ - - - - - - - - - - - @foreach (var forecast in forecasts) - { - - - - - - - } - -
DateTemp. (C)Temp. (F)Summary
@forecast.Date.ToShortDateString()@forecast.TemperatureC@forecast.TemperatureF@forecast.Summary
-} - -@code { - WeatherForecast[]? forecasts; - - protected override async Task OnInitializedAsync() - { - forecasts = await ForecastService.GetForecastAsync(DateTime.Now); - } -} diff --git a/src/Components/Samples/BlazorServerApp/Shared/MainLayout.razor b/src/Components/Samples/BlazorServerApp/Shared/MainLayout.razor index d353bfa08fa7..3bac776c888c 100644 --- a/src/Components/Samples/BlazorServerApp/Shared/MainLayout.razor +++ b/src/Components/Samples/BlazorServerApp/Shared/MainLayout.razor @@ -10,7 +10,9 @@
- @Body + + @Body +
diff --git a/src/Components/Samples/BlazorServerApp/Shared/NavMenu.razor b/src/Components/Samples/BlazorServerApp/Shared/NavMenu.razor index 5f6f4ae39140..b4d145836c93 100644 --- a/src/Components/Samples/BlazorServerApp/Shared/NavMenu.razor +++ b/src/Components/Samples/BlazorServerApp/Shared/NavMenu.razor @@ -18,8 +18,8 @@
  • - - Fetch data + + Errors
    • diff --git a/src/Components/Web/src/PublicAPI.Unshipped.txt b/src/Components/Web/src/PublicAPI.Unshipped.txt index f9125f300d01..42070070dd82 100644 --- a/src/Components/Web/src/PublicAPI.Unshipped.txt +++ b/src/Components/Web/src/PublicAPI.Unshipped.txt @@ -14,6 +14,17 @@ Microsoft.AspNetCore.Components.Forms.InputText.Element.set -> void Microsoft.AspNetCore.Components.Forms.InputTextArea.Element.get -> Microsoft.AspNetCore.Components.ElementReference? Microsoft.AspNetCore.Components.Forms.InputTextArea.Element.set -> void *REMOVED*static Microsoft.AspNetCore.Components.Forms.BrowserFileExtensions.RequestImageFileAsync(this Microsoft.AspNetCore.Components.Forms.IBrowserFile! browserFile, string! format, int maxWith, int maxHeight) -> System.Threading.Tasks.ValueTask +Microsoft.AspNetCore.Components.Web.ErrorBoundary +Microsoft.AspNetCore.Components.Web.ErrorBoundary.ChildContent.get -> Microsoft.AspNetCore.Components.RenderFragment? +Microsoft.AspNetCore.Components.Web.ErrorBoundary.ChildContent.set -> void +Microsoft.AspNetCore.Components.Web.ErrorBoundary.ErrorBoundary() -> void +Microsoft.AspNetCore.Components.Web.ErrorBoundary.ErrorContent.get -> Microsoft.AspNetCore.Components.RenderFragment? +Microsoft.AspNetCore.Components.Web.ErrorBoundary.ErrorContent.set -> void +Microsoft.AspNetCore.Components.Web.ErrorBoundary.FormattedError +Microsoft.AspNetCore.Components.Web.ErrorBoundary.FormattedError.Details.get -> string! +Microsoft.AspNetCore.Components.Web.ErrorBoundary.FormattedError.FormattedError(string! message, string! details) -> void +Microsoft.AspNetCore.Components.Web.ErrorBoundary.FormattedError.Message.get -> string! +Microsoft.AspNetCore.Components.Web.ErrorBoundary.HandleException(System.Exception! exception) -> void static Microsoft.AspNetCore.Components.ElementReferenceExtensions.FocusAsync(this Microsoft.AspNetCore.Components.ElementReference elementReference, bool preventScroll) -> System.Threading.Tasks.ValueTask static Microsoft.AspNetCore.Components.Forms.BrowserFileExtensions.RequestImageFileAsync(this Microsoft.AspNetCore.Components.Forms.IBrowserFile! browserFile, string! format, int maxWidth, int maxHeight) -> System.Threading.Tasks.ValueTask Microsoft.AspNetCore.Components.RenderTree.WebEventDescriptor.EventName.get -> string! diff --git a/src/Components/Web/src/Web/ErrorBoundary.cs b/src/Components/Web/src/Web/ErrorBoundary.cs new file mode 100644 index 000000000000..a7f0ef757c07 --- /dev/null +++ b/src/Components/Web/src/Web/ErrorBoundary.cs @@ -0,0 +1,111 @@ +// 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.Threading.Tasks; +using Microsoft.AspNetCore.Components.Rendering; +using Microsoft.JSInterop; + +namespace Microsoft.AspNetCore.Components.Web +{ + // TODO: Reimplement directly on IComponent + public sealed class ErrorBoundary : ComponentBase, IErrorBoundary + { + private FormattedError? _currentError; + + [Inject] private IJSRuntime? JS { get; set; } + + [Parameter] public RenderFragment? ChildContent { get; set; } + + // Notice that, to respect the IClientErrorFormatter, we have to do this in terms of a Message/Details + // pair, and not in terms of the original Exception. We can't assume that developers who provide an + // ErrorContent understand the issues with exposing the raw exception to the client. + [Parameter] public RenderFragment? ErrorContent { get; set; } + + // TODO: Eliminate the enableDetailedErrors flag (and corresponding API on Renderer) + // and instead have some new default DI service, IClientErrorFormatter. + public void HandleException(Exception exception) + { + // TODO: We should log the underlying exception to some ILogger using the same + // severity that we did before for unhandled exceptions + _currentError = FormatErrorForClient(exception); + StateHasChanged(); + + // TODO: Should there be an option not to auto-log exceptions to the console? + // Probably not. Don't want people thinking it's fine to have such exceptions. + _ = LogExceptionToClientIfPossible(); + } + + protected override void OnParametersSet() + { + // Not totally sure about this, but here we auto-recover if the parent component + // re-renders us. This has the benefit that in your layout, you can just wrap this + // around @Body and it does what you expect (recovering on navigate). But it might + // make other cases more awkward because the parent will keep recreating any children + // that just error out on init. Maybe it should be an option. + _currentError = null; + } + + protected override void BuildRenderTree(RenderTreeBuilder builder) + { + if (_currentError is null) + { + builder.AddContent(0, ChildContent); + } + else if (ErrorContent is not null) + { + builder.AddContent(1, ErrorContent(_currentError)); + } + else + { + builder.OpenRegion(2); + // TODO: Better UI (some kind of language-independent icon, CSS stylability) + // Could it simply be
    , with all the + // content provided in template CSS? Is this even going to be used in the + // template by default? It's probably best have some default UI that doesn't + // rely on there being any CSS, but make it overridable via CSS. + builder.OpenElement(0, "div"); + builder.AddAttribute(1, "onclick", (Func)(_ => + { + return LogExceptionToClientIfPossible(); + })); + + builder.AddContent(1, "Error"); + builder.CloseElement(); + builder.CloseRegion(); + } + } + + private async ValueTask LogExceptionToClientIfPossible() + { + // TODO: Handle prerendering too. We can't use IJSRuntime while prerendering. + if (_currentError is not null) + { + await JS!.InvokeVoidAsync("console.error", $"{_currentError.Message}\n{_currentError.Details}"); + } + } + + // TODO: Move into a new IClientErrorFormatter service + private static FormattedError FormatErrorForClient(Exception exception) + { + // TODO: Obviously this will be internal to IClientErrorFormatter + var enableDetailedErrors = true; + + return enableDetailedErrors + ? new FormattedError(exception.Message, exception.StackTrace ?? string.Empty) + : new FormattedError("There was an error", "For more details turn on detailed exceptions by setting 'DetailedErrors: true' in 'appSettings.Development.json' or set 'CircuitOptions.DetailedErrors'."); + } + + public record FormattedError + { + public FormattedError(string message, string details) + { + Message = message; + Details = details; + } + + public string Message { get; } + public string Details { get; } + } + } +} From 952cac0afc240ef28748aa4e3aad3c05cc2166a9 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Fri, 12 Mar 2021 11:05:18 +0000 Subject: [PATCH 02/55] Handle async event handler exceptions --- .../Components/src/RenderTree/Renderer.cs | 38 +++++++++++-------- .../src/Rendering/ComponentState.cs | 7 +++- 2 files changed, 28 insertions(+), 17 deletions(-) diff --git a/src/Components/Components/src/RenderTree/Renderer.cs b/src/Components/Components/src/RenderTree/Renderer.cs index aaa760cb68c1..88e75e42c66b 100644 --- a/src/Components/Components/src/RenderTree/Renderer.cs +++ b/src/Components/Components/src/RenderTree/Renderer.cs @@ -317,6 +317,7 @@ public virtual Task DispatchEventAsync(ulong eventHandlerId, EventFieldInfo? fie Dispatcher.AssertAccess(); var callback = GetRequiredEventCallback(eventHandlerId); + IComponent? componentReceiver = callback.Receiver as IComponent; // Null if the receiver is null or isn't a component Log.HandlingEvent(_logger, eventHandlerId, eventArgs); if (fieldInfo != null) @@ -351,7 +352,7 @@ public virtual Task DispatchEventAsync(ulong eventHandlerId, EventFieldInfo? fie // This is pretty subjective. Even in the "safe" case, it's possible that some non-component // (domain model) state has been left corrupted. But we have to let developers be responsible // for keeping their non-component state valid otherwise we couldn't have any error recovery. - var recovered = callback.Receiver is IComponent componentReceiver + var recovered = componentReceiver is not null && TrySendExceptionToErrorBoundary(componentReceiver, e); if (!recovered) { @@ -371,17 +372,14 @@ public virtual Task DispatchEventAsync(ulong eventHandlerId, EventFieldInfo? fie // Task completed synchronously or is still running. We already processed all of the rendering // work that was queued so let our error handler deal with it. - var result = GetErrorHandledTask(task); + var result = GetErrorHandledTask(task, componentReceiver); return result; } private bool TrySendExceptionToErrorBoundary(IComponent errorSource, Exception error) { - if (!_isBatchInProgress) - { - // This should never happen, but if we have a bug, we want to know - throw new InvalidOperationException($"{nameof(TrySendExceptionToErrorBoundary)} can only be called when a batch is in progress."); - } + // TODO: We probably need to ensure calls to this method are dispatched + Dispatcher.AssertAccess(); // This linear search will be slow, but only happens during exception handling, so is likely OK. // If it becomes problematic, we can maintain a lookup from component instance to ID. @@ -391,7 +389,6 @@ private bool TrySendExceptionToErrorBoundary(IComponent errorSource, Exception e // that will be terminated. while (componentState is not null) { - // TODO: Consider having an IErrorBoundary interface to support custom implementations if (componentState.ParentComponentState?.Component is IErrorBoundary errorBoundary) { // TODO: Is it OK that we're trusting the IErrorBoundary to remove its previous child @@ -402,6 +399,8 @@ private bool TrySendExceptionToErrorBoundary(IComponent errorSource, Exception e // Overall I think it's probably OK to let the IErrorBoundary make its own choice. Almost // everyone will have the default behaviors, and even if someone leaves the broken component // in place, we will already have done some level of recovery close to the call site. + // Also it's quite complicate to proactively dispose any component here, because we're not + // necessarily inside an in-progress render batch. errorBoundary.HandleException(error); return true; } @@ -448,7 +447,9 @@ internal void InstantiateChildComponentOnFrame(ref RenderTreeFrame frame, int pa frame.ComponentIdField = newComponentState.ComponentId; } - internal void AddToPendingTasks(Task task) + // TODO: Need to reason about all the code paths that lead here and ensure it makes sense + // to treat the exceptions as handleable + internal void AddToPendingTasks(Task task, IComponent? owningComponent) { switch (task == null ? TaskStatus.RanToCompletion : task.Status) { @@ -464,12 +465,13 @@ internal void AddToPendingTasks(Task task) // an 'async' state machine (the ones generated using async/await) where even // the synchronous exceptions will get captured and converted into a faulted // task. + // TODO: Presumably we need to use TrySendExceptionToErrorBoundary here HandleException(task.Exception.GetBaseException()); break; default: // It's important to evaluate the following even if we're not going to use // handledErrorTask below, because it has the side-effect of calling HandleException. - var handledErrorTask = GetErrorHandledTask(task); + var handledErrorTask = GetErrorHandledTask(task, owningComponent); // The pendingTasks collection is only used during prerendering to track quiescence, // so will be null at other times. @@ -758,7 +760,7 @@ private void NotifyRenderCompleted(ComponentState state, ref List batch) // The Task is incomplete. // Queue up the task and we can inspect it later. batch = batch ?? new List(); - batch.Add(GetErrorHandledTask(task)); + batch.Add(GetErrorHandledTask(task, null)); // TODO: Should there be an owningComponent? } private void RenderInExistingBatch(RenderQueueEntry renderQueueEntry) @@ -796,7 +798,9 @@ private void RenderInExistingBatch(RenderQueueEntry renderQueueEntry) } else { - AddToPendingTasks(GetHandledAsynchronousDisposalErrorsTask(result)); + // TODO: Should we use disposeComponentState.Component as the owningComponent + // so this is recoverable via IErrorBoundary? + AddToPendingTasks(GetHandledAsynchronousDisposalErrorsTask(result), null); async Task GetHandledAsynchronousDisposalErrorsTask(Task result) { @@ -874,7 +878,7 @@ async Task ContinueAfterTask(ArrayRange eventHandlerIds, Task afterTaskIg } } - private async Task GetErrorHandledTask(Task taskToHandle) + private async Task GetErrorHandledTask(Task taskToHandle, IComponent? owningComponent) { try { @@ -882,10 +886,14 @@ private async Task GetErrorHandledTask(Task taskToHandle) } catch (Exception ex) { + // Ignore errors due to task cancellations. if (!taskToHandle.IsCanceled) { - // Ignore errors due to task cancellations. - HandleException(ex); + if (owningComponent is null || !TrySendExceptionToErrorBoundary(owningComponent, ex)) + { + // It's unhandled + HandleException(ex); + } } } } diff --git a/src/Components/Components/src/Rendering/ComponentState.cs b/src/Components/Components/src/Rendering/ComponentState.cs index de620ccf8ffe..ae4cf25fe7e3 100644 --- a/src/Components/Components/src/Rendering/ComponentState.cs +++ b/src/Components/Components/src/Rendering/ComponentState.cs @@ -164,7 +164,8 @@ public void SetDirectParameters(ParameterView parameters) parameters = parameters.WithCascadingParameters(_cascadingParameters); } - _renderer.AddToPendingTasks(Component.SetParametersAsync(parameters)); + // TODO: Think about passing self as owningComponent for error recovery + _renderer.AddToPendingTasks(Component.SetParametersAsync(parameters), null); } public void NotifyCascadingValueChanged(in ParameterViewLifetime lifetime) @@ -174,7 +175,9 @@ public void NotifyCascadingValueChanged(in ParameterViewLifetime lifetime) : ParameterView.Empty; var allParams = directParams.WithCascadingParameters(_cascadingParameters!); var task = Component.SetParametersAsync(allParams); - _renderer.AddToPendingTasks(task); + + // TODO: Think about passing self as owningComponent for error recovery + _renderer.AddToPendingTasks(task, null); } private bool AddCascadingParameterSubscriptions() From 4b12fc2585a5efd9fca95a6a60133868f034b7fb Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Fri, 12 Mar 2021 11:14:49 +0000 Subject: [PATCH 03/55] Test cases for lifecycle methods --- .../BlazorServerApp/Pages/ErrorMaker.razor | 17 ++++++++ .../Shared/ErrorCausingChild.razor | 42 +++++++++++++++++++ 2 files changed, 59 insertions(+) create mode 100644 src/Components/Samples/BlazorServerApp/Shared/ErrorCausingChild.razor diff --git a/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor b/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor index 50b1f2defa6f..fbcbe2b0054b 100644 --- a/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor +++ b/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor @@ -8,9 +8,26 @@ + +
    + Lifecycle methods +
    OnParametersSet (sync)
    +
    OnParametersSetAsync (async)
    +
    OnAfterRender (sync)
    +
    OnAfterRenderAsync (async)
    + +
    @code { + private bool throwInOnParametersSet; + private bool throwInOnParametersSetAsync; + private bool throwInOnAfterRender; + private bool throwInOnAfterRenderAsync; + void EventHandlerErrorSync() => throw new InvalidTimeZoneException("Synchronous error from event handler"); diff --git a/src/Components/Samples/BlazorServerApp/Shared/ErrorCausingChild.razor b/src/Components/Samples/BlazorServerApp/Shared/ErrorCausingChild.razor new file mode 100644 index 000000000000..de0fc76720b6 --- /dev/null +++ b/src/Components/Samples/BlazorServerApp/Shared/ErrorCausingChild.razor @@ -0,0 +1,42 @@ +@code { + [Parameter] public bool ThrowOnParametersSet { get; set; } + [Parameter] public bool ThrowOnParametersSetAsync { get; set; } + [Parameter] public bool ThrowOnAfterRender { get; set; } + [Parameter] public bool ThrowOnAfterRenderAsync { get; set; } + + protected override void OnParametersSet() + { + if (ThrowOnParametersSet) + { + throw new InvalidTimeZoneException("Synchronous exception in OnParametersSet"); + } + } + + protected override async Task OnParametersSetAsync() + { + await Task.Yield(); + + if (ThrowOnParametersSetAsync) + { + throw new InvalidTimeZoneException("Asynchronous exception in OnParametersSetAsync"); + } + } + + protected override void OnAfterRender(bool firstRender) + { + if (ThrowOnAfterRender) + { + throw new InvalidTimeZoneException("Synchronous exception in OnAfterRender"); + } + } + + protected override async Task OnAfterRenderAsync(bool firstRender) + { + await Task.Yield(); + + if (ThrowOnAfterRenderAsync) + { + throw new InvalidTimeZoneException("Asynchronous exception in OnAfterRenderAsync"); + } + } +} From cc44e53d92bdc04409d25bbc4d29cd1c5851b95d Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Fri, 12 Mar 2021 11:28:31 +0000 Subject: [PATCH 04/55] Handle SetParametersAsync exceptions --- .../Components/src/RenderTree/Renderer.cs | 19 +++++++++++++------ .../src/Rendering/ComponentState.cs | 17 +++++++++++++++-- 2 files changed, 28 insertions(+), 8 deletions(-) diff --git a/src/Components/Components/src/RenderTree/Renderer.cs b/src/Components/Components/src/RenderTree/Renderer.cs index 88e75e42c66b..3b9d8b885c2e 100644 --- a/src/Components/Components/src/RenderTree/Renderer.cs +++ b/src/Components/Components/src/RenderTree/Renderer.cs @@ -352,8 +352,7 @@ public virtual Task DispatchEventAsync(ulong eventHandlerId, EventFieldInfo? fie // This is pretty subjective. Even in the "safe" case, it's possible that some non-component // (domain model) state has been left corrupted. But we have to let developers be responsible // for keeping their non-component state valid otherwise we couldn't have any error recovery. - var recovered = componentReceiver is not null - && TrySendExceptionToErrorBoundary(componentReceiver, e); + var recovered = TrySendExceptionToErrorBoundary(componentReceiver, e); if (!recovered) { HandleException(e); @@ -376,8 +375,13 @@ public virtual Task DispatchEventAsync(ulong eventHandlerId, EventFieldInfo? fie return result; } - private bool TrySendExceptionToErrorBoundary(IComponent errorSource, Exception error) + private bool TrySendExceptionToErrorBoundary(IComponent? errorSource, Exception error) { + if (errorSource is null) + { + return false; + } + // TODO: We probably need to ensure calls to this method are dispatched Dispatcher.AssertAccess(); @@ -465,8 +469,11 @@ internal void AddToPendingTasks(Task task, IComponent? owningComponent) // an 'async' state machine (the ones generated using async/await) where even // the synchronous exceptions will get captured and converted into a faulted // task. - // TODO: Presumably we need to use TrySendExceptionToErrorBoundary here - HandleException(task.Exception.GetBaseException()); + var baseException = task.Exception.GetBaseException(); + if (!TrySendExceptionToErrorBoundary(owningComponent, baseException)) + { + HandleException(baseException); + } break; default: // It's important to evaluate the following even if we're not going to use @@ -889,7 +896,7 @@ private async Task GetErrorHandledTask(Task taskToHandle, IComponent? owningComp // Ignore errors due to task cancellations. if (!taskToHandle.IsCanceled) { - if (owningComponent is null || !TrySendExceptionToErrorBoundary(owningComponent, ex)) + if (!TrySendExceptionToErrorBoundary(owningComponent, ex)) { // It's unhandled HandleException(ex); diff --git a/src/Components/Components/src/Rendering/ComponentState.cs b/src/Components/Components/src/Rendering/ComponentState.cs index ae4cf25fe7e3..ba062833d6a7 100644 --- a/src/Components/Components/src/Rendering/ComponentState.cs +++ b/src/Components/Components/src/Rendering/ComponentState.cs @@ -164,8 +164,21 @@ public void SetDirectParameters(ParameterView parameters) parameters = parameters.WithCascadingParameters(_cascadingParameters); } - // TODO: Think about passing self as owningComponent for error recovery - _renderer.AddToPendingTasks(Component.SetParametersAsync(parameters), null); + // Normalise sync and async exceptions into a Task, as there's no value in differentiating + // between "it returned an already-faulted Task" and "it just threw". + // TODO: This is the most invasive part of this whole work area on perf. Need to validate + // that this doesn't regress WebAssembly rendering perf too much in intense cases. + Task setParametersAsyncTask; + try + { + setParametersAsyncTask = Component.SetParametersAsync(parameters); + } + catch (Exception ex) + { + setParametersAsyncTask = Task.FromException(ex); + } + + _renderer.AddToPendingTasks(setParametersAsyncTask, Component); } public void NotifyCascadingValueChanged(in ParameterViewLifetime lifetime) From 53510894ef8ec8647b976d53ccf787fc08eaf9cf Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Fri, 12 Mar 2021 11:34:51 +0000 Subject: [PATCH 05/55] Handle IHandleAfterRender exceptions --- src/Components/Components/src/RenderTree/Renderer.cs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/Components/Components/src/RenderTree/Renderer.cs b/src/Components/Components/src/RenderTree/Renderer.cs index 3b9d8b885c2e..a0c66fac9242 100644 --- a/src/Components/Components/src/RenderTree/Renderer.cs +++ b/src/Components/Components/src/RenderTree/Renderer.cs @@ -759,7 +759,10 @@ private void NotifyRenderCompleted(ComponentState state, ref List batch) } else if (task.Status == TaskStatus.Faulted) { - HandleException(task.Exception); + if (!TrySendExceptionToErrorBoundary(state.Component, task.Exception)) + { + HandleException(task.Exception); + } return; } } @@ -767,7 +770,7 @@ private void NotifyRenderCompleted(ComponentState state, ref List batch) // The Task is incomplete. // Queue up the task and we can inspect it later. batch = batch ?? new List(); - batch.Add(GetErrorHandledTask(task, null)); // TODO: Should there be an owningComponent? + batch.Add(GetErrorHandledTask(task, state.Component)); } private void RenderInExistingBatch(RenderQueueEntry renderQueueEntry) From 4e7e3c54895780620c69fa93963d7ea849b7bee6 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Fri, 12 Mar 2021 13:17:42 +0000 Subject: [PATCH 06/55] Handle exceptions during rendering --- .../Components/src/RenderTree/Renderer.cs | 8 +++- .../src/Rendering/ComponentState.cs | 41 ++++++++++++++----- .../BlazorServerApp/Pages/ErrorMaker.razor | 23 +++++++++-- 3 files changed, 57 insertions(+), 15 deletions(-) diff --git a/src/Components/Components/src/RenderTree/Renderer.cs b/src/Components/Components/src/RenderTree/Renderer.cs index a0c66fac9242..0867ac7f1b76 100644 --- a/src/Components/Components/src/RenderTree/Renderer.cs +++ b/src/Components/Components/src/RenderTree/Renderer.cs @@ -8,6 +8,7 @@ using System.Diagnostics; using System.Diagnostics.CodeAnalysis; using System.Linq; +using System.Runtime.ExceptionServices; using System.Threading; using System.Threading.Tasks; using Microsoft.AspNetCore.Components.HotReload; @@ -777,7 +778,12 @@ private void RenderInExistingBatch(RenderQueueEntry renderQueueEntry) { var componentState = renderQueueEntry.ComponentState; Log.RenderingComponent(_logger, componentState); - componentState.RenderIntoBatch(_batchBuilder, renderQueueEntry.RenderFragment); + componentState.RenderIntoBatch(_batchBuilder, renderQueueEntry.RenderFragment, out var renderFragmentException); + if (renderFragmentException != null && !TrySendExceptionToErrorBoundary(componentState.Component, renderFragmentException)) + { + // Exception from render fragment could not be handled by an error boundary, so treat as unhandled (fatal) + ExceptionDispatchInfo.Capture(renderFragmentException).Throw(); + } List exceptions = null; diff --git a/src/Components/Components/src/Rendering/ComponentState.cs b/src/Components/Components/src/Rendering/ComponentState.cs index ba062833d6a7..341b163956bf 100644 --- a/src/Components/Components/src/Rendering/ComponentState.cs +++ b/src/Components/Components/src/Rendering/ComponentState.cs @@ -20,7 +20,7 @@ internal class ComponentState : IDisposable private readonly IReadOnlyList _cascadingParameters; private readonly bool _hasCascadingParameters; private readonly bool _hasAnyCascadingParameterSubscriptions; - private RenderTreeBuilder _renderTreeBuilderPrevious; + private RenderTreeBuilder _nextRenderTree; private ArrayBuilder? _latestDirectParametersSnapshot; // Lazily instantiated private bool _componentWasDisposed; @@ -39,7 +39,7 @@ public ComponentState(Renderer renderer, int componentId, IComponent component, _renderer = renderer ?? throw new ArgumentNullException(nameof(renderer)); _cascadingParameters = CascadingParameterState.FindCascadingParameters(this); CurrentRenderTree = new RenderTreeBuilder(); - _renderTreeBuilderPrevious = new RenderTreeBuilder(); + _nextRenderTree = new RenderTreeBuilder(); if (_cascadingParameters.Count != 0) { @@ -54,8 +54,10 @@ public ComponentState(Renderer renderer, int componentId, IComponent component, public ComponentState ParentComponentState { get; } public RenderTreeBuilder CurrentRenderTree { get; private set; } - public void RenderIntoBatch(RenderBatchBuilder batchBuilder, RenderFragment renderFragment) + public void RenderIntoBatch(RenderBatchBuilder batchBuilder, RenderFragment renderFragment, out Exception? renderFragmentException) { + renderFragmentException = null; + // A component might be in the render queue already before getting disposed by an // earlier entry in the render queue. In that case, rendering is a no-op. if (_componentWasDisposed) @@ -63,19 +65,38 @@ public void RenderIntoBatch(RenderBatchBuilder batchBuilder, RenderFragment rend return; } - // Swap the old and new tree builders - (CurrentRenderTree, _renderTreeBuilderPrevious) = (_renderTreeBuilderPrevious, CurrentRenderTree); + _nextRenderTree.Clear(); - CurrentRenderTree.Clear(); - renderFragment(CurrentRenderTree); + try + { + renderFragment(_nextRenderTree); + } + catch (Exception ex) + { + // If an exception occurs in the render fragment delegate, we won't process the diff in any way, so child components, + // event handlers, etc., will all be left untouched as if this component didn't re-render at all. We also are careful + // to leave ComponentState in an internally-consistent state so that technically this component could continue to be + // used. + // TODO: Verify that having a try/catch here doesn't degrade perf noticeably on WebAssembly. It might do. + // TODO: Should we actually terminate this component in some way to guarantee it doesn't re-render? That's tricky because + // we need it to be disposed in order that descendants, event handlers, etc. get cleaned up too. Unless we rely on it + // getting removed from its parent, we would have to duplicate quite a bit of internal state-keeping to ensure all the + // descendant state gets cleared up. + renderFragmentException = ex; + return; + } - CurrentRenderTree.AssertTreeIsValid(Component); + // We don't want to make errors from this be recoverable, because there's no legitimate reason for them to happen + _nextRenderTree.AssertTreeIsValid(Component); + + // Swap the old and new tree builders + (CurrentRenderTree, _nextRenderTree) = (_nextRenderTree, CurrentRenderTree); var diff = RenderTreeDiffBuilder.ComputeDiff( _renderer, batchBuilder, ComponentId, - _renderTreeBuilderPrevious.GetFrames(), + _nextRenderTree.GetFrames(), CurrentRenderTree.GetFrames()); batchBuilder.UpdatedComponentDiffs.Append(diff); batchBuilder.InvalidateParameterViews(); @@ -236,7 +257,7 @@ public void Dispose() private void DisposeBuffers() { - ((IDisposable)_renderTreeBuilderPrevious).Dispose(); + ((IDisposable)_nextRenderTree).Dispose(); ((IDisposable)CurrentRenderTree).Dispose(); _latestDirectParametersSnapshot?.Dispose(); } diff --git a/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor b/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor index fbcbe2b0054b..b6e1b5537a9a 100644 --- a/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor +++ b/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor @@ -11,22 +11,37 @@
    Lifecycle methods -
    OnParametersSet (sync)
    -
    OnParametersSetAsync (async)
    -
    OnAfterRender (sync)
    -
    OnAfterRenderAsync (async)
    +
    +
    +
    +
    + +
    + Rendering + + @if (throwWhileRendering) + { + throw new InvalidTimeZoneException($"Exception from {nameof(BuildRenderTree)}"); + } +
    @code { + // Also consider: + // - Attach + // - Dispose/DisposeAsync + // - InvokeAsync + private bool throwInOnParametersSet; private bool throwInOnParametersSetAsync; private bool throwInOnAfterRender; private bool throwInOnAfterRenderAsync; + private bool throwWhileRendering; void EventHandlerErrorSync() => throw new InvalidTimeZoneException("Synchronous error from event handler"); From e6c1c1ce5c7e296b86680d75aed2e42c36935594 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Fri, 12 Mar 2021 14:12:03 +0000 Subject: [PATCH 07/55] Update notes about disposal --- .../Samples/BlazorServerApp/Pages/ErrorMaker.razor | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor b/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor index b6e1b5537a9a..feef9227874b 100644 --- a/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor +++ b/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor @@ -34,8 +34,16 @@ @code { // Also consider: // - Attach - // - Dispose/DisposeAsync // - InvokeAsync + // - SetParametersAsync being triggered by a cascading parameter subscription + + // After a lot of consideration, I don't think we should allow exceptions during Dispose/DisposeAsync to be recoverable. + // We could do so, since components are only disposed once they are removed from their parents, so even if their disposal + // fails we know they are gone from the component hierarchy anyway, and we can still be sure to clean up all the framework + // managed resources for them and their descendants. However, if disposal throws, then it's very likely that the application + // cleanup logic didn't complete properly, and hence the application is now leaking memory. It's not smart to have a default + // behavior that lets the developer get away with randomly incomplete cleanup. If a developer really wants to opt into this + // on a per-Dispose/DisposeAsync basis, they can use a try/catch there, and in most cases they should not do so. private bool throwInOnParametersSet; private bool throwInOnParametersSetAsync; From 0d6acd7b0e66c57ab9f4a9bd4d3fd75d3ddccf0b Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Fri, 12 Mar 2021 14:24:36 +0000 Subject: [PATCH 08/55] Add note about Attach --- .../Samples/BlazorServerApp/Pages/ErrorMaker.razor | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor b/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor index feef9227874b..9cab456d729d 100644 --- a/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor +++ b/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor @@ -33,10 +33,20 @@ @code { // Also consider: - // - Attach // - InvokeAsync // - SetParametersAsync being triggered by a cascading parameter subscription + // I'm proposing not to allow handling exceptions that occur during either IComponentActivator.CreateInstance or + // IComponent.Attach, because: + // [1] For most applications, these won't run any developer-supplied code, and will only run the framework's + // built-in logic (e.g., ComponentBase.Attach which can't throw). The value here is pretty low. + // [2] Supporting recovery from these is difficult and leads to entirely new states, such as a "component" RenderTreeFrame + // not actually having an associated ComponentState because we never initialized it in the first place. + // These particular lifecycle points are outside the scope of error recovery that could be achieved in user code today + // (via the "cascading a custom ErrorBoundary-type component" technique), so are outside the scope of this work. We retain + // the option of making component initialization errors recoverable in the future, as I think it's fair to say that would + // not be a breaking change, as we'd be replacing a previously-fatal error with a nonfatal one. + // After a lot of consideration, I don't think we should allow exceptions during Dispose/DisposeAsync to be recoverable. // We could do so, since components are only disposed once they are removed from their parents, so even if their disposal // fails we know they are gone from the component hierarchy anyway, and we can still be sure to clean up all the framework From 2b35fe8e140fc64d9fa3b3ea30a2425a1e9f6a71 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Fri, 12 Mar 2021 14:41:39 +0000 Subject: [PATCH 09/55] Handle exceptions during cascading value notifications --- .../src/Rendering/ComponentState.cs | 31 +++++++++++-------- .../BlazorServerApp/Pages/ErrorMaker.razor | 21 ++++++++++--- .../Shared/ErrorCausingChild.razor | 10 ++++-- 3 files changed, 42 insertions(+), 20 deletions(-) diff --git a/src/Components/Components/src/Rendering/ComponentState.cs b/src/Components/Components/src/Rendering/ComponentState.cs index 341b163956bf..75bba063bdc6 100644 --- a/src/Components/Components/src/Rendering/ComponentState.cs +++ b/src/Components/Components/src/Rendering/ComponentState.cs @@ -185,6 +185,23 @@ public void SetDirectParameters(ParameterView parameters) parameters = parameters.WithCascadingParameters(_cascadingParameters); } + SupplyCombinedParameters(parameters); + } + + public void NotifyCascadingValueChanged(in ParameterViewLifetime lifetime) + { + var directParams = _latestDirectParametersSnapshot != null + ? new ParameterView(lifetime, _latestDirectParametersSnapshot.Buffer, 0) + : ParameterView.Empty; + var allParams = directParams.WithCascadingParameters(_cascadingParameters!); + SupplyCombinedParameters(allParams); + } + + // This should not be called from anywhere except SetDirectParameters or NotifyCascadingValueChanged. + // Those two methods know how to correctly combine both cascading and non-cascading parameters to supply + // a consistent set to the recipient. + private void SupplyCombinedParameters(ParameterView directAndCascadingParameters) + { // Normalise sync and async exceptions into a Task, as there's no value in differentiating // between "it returned an already-faulted Task" and "it just threw". // TODO: This is the most invasive part of this whole work area on perf. Need to validate @@ -192,7 +209,7 @@ public void SetDirectParameters(ParameterView parameters) Task setParametersAsyncTask; try { - setParametersAsyncTask = Component.SetParametersAsync(parameters); + setParametersAsyncTask = Component.SetParametersAsync(directAndCascadingParameters); } catch (Exception ex) { @@ -202,18 +219,6 @@ public void SetDirectParameters(ParameterView parameters) _renderer.AddToPendingTasks(setParametersAsyncTask, Component); } - public void NotifyCascadingValueChanged(in ParameterViewLifetime lifetime) - { - var directParams = _latestDirectParametersSnapshot != null - ? new ParameterView(lifetime, _latestDirectParametersSnapshot.Buffer, 0) - : ParameterView.Empty; - var allParams = directParams.WithCascadingParameters(_cascadingParameters!); - var task = Component.SetParametersAsync(allParams); - - // TODO: Think about passing self as owningComponent for error recovery - _renderer.AddToPendingTasks(task, null); - } - private bool AddCascadingParameterSubscriptions() { var hasSubscription = false; diff --git a/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor b/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor index 9cab456d729d..3d5c0fb3be6f 100644 --- a/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor +++ b/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor @@ -13,13 +13,22 @@ Lifecycle methods
    +
    +
    - - + + + + + + +
    Rendering @@ -57,6 +66,8 @@ private bool throwInOnParametersSet; private bool throwInOnParametersSetAsync; + private bool throwInOnParametersSetViaCascading; + private bool throwInOnParametersSetAsyncViaCascading; private bool throwInOnAfterRender; private bool throwInOnAfterRenderAsync; private bool throwWhileRendering; diff --git a/src/Components/Samples/BlazorServerApp/Shared/ErrorCausingChild.razor b/src/Components/Samples/BlazorServerApp/Shared/ErrorCausingChild.razor index de0fc76720b6..876ed2170ef3 100644 --- a/src/Components/Samples/BlazorServerApp/Shared/ErrorCausingChild.razor +++ b/src/Components/Samples/BlazorServerApp/Shared/ErrorCausingChild.razor @@ -4,9 +4,15 @@ [Parameter] public bool ThrowOnAfterRender { get; set; } [Parameter] public bool ThrowOnAfterRenderAsync { get; set; } + [CascadingParameter(Name = nameof(ThrowOnCascadingParameterNotification))] + public bool ThrowOnCascadingParameterNotification { get; set; } + + [CascadingParameter(Name = nameof(ThrowOnCascadingParameterNotificationAsync))] + public bool ThrowOnCascadingParameterNotificationAsync { get; set; } + protected override void OnParametersSet() { - if (ThrowOnParametersSet) + if (ThrowOnParametersSet || ThrowOnCascadingParameterNotification) { throw new InvalidTimeZoneException("Synchronous exception in OnParametersSet"); } @@ -16,7 +22,7 @@ { await Task.Yield(); - if (ThrowOnParametersSetAsync) + if (ThrowOnParametersSetAsync || ThrowOnCascadingParameterNotificationAsync) { throw new InvalidTimeZoneException("Asynchronous exception in OnParametersSetAsync"); } From 42d1911885a90d5a04aba7454146b954230a70a8 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Fri, 12 Mar 2021 14:42:01 +0000 Subject: [PATCH 10/55] Update ErrorMaker.razor --- src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor b/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor index 3d5c0fb3be6f..7bd3df803724 100644 --- a/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor +++ b/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor @@ -43,7 +43,6 @@ @code { // Also consider: // - InvokeAsync - // - SetParametersAsync being triggered by a cascading parameter subscription // I'm proposing not to allow handling exceptions that occur during either IComponentActivator.CreateInstance or // IComponent.Attach, because: From 2f68770da500a8d75a53a59e038c6e64208c385c Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Fri, 12 Mar 2021 15:04:01 +0000 Subject: [PATCH 11/55] Add note about InvokeAsync --- .../BlazorServerApp/Pages/ErrorMaker.razor | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor b/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor index 7bd3df803724..c7ed31eddec9 100644 --- a/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor +++ b/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor @@ -28,7 +28,7 @@ ThrowOnAfterRenderAsync="@throwInOnAfterRenderAsync" /> -
    +
    Rendering @@ -41,11 +41,18 @@
    @code { - // Also consider: - // - InvokeAsync + // Note that unhandled exceptions inside InvokeAsync are already non-fatal, since we simply return them to the + // upstream caller who decides what to do. We lack any API for transferring execution into the sync context + // that returns void and takes care of its own error handling (besides the complicated trick of stashing an + // exception in a field and triggering a render that then throws during rendering). If we add such an API + // in the future, it would be a method on ComponentBase that internally does InvokeAsync to a delegate that + // does try/catch around your Action/Func, and if there's an exception, calls some new method on RenderHandle + // that passes the exception and IComponent instance onto the Renderer, which then uses TrySendExceptionToErrorBoundary + // and falls back on HandleError (fatal). We could call it something like Execute/ExecuteAsync (both void), + // or maybe InvokeAndHandle/InvokeAndHandleAsync (both void). - // I'm proposing not to allow handling exceptions that occur during either IComponentActivator.CreateInstance or - // IComponent.Attach, because: + // I'm proposing not to allow handling exceptions that occur during either IComponentActivator.CreateInstance + // (e.g., component constructor) or IComponent.Attach, because: // [1] For most applications, these won't run any developer-supplied code, and will only run the framework's // built-in logic (e.g., ComponentBase.Attach which can't throw). The value here is pretty low. // [2] Supporting recovery from these is difficult and leads to entirely new states, such as a "component" RenderTreeFrame From 80ebf30f099372ce09a4449c58f66f76b180715d Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Fri, 12 Mar 2021 15:19:21 +0000 Subject: [PATCH 12/55] Clean up some notes --- .../Components/src/RenderTree/Renderer.cs | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/Components/Components/src/RenderTree/Renderer.cs b/src/Components/Components/src/RenderTree/Renderer.cs index 0867ac7f1b76..f6ecd0609f8f 100644 --- a/src/Components/Components/src/RenderTree/Renderer.cs +++ b/src/Components/Components/src/RenderTree/Renderer.cs @@ -383,29 +383,29 @@ private bool TrySendExceptionToErrorBoundary(IComponent? errorSource, Exception return false; } - // TODO: We probably need to ensure calls to this method are dispatched + // We only get here in specific situations. Currently, all of them are when we're + // already on the sync context (and if not, we have a bug we want to know about). Dispatcher.AssertAccess(); - // This linear search will be slow, but only happens during exception handling, so is likely OK. + // Slow linear search is fine because this is only during exception handling. // If it becomes problematic, we can maintain a lookup from component instance to ID. var componentState = _componentStateById.FirstOrDefault(kvp => kvp.Value.Component == errorSource).Value; - // Find the closest ancestor that's an immediate child of an error boundary. This is the component - // that will be terminated. - while (componentState is not null) + // Find the closest IErrorBoundary, if any + while (componentState is not null) // Could be null initially if the errorSource was already disposed { - if (componentState.ParentComponentState?.Component is IErrorBoundary errorBoundary) + if (componentState.Component is IErrorBoundary errorBoundary) { - // TODO: Is it OK that we're trusting the IErrorBoundary to remove its previous child - // content? What if it fails to do so? We could proactively dispose componentState.Component - // within the current batch here if we wanted, but that does complicate things. Until now, - // we only dispose components during diffing, and there's an error if diffing tries to queue a - // disposal for something that's already disposed. - // Overall I think it's probably OK to let the IErrorBoundary make its own choice. Almost - // everyone will have the default behaviors, and even if someone leaves the broken component - // in place, we will already have done some level of recovery close to the call site. - // Also it's quite complicate to proactively dispose any component here, because we're not - // necessarily inside an in-progress render batch. + // Normally we expect the IErrorBoundary to remove its previous child content on error, + // thereby cleaning up all the resources in its subtree. However, error boundaries are + // allowed to ignore errors and leave the child content in place. In part this is because + // it's hard to stop them from doing so, but also because the only kinds of exceptions + // we send to them are truly recoverable ones where the framework ensures that no framework + // state is left corrupted and all resources can still be cleaned up. + // TODO: What if, in the future, we do need to guarantee that the failed subtree gets + // removed? That would be a breaking change. Should we somehow ensure it gets removed + // now, just to mitigate this future risk, even though it's not necessary today? + // It's unclear how we'd do this without breaking normal rendering rules. errorBoundary.HandleException(error); return true; } From 83de66738d2d51d6d271fb31aebd36aa7f7d0c52 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Fri, 12 Mar 2021 15:32:00 +0000 Subject: [PATCH 13/55] Clarify what we do if the IErrorBoundary component itself has an error --- .../Components/src/RenderTree/Renderer.cs | 22 +++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/src/Components/Components/src/RenderTree/Renderer.cs b/src/Components/Components/src/RenderTree/Renderer.cs index f6ecd0609f8f..9102ce092c5d 100644 --- a/src/Components/Components/src/RenderTree/Renderer.cs +++ b/src/Components/Components/src/RenderTree/Renderer.cs @@ -391,8 +391,11 @@ private bool TrySendExceptionToErrorBoundary(IComponent? errorSource, Exception // If it becomes problematic, we can maintain a lookup from component instance to ID. var componentState = _componentStateById.FirstOrDefault(kvp => kvp.Value.Component == errorSource).Value; - // Find the closest IErrorBoundary, if any - while (componentState is not null) // Could be null initially if the errorSource was already disposed + // Find the closest IErrorBoundary, if any. Start looking at the first parent, to ensure + // that an error boundary component doesn't try to handle its own errors, as that could + // be an infinite loop. + componentState = componentState?.ParentComponentState; + while (componentState is not null) { if (componentState.Component is IErrorBoundary errorBoundary) { @@ -406,8 +409,19 @@ private bool TrySendExceptionToErrorBoundary(IComponent? errorSource, Exception // removed? That would be a breaking change. Should we somehow ensure it gets removed // now, just to mitigate this future risk, even though it's not necessary today? // It's unclear how we'd do this without breaking normal rendering rules. - errorBoundary.HandleException(error); - return true; + try + { + errorBoundary.HandleException(error); + return true; + } + catch (Exception errorBoundaryException) + { + // If *notifying* about an exception fails, it's OK for that to be fatal. This is + // different from an IErrorBoundary component throwing during its own rendering or + // lifecycle methods. + HandleException(errorBoundaryException); + return false; + } } componentState = componentState.ParentComponentState; From 2a5a7d30c100e0b8f34384f7b9e5400badf1c9e3 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Fri, 12 Mar 2021 16:43:52 +0000 Subject: [PATCH 14/55] Force error boundaries to clear up their descendants on error, even before we notify them. Also think about infinite error loops. --- .../Components/src/IErrorBoundary.cs | 11 ++++++++ .../Components/src/RenderTree/Renderer.cs | 26 ++++++++++--------- .../BlazorServerApp/Pages/ErrorMaker.razor | 19 ++++++++++++++ .../Shared/ErrorCausingCounter.razor | 14 ++++++++++ .../BlazorServerApp/Shared/ErrorIgnorer.razor | 18 +++++++++++++ 5 files changed, 76 insertions(+), 12 deletions(-) create mode 100644 src/Components/Samples/BlazorServerApp/Shared/ErrorCausingCounter.razor create mode 100644 src/Components/Samples/BlazorServerApp/Shared/ErrorIgnorer.razor diff --git a/src/Components/Components/src/IErrorBoundary.cs b/src/Components/Components/src/IErrorBoundary.cs index b648239706cb..13b128bf0b46 100644 --- a/src/Components/Components/src/IErrorBoundary.cs +++ b/src/Components/Components/src/IErrorBoundary.cs @@ -7,6 +7,17 @@ namespace Microsoft.AspNetCore.Components { public interface IErrorBoundary { + // Warning: if you make an IErrorBoundary that reacts to errors by rendering some child + // that throws during its own rendering, you will have an infinite loop of errors. How could + // we detect this? As long as it's legit to recover, then you have to deal with a succession + // of valid (non-infinite-looping) errors over time. We can't detect this by looking at the + // call stack, because a child might trigger an error asynchronously, making it into a slightly + // spaced out but still infinite loop. + // + // It would be weird, but we could track the timestamp of the last error, and if another one + // occurs in < 100ms, then treat it as fatal. Technically we could do this for all IErrorBoundary + // (not just the .Web one) by stashing that info in ComponentState, at the cost of using more + // memory for every component, not just IErrorBoundary. void HandleException(Exception exception); } } diff --git a/src/Components/Components/src/RenderTree/Renderer.cs b/src/Components/Components/src/RenderTree/Renderer.cs index 9102ce092c5d..a02a0acaf6ae 100644 --- a/src/Components/Components/src/RenderTree/Renderer.cs +++ b/src/Components/Components/src/RenderTree/Renderer.cs @@ -399,20 +399,21 @@ private bool TrySendExceptionToErrorBoundary(IComponent? errorSource, Exception { if (componentState.Component is IErrorBoundary errorBoundary) { - // Normally we expect the IErrorBoundary to remove its previous child content on error, - // thereby cleaning up all the resources in its subtree. However, error boundaries are - // allowed to ignore errors and leave the child content in place. In part this is because - // it's hard to stop them from doing so, but also because the only kinds of exceptions - // we send to them are truly recoverable ones where the framework ensures that no framework - // state is left corrupted and all resources can still be cleaned up. - // TODO: What if, in the future, we do need to guarantee that the failed subtree gets - // removed? That would be a breaking change. Should we somehow ensure it gets removed - // now, just to mitigate this future risk, even though it's not necessary today? - // It's unclear how we'd do this without breaking normal rendering rules. + // Force the IErrorBoundary component to clear its output, regardless of any logic inside + // that component. This ensures that all descendants are cleaned up. We don't strictly have + // to do this, since the only errors we handle this way are actually recoverable, so technically + // it would be OK if the old output was left in place and continued operating. However that + // would be a whole new way of keeping components running after failure which we don't want + // to introduce and guarantee to support forever. + AddToRenderQueue(componentState.ComponentId, builder => { }); + try { + // TODO: Should we log the error here? Currently it's up to the IErrorBoundary to do what + // it wants, including hiding the error. Maybe we should force it to get logged regardless + // of what the IErrorBoundary want to do. Whichever way we go on this is permanent, as + // switching in either direction would be a breaking change. errorBoundary.HandleException(error); - return true; } catch (Exception errorBoundaryException) { @@ -420,8 +421,9 @@ private bool TrySendExceptionToErrorBoundary(IComponent? errorSource, Exception // different from an IErrorBoundary component throwing during its own rendering or // lifecycle methods. HandleException(errorBoundaryException); - return false; } + + return true; } componentState = componentState.ParentComponentState; diff --git a/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor b/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor index c7ed31eddec9..e1927fef1a4e 100644 --- a/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor +++ b/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor @@ -38,9 +38,28 @@ throw new InvalidTimeZoneException($"Exception from {nameof(BuildRenderTree)}"); } + +
    + Trying to ignore errors + + + +
    @code { + // A weird subtlety is what happens if you do: + // Some inline content that may throw, either in a event-handling lambda or on render + // Syntactically it looks as if the error boundary will take care of errors in that content inside itself, but + // actually: + // [1] Any error-throwing lambdas in there are mapped to the host component outside the , since + // that's where the delegate lives. So those errors will not be handled by this error boundary. + // [2] Any rendering errors in there are explicitly *not* dispatched to that because it would + // commonly be an infinite error loop. So again, those errors will go up to any boundaries in ancestors. + // People will have to understand that error boundaries are responsible for their descendants, not for themselves. + // However on consideration I think we should probably change rule [2], as long as there is some mechanism to + // detect infinite error loops. + // Note that unhandled exceptions inside InvokeAsync are already non-fatal, since we simply return them to the // upstream caller who decides what to do. We lack any API for transferring execution into the sync context // that returns void and takes care of its own error handling (besides the complicated trick of stashing an diff --git a/src/Components/Samples/BlazorServerApp/Shared/ErrorCausingCounter.razor b/src/Components/Samples/BlazorServerApp/Shared/ErrorCausingCounter.razor new file mode 100644 index 000000000000..efc03677955e --- /dev/null +++ b/src/Components/Samples/BlazorServerApp/Shared/ErrorCausingCounter.razor @@ -0,0 +1,14 @@ +

    + Count: @currentCount + + +

    + +@code { + private int currentCount; + + private void Throw() + { + throw new InvalidTimeZoneException($"Exception from {nameof(ErrorCausingCounter)}"); + } +} diff --git a/src/Components/Samples/BlazorServerApp/Shared/ErrorIgnorer.razor b/src/Components/Samples/BlazorServerApp/Shared/ErrorIgnorer.razor new file mode 100644 index 000000000000..84477c3c2e4a --- /dev/null +++ b/src/Components/Samples/BlazorServerApp/Shared/ErrorIgnorer.razor @@ -0,0 +1,18 @@ +@implements IErrorBoundary +@ChildContent +@code { + // This represents a badly-behaved IErrorBoundary that tries to keep rendering the + // same content even after an error. Error boundaries shouldn't do this since it + // risks an infinite rendering loop if the child content throws immediately on + // rendering. + + [Parameter] public RenderFragment? ChildContent { get; set; } + + public void HandleException(Exception ex) + { + Console.WriteLine($"Got an error, but will try to pretend it didn't happen: {ex.Message}"); + + // Trying to bring back the child content + StateHasChanged(); + } +} From 0cd43ffd02094b0fd7c5a77ad7b809d95af80a5b Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Fri, 12 Mar 2021 16:48:09 +0000 Subject: [PATCH 15/55] More thoughts on error loops --- src/Components/Components/src/IErrorBoundary.cs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/Components/Components/src/IErrorBoundary.cs b/src/Components/Components/src/IErrorBoundary.cs index 13b128bf0b46..9ea183bdd59c 100644 --- a/src/Components/Components/src/IErrorBoundary.cs +++ b/src/Components/Components/src/IErrorBoundary.cs @@ -18,6 +18,12 @@ public interface IErrorBoundary // occurs in < 100ms, then treat it as fatal. Technically we could do this for all IErrorBoundary // (not just the .Web one) by stashing that info in ComponentState, at the cost of using more // memory for every component, not just IErrorBoundary. + // + // Another option is to handle this in the .Web one using the rule "if you notify me of an error + // while I believe I'm already showing an error, then I'll just rethrow so it gets treated as + // fatal for the whole circuit". This would take care of errors caused when rendering ErrorContent. + // This might be the best solution. People implementing a custom IErrorBoundary should take care + // to put in similar logic, if we're OK relying on that. void HandleException(Exception exception); } } From 0a7a07f9daf42622f557b8b4d17920b02c0642e2 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Fri, 12 Mar 2021 16:53:53 +0000 Subject: [PATCH 16/55] More thoughts --- src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor b/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor index e1927fef1a4e..d7063c8fd487 100644 --- a/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor +++ b/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor @@ -80,6 +80,8 @@ // (via the "cascading a custom ErrorBoundary-type component" technique), so are outside the scope of this work. We retain // the option of making component initialization errors recoverable in the future, as I think it's fair to say that would // not be a breaking change, as we'd be replacing a previously-fatal error with a nonfatal one. + // - Actually we don't need to handle it for these anyway, because in these cases the exception will just bubble + // up to the SetParametersAsync on the parent, and become a handleable error at that level. // After a lot of consideration, I don't think we should allow exceptions during Dispose/DisposeAsync to be recoverable. // We could do so, since components are only disposed once they are removed from their parents, so even if their disposal From 8089c36a26b1c57c1324e339d048b0c7e109ac7d Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Mon, 15 Mar 2021 12:01:03 +0000 Subject: [PATCH 17/55] Clean up all the stuff about detailed errors and prerendering --- .../BlazorServerApp/Shared/MainLayout.razor | 2 +- .../src/Circuits/RemoteErrorBoundaryLogger.cs | 57 ++++++++++++++ .../Server/src/Circuits/RemoteJSRuntime.cs | 2 + .../ComponentServiceCollectionExtensions.cs | 2 + .../Web/src/PublicAPI.Unshipped.txt | 10 +-- src/Components/Web/src/Web/ErrorBoundary.cs | 78 ++++++------------- .../Web/src/Web/IErrorBoundaryLogger.cs | 28 +++++++ .../src/Hosting/WebAssemblyHostBuilder.cs | 1 + .../WebAssemblyErrorBoundaryLogger.cs | 28 +++++++ 9 files changed, 146 insertions(+), 62 deletions(-) create mode 100644 src/Components/Server/src/Circuits/RemoteErrorBoundaryLogger.cs create mode 100644 src/Components/Web/src/Web/IErrorBoundaryLogger.cs create mode 100644 src/Components/WebAssembly/WebAssembly/src/Services/WebAssemblyErrorBoundaryLogger.cs diff --git a/src/Components/Samples/BlazorServerApp/Shared/MainLayout.razor b/src/Components/Samples/BlazorServerApp/Shared/MainLayout.razor index 3bac776c888c..bbd879c05fba 100644 --- a/src/Components/Samples/BlazorServerApp/Shared/MainLayout.razor +++ b/src/Components/Samples/BlazorServerApp/Shared/MainLayout.razor @@ -10,7 +10,7 @@
    - + @Body
    diff --git a/src/Components/Server/src/Circuits/RemoteErrorBoundaryLogger.cs b/src/Components/Server/src/Circuits/RemoteErrorBoundaryLogger.cs new file mode 100644 index 000000000000..a2e5cff81631 --- /dev/null +++ b/src/Components/Server/src/Circuits/RemoteErrorBoundaryLogger.cs @@ -0,0 +1,57 @@ +// 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.Threading.Tasks; +using Microsoft.AspNetCore.Components.Web; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; +using Microsoft.JSInterop; + +namespace Microsoft.AspNetCore.Components.Server.Circuits +{ + internal class RemoteErrorBoundaryLogger : IErrorBoundaryLogger + { + private static readonly Action _exceptionCaughtByErrorBoundary = LoggerMessage.Define( + LogLevel.Warning, + 100, + "Unhandled exception rendering component: {Message}"); + + private readonly ILogger _logger; + private readonly IJSRuntime _jsRuntime; + private readonly CircuitOptions _options; + + public RemoteErrorBoundaryLogger(ILogger logger, IJSRuntime jsRuntime, IOptions options) + { + _logger = logger; + _jsRuntime = jsRuntime; + _options = options.Value; + } + + public ValueTask LogErrorAsync(Exception exception, bool clientOnly) + { + // If the end user clicks on the UI to re-log the error, we don't want to spam the + // server logs so we only re-load to the client + if (!clientOnly) + { + // We always log detailed information to the server-side log + _exceptionCaughtByErrorBoundary(_logger, exception.Message, exception); + } + + // We log to the client only if the browser is connected interactively, and even then + // we may suppress the details + var shouldLogToClient = (_jsRuntime as RemoteJSRuntime)?.IsInitialized == true; + if (shouldLogToClient) + { + var message = _options.DetailedErrors + ? exception.ToString() + : $"For more details turn on detailed exceptions in '{nameof(CircuitOptions)}.{nameof(CircuitOptions.DetailedErrors)}'"; + return _jsRuntime.InvokeVoidAsync("console.error", message); + } + else + { + return ValueTask.CompletedTask; + } + } + } +} diff --git a/src/Components/Server/src/Circuits/RemoteJSRuntime.cs b/src/Components/Server/src/Circuits/RemoteJSRuntime.cs index 3a3cb11d27c3..ffd813b5ddfe 100644 --- a/src/Components/Server/src/Circuits/RemoteJSRuntime.cs +++ b/src/Components/Server/src/Circuits/RemoteJSRuntime.cs @@ -19,6 +19,8 @@ internal class RemoteJSRuntime : JSRuntime public ElementReferenceContext ElementReferenceContext { get; } + public bool IsInitialized => _clientProxy is not null; + public RemoteJSRuntime(IOptions options, ILogger logger) { _options = options.Value; diff --git a/src/Components/Server/src/DependencyInjection/ComponentServiceCollectionExtensions.cs b/src/Components/Server/src/DependencyInjection/ComponentServiceCollectionExtensions.cs index f87dc1392852..b6b1b08db96d 100644 --- a/src/Components/Server/src/DependencyInjection/ComponentServiceCollectionExtensions.cs +++ b/src/Components/Server/src/DependencyInjection/ComponentServiceCollectionExtensions.cs @@ -10,6 +10,7 @@ using Microsoft.AspNetCore.Components.Server.BlazorPack; using Microsoft.AspNetCore.Components.Server.Circuits; using Microsoft.AspNetCore.Components.Server.ProtectedBrowserStorage; +using Microsoft.AspNetCore.Components.Web; using Microsoft.AspNetCore.SignalR.Protocol; using Microsoft.Extensions.DependencyInjection.Extensions; using Microsoft.Extensions.Options; @@ -65,6 +66,7 @@ public static IServerSideBlazorBuilder AddServerSideBlazor(this IServiceCollecti services.TryAddSingleton(); services.TryAddSingleton(); services.TryAddSingleton(); + services.TryAddScoped(); services.TryAddScoped(s => s.GetRequiredService().Circuit); services.TryAddScoped(); diff --git a/src/Components/Web/src/PublicAPI.Unshipped.txt b/src/Components/Web/src/PublicAPI.Unshipped.txt index 42070070dd82..bc499176cdcb 100644 --- a/src/Components/Web/src/PublicAPI.Unshipped.txt +++ b/src/Components/Web/src/PublicAPI.Unshipped.txt @@ -15,16 +15,16 @@ Microsoft.AspNetCore.Components.Forms.InputTextArea.Element.get -> Microsoft.Asp Microsoft.AspNetCore.Components.Forms.InputTextArea.Element.set -> void *REMOVED*static Microsoft.AspNetCore.Components.Forms.BrowserFileExtensions.RequestImageFileAsync(this Microsoft.AspNetCore.Components.Forms.IBrowserFile! browserFile, string! format, int maxWith, int maxHeight) -> System.Threading.Tasks.ValueTask Microsoft.AspNetCore.Components.Web.ErrorBoundary +Microsoft.AspNetCore.Components.Web.ErrorBoundary.AutoReset.get -> bool +Microsoft.AspNetCore.Components.Web.ErrorBoundary.AutoReset.set -> void Microsoft.AspNetCore.Components.Web.ErrorBoundary.ChildContent.get -> Microsoft.AspNetCore.Components.RenderFragment? Microsoft.AspNetCore.Components.Web.ErrorBoundary.ChildContent.set -> void Microsoft.AspNetCore.Components.Web.ErrorBoundary.ErrorBoundary() -> void -Microsoft.AspNetCore.Components.Web.ErrorBoundary.ErrorContent.get -> Microsoft.AspNetCore.Components.RenderFragment? +Microsoft.AspNetCore.Components.Web.ErrorBoundary.ErrorContent.get -> Microsoft.AspNetCore.Components.RenderFragment? Microsoft.AspNetCore.Components.Web.ErrorBoundary.ErrorContent.set -> void -Microsoft.AspNetCore.Components.Web.ErrorBoundary.FormattedError -Microsoft.AspNetCore.Components.Web.ErrorBoundary.FormattedError.Details.get -> string! -Microsoft.AspNetCore.Components.Web.ErrorBoundary.FormattedError.FormattedError(string! message, string! details) -> void -Microsoft.AspNetCore.Components.Web.ErrorBoundary.FormattedError.Message.get -> string! Microsoft.AspNetCore.Components.Web.ErrorBoundary.HandleException(System.Exception! exception) -> void +Microsoft.AspNetCore.Components.Web.IErrorBoundaryLogger +Microsoft.AspNetCore.Components.Web.IErrorBoundaryLogger.LogErrorAsync(System.Exception! exception, bool clientOnly) -> System.Threading.Tasks.ValueTask static Microsoft.AspNetCore.Components.ElementReferenceExtensions.FocusAsync(this Microsoft.AspNetCore.Components.ElementReference elementReference, bool preventScroll) -> System.Threading.Tasks.ValueTask static Microsoft.AspNetCore.Components.Forms.BrowserFileExtensions.RequestImageFileAsync(this Microsoft.AspNetCore.Components.Forms.IBrowserFile! browserFile, string! format, int maxWidth, int maxHeight) -> System.Threading.Tasks.ValueTask Microsoft.AspNetCore.Components.RenderTree.WebEventDescriptor.EventName.get -> string! diff --git a/src/Components/Web/src/Web/ErrorBoundary.cs b/src/Components/Web/src/Web/ErrorBoundary.cs index a7f0ef757c07..37c18562f922 100644 --- a/src/Components/Web/src/Web/ErrorBoundary.cs +++ b/src/Components/Web/src/Web/ErrorBoundary.cs @@ -4,36 +4,32 @@ using System; using System.Threading.Tasks; using Microsoft.AspNetCore.Components.Rendering; -using Microsoft.JSInterop; namespace Microsoft.AspNetCore.Components.Web { // TODO: Reimplement directly on IComponent public sealed class ErrorBoundary : ComponentBase, IErrorBoundary { - private FormattedError? _currentError; + private Exception? _currentException; - [Inject] private IJSRuntime? JS { get; set; } + [Inject] private IErrorBoundaryLogger? ErrorBoundaryLogger { get; set; } + + /// + /// Specifies whether to reset the error state each time the is rendered + /// by its parent. This allows the child content to be recreated in an attempt to recover from the error. + /// + [Parameter] public bool AutoReset { get; set; } [Parameter] public RenderFragment? ChildContent { get; set; } - // Notice that, to respect the IClientErrorFormatter, we have to do this in terms of a Message/Details - // pair, and not in terms of the original Exception. We can't assume that developers who provide an - // ErrorContent understand the issues with exposing the raw exception to the client. - [Parameter] public RenderFragment? ErrorContent { get; set; } + [Parameter] public RenderFragment? ErrorContent { get; set; } - // TODO: Eliminate the enableDetailedErrors flag (and corresponding API on Renderer) - // and instead have some new default DI service, IClientErrorFormatter. public void HandleException(Exception exception) { - // TODO: We should log the underlying exception to some ILogger using the same - // severity that we did before for unhandled exceptions - _currentError = FormatErrorForClient(exception); - StateHasChanged(); + _ = ErrorBoundaryLogger!.LogErrorAsync(exception, clientOnly: false); - // TODO: Should there be an option not to auto-log exceptions to the console? - // Probably not. Don't want people thinking it's fine to have such exceptions. - _ = LogExceptionToClientIfPossible(); + _currentException = exception; + StateHasChanged(); } protected override void OnParametersSet() @@ -42,19 +38,23 @@ protected override void OnParametersSet() // re-renders us. This has the benefit that in your layout, you can just wrap this // around @Body and it does what you expect (recovering on navigate). But it might // make other cases more awkward because the parent will keep recreating any children - // that just error out on init. Maybe it should be an option. - _currentError = null; + // that just error out on init. + if (AutoReset) + { + _currentException = null; + } } protected override void BuildRenderTree(RenderTreeBuilder builder) { - if (_currentError is null) + var exception = _currentException; + if (exception is null) { builder.AddContent(0, ChildContent); } else if (ErrorContent is not null) { - builder.AddContent(1, ErrorContent(_currentError)); + builder.AddContent(1, ErrorContent(exception)); } else { @@ -66,46 +66,12 @@ protected override void BuildRenderTree(RenderTreeBuilder builder) // rely on there being any CSS, but make it overridable via CSS. builder.OpenElement(0, "div"); builder.AddAttribute(1, "onclick", (Func)(_ => - { - return LogExceptionToClientIfPossible(); - })); - + // Re-log, to help the developer figure out which ErrorBoundary issued which log message + ErrorBoundaryLogger!.LogErrorAsync(exception, clientOnly: true))); builder.AddContent(1, "Error"); builder.CloseElement(); builder.CloseRegion(); } } - - private async ValueTask LogExceptionToClientIfPossible() - { - // TODO: Handle prerendering too. We can't use IJSRuntime while prerendering. - if (_currentError is not null) - { - await JS!.InvokeVoidAsync("console.error", $"{_currentError.Message}\n{_currentError.Details}"); - } - } - - // TODO: Move into a new IClientErrorFormatter service - private static FormattedError FormatErrorForClient(Exception exception) - { - // TODO: Obviously this will be internal to IClientErrorFormatter - var enableDetailedErrors = true; - - return enableDetailedErrors - ? new FormattedError(exception.Message, exception.StackTrace ?? string.Empty) - : new FormattedError("There was an error", "For more details turn on detailed exceptions by setting 'DetailedErrors: true' in 'appSettings.Development.json' or set 'CircuitOptions.DetailedErrors'."); - } - - public record FormattedError - { - public FormattedError(string message, string details) - { - Message = message; - Details = details; - } - - public string Message { get; } - public string Details { get; } - } } } diff --git a/src/Components/Web/src/Web/IErrorBoundaryLogger.cs b/src/Components/Web/src/Web/IErrorBoundaryLogger.cs new file mode 100644 index 000000000000..6d616a5bca52 --- /dev/null +++ b/src/Components/Web/src/Web/IErrorBoundaryLogger.cs @@ -0,0 +1,28 @@ +// 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.Threading.Tasks; + +namespace Microsoft.AspNetCore.Components.Web +{ + // The rules arround logging differ between environments. + // - For WebAssembly, we always log to the console with detailed information + // - For Server, we log to both the server-side log (with detailed information), and to the + // client (respecting the DetailedError option) + // - In prerendering, we log only to the server-side log + + /// + /// Logs exception information for a component. + /// + public interface IErrorBoundaryLogger + { + /// + /// Logs the supplied . + /// + /// The to log. + /// If true, indicates that the error should only be logged to the client (e.g., because it was already logged to the server). + /// A representing the completion of the operation. + ValueTask LogErrorAsync(Exception exception, bool clientOnly); + } +} diff --git a/src/Components/WebAssembly/WebAssembly/src/Hosting/WebAssemblyHostBuilder.cs b/src/Components/WebAssembly/WebAssembly/src/Hosting/WebAssemblyHostBuilder.cs index 0d73eac48901..c42b7fce4da1 100644 --- a/src/Components/WebAssembly/WebAssembly/src/Hosting/WebAssemblyHostBuilder.cs +++ b/src/Components/WebAssembly/WebAssembly/src/Hosting/WebAssemblyHostBuilder.cs @@ -246,6 +246,7 @@ internal void InitializeDefaultServices() Services.AddSingleton(new LazyAssemblyLoader(DefaultWebAssemblyJSRuntime.Instance)); Services.AddSingleton(); Services.AddSingleton(sp => sp.GetRequiredService().State); + Services.AddSingleton(); Services.AddLogging(builder => { builder.AddProvider(new WebAssemblyConsoleLoggerProvider(DefaultWebAssemblyJSRuntime.Instance)); diff --git a/src/Components/WebAssembly/WebAssembly/src/Services/WebAssemblyErrorBoundaryLogger.cs b/src/Components/WebAssembly/WebAssembly/src/Services/WebAssemblyErrorBoundaryLogger.cs new file mode 100644 index 000000000000..736a22254905 --- /dev/null +++ b/src/Components/WebAssembly/WebAssembly/src/Services/WebAssemblyErrorBoundaryLogger.cs @@ -0,0 +1,28 @@ +// 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.Threading.Tasks; +using Microsoft.AspNetCore.Components.Web; +using Microsoft.Extensions.Logging; + +namespace Microsoft.AspNetCore.Components.WebAssembly.Services +{ + internal class WebAssemblyErrorBoundaryLogger : IErrorBoundaryLogger + { + private readonly ILogger _errorBoundaryLogger; + + public WebAssemblyErrorBoundaryLogger(ILogger errorBoundaryLogger) + { + _errorBoundaryLogger = errorBoundaryLogger ?? throw new ArgumentNullException(nameof(errorBoundaryLogger)); ; + } + + public ValueTask LogErrorAsync(Exception exception, bool clientOnly) + { + // For, client-side code, all internal state is visible to the end user. We can just + // log directly to the console. + _errorBoundaryLogger.LogError(exception.ToString()); + return ValueTask.CompletedTask; + } + } +} From 4514899f083d4d4eca5a931f6edfe3f89963e30a Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Mon, 15 Mar 2021 13:03:19 +0000 Subject: [PATCH 18/55] Add PrerenderingErrorBoundaryLogger --- ...MvcViewFeaturesMvcCoreBuilderExtensions.cs | 2 ++ .../PrerenderingErrorBoundaryLogger.cs | 30 +++++++++++++++++++ 2 files changed, 32 insertions(+) create mode 100644 src/Mvc/Mvc.ViewFeatures/src/RazorComponents/PrerenderingErrorBoundaryLogger.cs diff --git a/src/Mvc/Mvc.ViewFeatures/src/DependencyInjection/MvcViewFeaturesMvcCoreBuilderExtensions.cs b/src/Mvc/Mvc.ViewFeatures/src/DependencyInjection/MvcViewFeaturesMvcCoreBuilderExtensions.cs index d98679b5e865..94d5c80c9c40 100644 --- a/src/Mvc/Mvc.ViewFeatures/src/DependencyInjection/MvcViewFeaturesMvcCoreBuilderExtensions.cs +++ b/src/Mvc/Mvc.ViewFeatures/src/DependencyInjection/MvcViewFeaturesMvcCoreBuilderExtensions.cs @@ -8,6 +8,7 @@ using Microsoft.AspNetCore.Components.Lifetime; using Microsoft.AspNetCore.Components.Rendering; using Microsoft.AspNetCore.Components.Routing; +using Microsoft.AspNetCore.Components.Web; using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.Mvc.ApplicationModels; using Microsoft.AspNetCore.Mvc.ApplicationParts; @@ -240,6 +241,7 @@ internal static void AddViewServices(IServiceCollection services) services.TryAddScoped(); services.TryAddScoped(); services.TryAddScoped(sp => sp.GetRequiredService().State); + services.TryAddScoped(); services.TryAddTransient(); diff --git a/src/Mvc/Mvc.ViewFeatures/src/RazorComponents/PrerenderingErrorBoundaryLogger.cs b/src/Mvc/Mvc.ViewFeatures/src/RazorComponents/PrerenderingErrorBoundaryLogger.cs new file mode 100644 index 000000000000..89d419b81ac8 --- /dev/null +++ b/src/Mvc/Mvc.ViewFeatures/src/RazorComponents/PrerenderingErrorBoundaryLogger.cs @@ -0,0 +1,30 @@ +// 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.Threading.Tasks; +using Microsoft.AspNetCore.Components.Web; + +namespace Microsoft.AspNetCore.Mvc.ViewFeatures +{ + internal class PrerenderingErrorBoundaryLogger : IErrorBoundaryLogger + { + private static readonly Action _exceptionCaughtByErrorBoundary = LoggerMessage.Define( + LogLevel.Warning, + 100, + "Unhandled exception rendering component: {Message}"); + + private readonly ILogger _logger; + + public PrerenderingErrorBoundaryLogger(ILogger logger) + { + _logger = logger; + } + + public ValueTask LogErrorAsync(Exception exception, bool clientOnly) + { + _exceptionCaughtByErrorBoundary(_logger, exception.Message, exception); + return ValueTask.CompletedTask; + } + } +} From 2de42595dcf3352f44106c7405beadc38a6e8850 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Tue, 16 Mar 2021 11:48:52 +0000 Subject: [PATCH 19/55] Let error boundaries catch their own inline errors --- .../Components/src/RenderTree/Renderer.cs | 5 +---- .../BlazorServerApp/Pages/ErrorMaker.razor | 18 ++++++++++++++++++ src/Components/Web/src/Web/ErrorBoundary.cs | 8 ++++++++ 3 files changed, 27 insertions(+), 4 deletions(-) diff --git a/src/Components/Components/src/RenderTree/Renderer.cs b/src/Components/Components/src/RenderTree/Renderer.cs index a02a0acaf6ae..2ae83d5713ec 100644 --- a/src/Components/Components/src/RenderTree/Renderer.cs +++ b/src/Components/Components/src/RenderTree/Renderer.cs @@ -391,10 +391,7 @@ private bool TrySendExceptionToErrorBoundary(IComponent? errorSource, Exception // If it becomes problematic, we can maintain a lookup from component instance to ID. var componentState = _componentStateById.FirstOrDefault(kvp => kvp.Value.Component == errorSource).Value; - // Find the closest IErrorBoundary, if any. Start looking at the first parent, to ensure - // that an error boundary component doesn't try to handle its own errors, as that could - // be an infinite loop. - componentState = componentState?.ParentComponentState; + // Find the closest IErrorBoundary, if any while (componentState is not null) { if (componentState.Component is IErrorBoundary errorBoundary) diff --git a/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor b/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor index d7063c8fd487..8e62939bcb54 100644 --- a/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor +++ b/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor @@ -45,6 +45,22 @@ + +
    + Exception inline in error boundary markup + + + @if (throwInline) { throw new InvalidTimeZoneException("Inline exception"); } +

    Hello!

    +     + + @if (throwInErrorContent) { throw new InvalidTimeZoneException("Inline exception in error content"); } +

    There was an error: @context

    +     +     + + +
    @code { @@ -98,6 +114,8 @@ private bool throwInOnAfterRender; private bool throwInOnAfterRenderAsync; private bool throwWhileRendering; + private bool throwInline; + private bool throwInErrorContent; void EventHandlerErrorSync() => throw new InvalidTimeZoneException("Synchronous error from event handler"); diff --git a/src/Components/Web/src/Web/ErrorBoundary.cs b/src/Components/Web/src/Web/ErrorBoundary.cs index 37c18562f922..60932e81b29b 100644 --- a/src/Components/Web/src/Web/ErrorBoundary.cs +++ b/src/Components/Web/src/Web/ErrorBoundary.cs @@ -2,6 +2,7 @@ // 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; using Microsoft.AspNetCore.Components.Rendering; @@ -26,6 +27,13 @@ public sealed class ErrorBoundary : ComponentBase, IErrorBoundary public void HandleException(Exception exception) { + if (_currentException is not null) + { + // If there's an error while we're already displaying error content, then it's the + // error content that's failing. Avoid the risk of an infinite error rendering loop. + ExceptionDispatchInfo.Capture(exception).Throw(); + } + _ = ErrorBoundaryLogger!.LogErrorAsync(exception, clientOnly: false); _currentException = exception; From 415d8ddf568a86aadab506401908df11bcb4a9af Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Tue, 16 Mar 2021 14:26:45 +0000 Subject: [PATCH 20/55] Redesign around ErrorBoundaryBase abstract base class. --- .../Components/src/ErrorBoundaryBase.cs | 131 ++++++++++++++++++ .../Components/src/IErrorBoundary.cs | 29 ---- .../Components/src/PublicAPI.Unshipped.txt | 14 +- .../Components/src/RenderTree/Renderer.cs | 16 +-- .../src/Rendering/ComponentState.cs | 7 +- .../BlazorServerApp/Shared/ErrorIgnorer.cs | 23 +++ .../BlazorServerApp/Shared/ErrorIgnorer.razor | 18 --- .../Web/src/PublicAPI.Unshipped.txt | 7 - src/Components/Web/src/Web/ErrorBoundary.cs | 86 +++--------- 9 files changed, 196 insertions(+), 135 deletions(-) create mode 100644 src/Components/Components/src/ErrorBoundaryBase.cs delete mode 100644 src/Components/Components/src/IErrorBoundary.cs create mode 100644 src/Components/Samples/BlazorServerApp/Shared/ErrorIgnorer.cs delete mode 100644 src/Components/Samples/BlazorServerApp/Shared/ErrorIgnorer.razor diff --git a/src/Components/Components/src/ErrorBoundaryBase.cs b/src/Components/Components/src/ErrorBoundaryBase.cs new file mode 100644 index 000000000000..43f4f2dfb1f5 --- /dev/null +++ b/src/Components/Components/src/ErrorBoundaryBase.cs @@ -0,0 +1,131 @@ +// 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; +using Microsoft.AspNetCore.Components.Rendering; + +namespace Microsoft.AspNetCore.Components +{ + /// + /// A base class for error boundary components. + /// + public abstract class ErrorBoundaryBase : IComponent + { + // This deliberately doesn't inherit from ComponentBase because it's not intended to be + // subclassable using a .razor file. ErrorBoundaryBase shouldn't be used as a base class + // for all components indiscriminately, because that will lead to undesirable error-ignoring + // patterns. We expect that subclassing ErrorBoundaryBase to be done mainly by platform + // authors, providing just a default error UI for their rendering technology and not + // customizing other aspects of the semantics, such as whether to re-render after an error. + + private RenderHandle _renderHandle; + private Exception? _currentException; + + /// + /// The content to be displayed when there is no error. + /// + [Parameter] public RenderFragment? ChildContent { get; set; } + + /// + /// The content to be displayed when there is an error. + /// + [Parameter] public RenderFragment? ErrorContent { get; set; } + + /// + /// Specifies whether to reset the error state each time this component instance is rendered + /// by its parent. This allows the child content to be recreated in an attempt to recover from the error. + /// + [Parameter] public bool AutoReset { get; set; } + + /// + public void Attach(RenderHandle renderHandle) + { + _renderHandle = renderHandle; + } + + /// + public Task SetParametersAsync(ParameterView parameters) + { + foreach (var parameter in parameters) + { + if (parameter.Name.Equals(nameof(ChildContent), StringComparison.OrdinalIgnoreCase)) + { + ChildContent = (RenderFragment)parameter.Value; + } + else if (parameter.Name.Equals(nameof(ErrorContent), StringComparison.OrdinalIgnoreCase)) + { + ErrorContent = (RenderFragment)parameter.Value; + } + else if (parameter.Name.Equals(nameof(AutoReset), StringComparison.OrdinalIgnoreCase)) + { + AutoReset = (bool)parameter.Value; + } + else + { + throw new ArgumentException($"The component '{GetType().FullName}' does not accept a parameter with the name '{parameter.Name}'."); + } + } + + if (AutoReset) + { + _currentException = null; + } + + _renderHandle.Render(Render); + return Task.CompletedTask; + } + + /// + /// Logs the exception. + /// + /// The being handled. + protected abstract ValueTask LogExceptionAsync(Exception exception); + + /// + /// Renders the default error content. This will only be used when + /// was not supplied. + /// + /// The + /// The current exception. + protected abstract void RenderDefaultErrorContent(RenderTreeBuilder builder, Exception exception); + + internal void HandleException(Exception exception) + { + if (_currentException is not null) + { + // If there's an error while we're already displaying error content, then it's the + // error content that's failing. Avoid the risk of an infinite error rendering loop. + ExceptionDispatchInfo.Capture(exception).Throw(); + } + + // TODO: If there's an async exception here, do something with it (can be fatal) + _ = LogExceptionAsync(exception); + + _currentException = exception; + _renderHandle.Render(Render); + } + + private void Render(RenderTreeBuilder builder) + { + if (_currentException is null) + { + builder.AddContent(0, ChildContent); + } + else if (ErrorContent is not null) + { + builder.AddContent(1, ErrorContent(_currentException)); + } + else + { + // Even if the subclass tries to re-render the same ChildContent as its default error content, + // we still won't reuse the subtree components because they are in a different region. So we + // can be sure the old tree will be correctly disposed. + builder.OpenRegion(2); + RenderDefaultErrorContent(builder, _currentException); + builder.CloseRegion(); + } + } + } +} diff --git a/src/Components/Components/src/IErrorBoundary.cs b/src/Components/Components/src/IErrorBoundary.cs deleted file mode 100644 index 9ea183bdd59c..000000000000 --- a/src/Components/Components/src/IErrorBoundary.cs +++ /dev/null @@ -1,29 +0,0 @@ -// 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 -{ - public interface IErrorBoundary - { - // Warning: if you make an IErrorBoundary that reacts to errors by rendering some child - // that throws during its own rendering, you will have an infinite loop of errors. How could - // we detect this? As long as it's legit to recover, then you have to deal with a succession - // of valid (non-infinite-looping) errors over time. We can't detect this by looking at the - // call stack, because a child might trigger an error asynchronously, making it into a slightly - // spaced out but still infinite loop. - // - // It would be weird, but we could track the timestamp of the last error, and if another one - // occurs in < 100ms, then treat it as fatal. Technically we could do this for all IErrorBoundary - // (not just the .Web one) by stashing that info in ComponentState, at the cost of using more - // memory for every component, not just IErrorBoundary. - // - // Another option is to handle this in the .Web one using the rule "if you notify me of an error - // while I believe I'm already showing an error, then I'll just rethrow so it gets treated as - // fatal for the whole circuit". This would take care of errors caused when rendering ErrorContent. - // This might be the best solution. People implementing a custom IErrorBoundary should take care - // to put in similar logic, if we're OK relying on that. - void HandleException(Exception exception); - } -} diff --git a/src/Components/Components/src/PublicAPI.Unshipped.txt b/src/Components/Components/src/PublicAPI.Unshipped.txt index ada7a56545e0..97fa4530f9db 100644 --- a/src/Components/Components/src/PublicAPI.Unshipped.txt +++ b/src/Components/Components/src/PublicAPI.Unshipped.txt @@ -9,8 +9,16 @@ Microsoft.AspNetCore.Components.ComponentApplicationState.PersistAsJson( Microsoft.AspNetCore.Components.ComponentApplicationState.PersistState(string! key, byte[]! value) -> void Microsoft.AspNetCore.Components.ComponentApplicationState.TryTakeAsJson(string! key, out TValue? instance) -> bool Microsoft.AspNetCore.Components.ComponentApplicationState.TryTakePersistedState(string! key, out byte[]? value) -> bool -Microsoft.AspNetCore.Components.IErrorBoundary -Microsoft.AspNetCore.Components.IErrorBoundary.HandleException(System.Exception! exception) -> void +Microsoft.AspNetCore.Components.ErrorBoundaryBase +Microsoft.AspNetCore.Components.ErrorBoundaryBase.Attach(Microsoft.AspNetCore.Components.RenderHandle renderHandle) -> void +Microsoft.AspNetCore.Components.ErrorBoundaryBase.AutoReset.get -> bool +Microsoft.AspNetCore.Components.ErrorBoundaryBase.AutoReset.set -> void +Microsoft.AspNetCore.Components.ErrorBoundaryBase.ChildContent.get -> Microsoft.AspNetCore.Components.RenderFragment? +Microsoft.AspNetCore.Components.ErrorBoundaryBase.ChildContent.set -> void +Microsoft.AspNetCore.Components.ErrorBoundaryBase.ErrorBoundaryBase() -> void +Microsoft.AspNetCore.Components.ErrorBoundaryBase.ErrorContent.get -> Microsoft.AspNetCore.Components.RenderFragment? +Microsoft.AspNetCore.Components.ErrorBoundaryBase.ErrorContent.set -> void +Microsoft.AspNetCore.Components.ErrorBoundaryBase.SetParametersAsync(Microsoft.AspNetCore.Components.ParameterView parameters) -> System.Threading.Tasks.Task! Microsoft.AspNetCore.Components.Lifetime.ComponentApplicationLifetime Microsoft.AspNetCore.Components.Lifetime.ComponentApplicationLifetime.ComponentApplicationLifetime(Microsoft.Extensions.Logging.ILogger! logger) -> void Microsoft.AspNetCore.Components.Lifetime.ComponentApplicationLifetime.PersistStateAsync(Microsoft.AspNetCore.Components.Lifetime.IComponentApplicationStateStore! store, Microsoft.AspNetCore.Components.RenderTree.Renderer! renderer) -> System.Threading.Tasks.Task! @@ -35,6 +43,8 @@ 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.LogExceptionAsync(System.Exception! exception) -> System.Threading.Tasks.ValueTask +abstract Microsoft.AspNetCore.Components.ErrorBoundaryBase.RenderDefaultErrorContent(Microsoft.AspNetCore.Components.Rendering.RenderTreeBuilder! builder, System.Exception! exception) -> void 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! 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! diff --git a/src/Components/Components/src/RenderTree/Renderer.cs b/src/Components/Components/src/RenderTree/Renderer.cs index 2ae83d5713ec..184ad615e61f 100644 --- a/src/Components/Components/src/RenderTree/Renderer.cs +++ b/src/Components/Components/src/RenderTree/Renderer.cs @@ -391,10 +391,10 @@ private bool TrySendExceptionToErrorBoundary(IComponent? errorSource, Exception // If it becomes problematic, we can maintain a lookup from component instance to ID. var componentState = _componentStateById.FirstOrDefault(kvp => kvp.Value.Component == errorSource).Value; - // Find the closest IErrorBoundary, if any + // Find the closest error boundary, if any while (componentState is not null) { - if (componentState.Component is IErrorBoundary errorBoundary) + if (componentState.Component is ErrorBoundaryBase errorBoundary) { // Force the IErrorBoundary component to clear its output, regardless of any logic inside // that component. This ensures that all descendants are cleaned up. We don't strictly have @@ -406,10 +406,10 @@ private bool TrySendExceptionToErrorBoundary(IComponent? errorSource, Exception try { - // TODO: Should we log the error here? Currently it's up to the IErrorBoundary to do what - // it wants, including hiding the error. Maybe we should force it to get logged regardless - // of what the IErrorBoundary want to do. Whichever way we go on this is permanent, as - // switching in either direction would be a breaking change. + // We don't log the error here, and instead leave it up to the IErrorBoundary to do + // what it wants (which could be suppressing the error entirely). Note that the default + // logging behavior has to vary between hosting models. + // TODO: Are we happy with letting IErrorBoundary suppress errors if it wants? errorBoundary.HandleException(error); } catch (Exception errorBoundaryException) @@ -417,6 +417,8 @@ private bool TrySendExceptionToErrorBoundary(IComponent? errorSource, Exception // If *notifying* about an exception fails, it's OK for that to be fatal. This is // different from an IErrorBoundary component throwing during its own rendering or // lifecycle methods. + // TODO: Should we support rethrowing from inside HandleException? I prefer not to + // unless we get a better justification. See design doc. HandleException(errorBoundaryException); } @@ -827,8 +829,6 @@ private void RenderInExistingBatch(RenderQueueEntry renderQueueEntry) } else { - // TODO: Should we use disposeComponentState.Component as the owningComponent - // so this is recoverable via IErrorBoundary? AddToPendingTasks(GetHandledAsynchronousDisposalErrorsTask(result), null); async Task GetHandledAsynchronousDisposalErrorsTask(Task result) diff --git a/src/Components/Components/src/Rendering/ComponentState.cs b/src/Components/Components/src/Rendering/ComponentState.cs index 75bba063bdc6..d496d1186f43 100644 --- a/src/Components/Components/src/Rendering/ComponentState.cs +++ b/src/Components/Components/src/Rendering/ComponentState.cs @@ -76,12 +76,9 @@ public void RenderIntoBatch(RenderBatchBuilder batchBuilder, RenderFragment rend // If an exception occurs in the render fragment delegate, we won't process the diff in any way, so child components, // event handlers, etc., will all be left untouched as if this component didn't re-render at all. We also are careful // to leave ComponentState in an internally-consistent state so that technically this component could continue to be - // used. + // used. However, the Renderer will not allow the component to continue to be used, except if it's the IErrorBoundary, + // because it forcibly clears the descendants of the IErrorBoundary before notifying it. // TODO: Verify that having a try/catch here doesn't degrade perf noticeably on WebAssembly. It might do. - // TODO: Should we actually terminate this component in some way to guarantee it doesn't re-render? That's tricky because - // we need it to be disposed in order that descendants, event handlers, etc. get cleaned up too. Unless we rely on it - // getting removed from its parent, we would have to duplicate quite a bit of internal state-keeping to ensure all the - // descendant state gets cleared up. renderFragmentException = ex; return; } diff --git a/src/Components/Samples/BlazorServerApp/Shared/ErrorIgnorer.cs b/src/Components/Samples/BlazorServerApp/Shared/ErrorIgnorer.cs new file mode 100644 index 000000000000..30928077c0f1 --- /dev/null +++ b/src/Components/Samples/BlazorServerApp/Shared/ErrorIgnorer.cs @@ -0,0 +1,23 @@ +using System; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Components; +using Microsoft.AspNetCore.Components.Rendering; + +namespace BlazorServerApp.Shared +{ + // A badly-behaved error boundary that tries to keep using its same ChildContent even after an error + // This is to check we still don't retain the descendant component instances + public class ErrorIgnorer : ErrorBoundaryBase + { + protected override ValueTask LogExceptionAsync(Exception exception) + { + Console.WriteLine($"There was an error, but we'll try to ignore it. La la la la, can't year you. [{exception}]"); + return ValueTask.CompletedTask; + } + + protected override void RenderDefaultErrorContent(RenderTreeBuilder builder, Exception exception) + { + ChildContent!(builder); + } + } +} diff --git a/src/Components/Samples/BlazorServerApp/Shared/ErrorIgnorer.razor b/src/Components/Samples/BlazorServerApp/Shared/ErrorIgnorer.razor deleted file mode 100644 index 84477c3c2e4a..000000000000 --- a/src/Components/Samples/BlazorServerApp/Shared/ErrorIgnorer.razor +++ /dev/null @@ -1,18 +0,0 @@ -@implements IErrorBoundary -@ChildContent -@code { - // This represents a badly-behaved IErrorBoundary that tries to keep rendering the - // same content even after an error. Error boundaries shouldn't do this since it - // risks an infinite rendering loop if the child content throws immediately on - // rendering. - - [Parameter] public RenderFragment? ChildContent { get; set; } - - public void HandleException(Exception ex) - { - Console.WriteLine($"Got an error, but will try to pretend it didn't happen: {ex.Message}"); - - // Trying to bring back the child content - StateHasChanged(); - } -} diff --git a/src/Components/Web/src/PublicAPI.Unshipped.txt b/src/Components/Web/src/PublicAPI.Unshipped.txt index bc499176cdcb..fbeafd68bf5e 100644 --- a/src/Components/Web/src/PublicAPI.Unshipped.txt +++ b/src/Components/Web/src/PublicAPI.Unshipped.txt @@ -15,14 +15,7 @@ Microsoft.AspNetCore.Components.Forms.InputTextArea.Element.get -> Microsoft.Asp Microsoft.AspNetCore.Components.Forms.InputTextArea.Element.set -> void *REMOVED*static Microsoft.AspNetCore.Components.Forms.BrowserFileExtensions.RequestImageFileAsync(this Microsoft.AspNetCore.Components.Forms.IBrowserFile! browserFile, string! format, int maxWith, int maxHeight) -> System.Threading.Tasks.ValueTask Microsoft.AspNetCore.Components.Web.ErrorBoundary -Microsoft.AspNetCore.Components.Web.ErrorBoundary.AutoReset.get -> bool -Microsoft.AspNetCore.Components.Web.ErrorBoundary.AutoReset.set -> void -Microsoft.AspNetCore.Components.Web.ErrorBoundary.ChildContent.get -> Microsoft.AspNetCore.Components.RenderFragment? -Microsoft.AspNetCore.Components.Web.ErrorBoundary.ChildContent.set -> void Microsoft.AspNetCore.Components.Web.ErrorBoundary.ErrorBoundary() -> void -Microsoft.AspNetCore.Components.Web.ErrorBoundary.ErrorContent.get -> Microsoft.AspNetCore.Components.RenderFragment? -Microsoft.AspNetCore.Components.Web.ErrorBoundary.ErrorContent.set -> void -Microsoft.AspNetCore.Components.Web.ErrorBoundary.HandleException(System.Exception! exception) -> void Microsoft.AspNetCore.Components.Web.IErrorBoundaryLogger Microsoft.AspNetCore.Components.Web.IErrorBoundaryLogger.LogErrorAsync(System.Exception! exception, bool clientOnly) -> System.Threading.Tasks.ValueTask static Microsoft.AspNetCore.Components.ElementReferenceExtensions.FocusAsync(this Microsoft.AspNetCore.Components.ElementReference elementReference, bool preventScroll) -> System.Threading.Tasks.ValueTask diff --git a/src/Components/Web/src/Web/ErrorBoundary.cs b/src/Components/Web/src/Web/ErrorBoundary.cs index 60932e81b29b..cf0324794a91 100644 --- a/src/Components/Web/src/Web/ErrorBoundary.cs +++ b/src/Components/Web/src/Web/ErrorBoundary.cs @@ -2,84 +2,38 @@ // 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; using Microsoft.AspNetCore.Components.Rendering; namespace Microsoft.AspNetCore.Components.Web { - // TODO: Reimplement directly on IComponent - public sealed class ErrorBoundary : ComponentBase, IErrorBoundary + /// + /// Captures errors thrown from its child content. + /// + public sealed class ErrorBoundary : ErrorBoundaryBase { - private Exception? _currentException; - [Inject] private IErrorBoundaryLogger? ErrorBoundaryLogger { get; set; } - /// - /// Specifies whether to reset the error state each time the is rendered - /// by its parent. This allows the child content to be recreated in an attempt to recover from the error. - /// - [Parameter] public bool AutoReset { get; set; } - - [Parameter] public RenderFragment? ChildContent { get; set; } - - [Parameter] public RenderFragment? ErrorContent { get; set; } - - public void HandleException(Exception exception) - { - if (_currentException is not null) - { - // If there's an error while we're already displaying error content, then it's the - // error content that's failing. Avoid the risk of an infinite error rendering loop. - ExceptionDispatchInfo.Capture(exception).Throw(); - } - - _ = ErrorBoundaryLogger!.LogErrorAsync(exception, clientOnly: false); - - _currentException = exception; - StateHasChanged(); - } - - protected override void OnParametersSet() + /// + protected override ValueTask LogExceptionAsync(Exception exception) { - // Not totally sure about this, but here we auto-recover if the parent component - // re-renders us. This has the benefit that in your layout, you can just wrap this - // around @Body and it does what you expect (recovering on navigate). But it might - // make other cases more awkward because the parent will keep recreating any children - // that just error out on init. - if (AutoReset) - { - _currentException = null; - } + return ErrorBoundaryLogger!.LogErrorAsync(exception, clientOnly: false); } - protected override void BuildRenderTree(RenderTreeBuilder builder) + /// + protected override void RenderDefaultErrorContent(RenderTreeBuilder builder, Exception exception) { - var exception = _currentException; - if (exception is null) - { - builder.AddContent(0, ChildContent); - } - else if (ErrorContent is not null) - { - builder.AddContent(1, ErrorContent(exception)); - } - else - { - builder.OpenRegion(2); - // TODO: Better UI (some kind of language-independent icon, CSS stylability) - // Could it simply be
    , with all the - // content provided in template CSS? Is this even going to be used in the - // template by default? It's probably best have some default UI that doesn't - // rely on there being any CSS, but make it overridable via CSS. - builder.OpenElement(0, "div"); - builder.AddAttribute(1, "onclick", (Func)(_ => - // Re-log, to help the developer figure out which ErrorBoundary issued which log message - ErrorBoundaryLogger!.LogErrorAsync(exception, clientOnly: true))); - builder.AddContent(1, "Error"); - builder.CloseElement(); - builder.CloseRegion(); - } + // TODO: Better UI (some kind of language-independent icon, CSS stylability) + // Could it simply be
    , with all the + // content provided in template CSS? Is this even going to be used in the + // template by default? It's probably best have some default UI that doesn't + // rely on there being any CSS, but make it overridable via CSS. + builder.OpenElement(0, "div"); + builder.AddAttribute(1, "onclick", (Func)(_ => + // Re-log, to help the developer figure out which ErrorBoundary issued which log message + ErrorBoundaryLogger!.LogErrorAsync(exception, clientOnly: true))); + builder.AddContent(1, "Error"); + builder.CloseElement(); } } } From 95291632fa4ac38b30523d4c6b41eae9c58715ed Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Wed, 17 Mar 2021 09:39:52 +0000 Subject: [PATCH 21/55] Define IErrorBoundary as an internal interface for clarity --- .../Components/src/ErrorBoundaryBase.cs | 4 ++-- .../Components/src/IErrorBoundary.cs | 23 +++++++++++++++++++ .../Components/src/RenderTree/Renderer.cs | 13 +++++------ 3 files changed, 31 insertions(+), 9 deletions(-) create mode 100644 src/Components/Components/src/IErrorBoundary.cs diff --git a/src/Components/Components/src/ErrorBoundaryBase.cs b/src/Components/Components/src/ErrorBoundaryBase.cs index 43f4f2dfb1f5..a467c70d0ae8 100644 --- a/src/Components/Components/src/ErrorBoundaryBase.cs +++ b/src/Components/Components/src/ErrorBoundaryBase.cs @@ -11,7 +11,7 @@ namespace Microsoft.AspNetCore.Components /// /// A base class for error boundary components. /// - public abstract class ErrorBoundaryBase : IComponent + public abstract class ErrorBoundaryBase : IComponent, IErrorBoundary { // This deliberately doesn't inherit from ComponentBase because it's not intended to be // subclassable using a .razor file. ErrorBoundaryBase shouldn't be used as a base class @@ -91,7 +91,7 @@ public Task SetParametersAsync(ParameterView parameters) /// The current exception. protected abstract void RenderDefaultErrorContent(RenderTreeBuilder builder, Exception exception); - internal void HandleException(Exception exception) + void IErrorBoundary.HandleException(Exception exception) { if (_currentException is not null) { diff --git a/src/Components/Components/src/IErrorBoundary.cs b/src/Components/Components/src/IErrorBoundary.cs new file mode 100644 index 000000000000..b433dd055737 --- /dev/null +++ b/src/Components/Components/src/IErrorBoundary.cs @@ -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: + // + // [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); + } +} diff --git a/src/Components/Components/src/RenderTree/Renderer.cs b/src/Components/Components/src/RenderTree/Renderer.cs index 184ad615e61f..95db63599a8e 100644 --- a/src/Components/Components/src/RenderTree/Renderer.cs +++ b/src/Components/Components/src/RenderTree/Renderer.cs @@ -394,14 +394,13 @@ private bool TrySendExceptionToErrorBoundary(IComponent? errorSource, Exception // Find the closest error boundary, if any while (componentState is not null) { - if (componentState.Component is ErrorBoundaryBase errorBoundary) + if (componentState.Component is IErrorBoundary errorBoundary) { - // Force the IErrorBoundary component to clear its output, regardless of any logic inside - // that component. This ensures that all descendants are cleaned up. We don't strictly have - // to do this, since the only errors we handle this way are actually recoverable, so technically - // it would be OK if the old output was left in place and continued operating. However that - // would be a whole new way of keeping components running after failure which we don't want - // to introduce and guarantee to support forever. + // Even though ErrorBoundaryBase always removes its ChildContent from the tree when + // switching into an error state, the Renderer doesn't rely on that. To guarantee that + // the failed subtree is disposed, forcibly remove it here. If the failed components did + // continue to run, it wouldn't harm framework state, but it would be a whole new kind of + // edge case to support forever. AddToRenderQueue(componentState.ComponentId, builder => { }); try From bf04cf3cce378ee0280d1eae1286b4731e1d7ea7 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Thu, 18 Mar 2021 13:53:21 +0000 Subject: [PATCH 22/55] Minor fix --- .../src/RazorComponents/PrerenderingErrorBoundaryLogger.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Mvc/Mvc.ViewFeatures/src/RazorComponents/PrerenderingErrorBoundaryLogger.cs b/src/Mvc/Mvc.ViewFeatures/src/RazorComponents/PrerenderingErrorBoundaryLogger.cs index 89d419b81ac8..34fae7dd21fd 100644 --- a/src/Mvc/Mvc.ViewFeatures/src/RazorComponents/PrerenderingErrorBoundaryLogger.cs +++ b/src/Mvc/Mvc.ViewFeatures/src/RazorComponents/PrerenderingErrorBoundaryLogger.cs @@ -4,6 +4,7 @@ using System; using System.Threading.Tasks; using Microsoft.AspNetCore.Components.Web; +using Microsoft.Extensions.Logging; namespace Microsoft.AspNetCore.Mvc.ViewFeatures { From 1db319edcfa912d628832969d422058832bb041e Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Tue, 6 Apr 2021 09:51:42 +0100 Subject: [PATCH 23/55] Handle logging errors --- .../Components/src/ErrorBoundaryBase.cs | 24 ++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/src/Components/Components/src/ErrorBoundaryBase.cs b/src/Components/Components/src/ErrorBoundaryBase.cs index a467c70d0ae8..47c22286f376 100644 --- a/src/Components/Components/src/ErrorBoundaryBase.cs +++ b/src/Components/Components/src/ErrorBoundaryBase.cs @@ -100,11 +100,29 @@ void IErrorBoundary.HandleException(Exception exception) ExceptionDispatchInfo.Capture(exception).Throw(); } - // TODO: If there's an async exception here, do something with it (can be fatal) - _ = LogExceptionAsync(exception); - + var logExceptionTask = LogExceptionAsync(exception); _currentException = exception; _renderHandle.Render(Render); + + // If the logger is failing, show its exception in preference to showing the original + // exception, since otherwise the developer has no way to discover it. + if (!logExceptionTask.IsCompletedSuccessfully) + { + _ = HandleAnyLoggingErrors(logExceptionTask); + } + } + + private async Task HandleAnyLoggingErrors(ValueTask logExceptionTask) + { + try + { + await logExceptionTask; + } + catch (Exception exception) + { + _currentException = exception; + _renderHandle.Render(Render); + } } private void Render(RenderTreeBuilder builder) From 0415115e0aec9cc161869ed2dfc3060f1ae01570 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Tue, 6 Apr 2021 11:49:45 +0100 Subject: [PATCH 24/55] Make it possible to customize the UI via subclassing ErrorBoundary --- .../Components/src/ErrorBoundaryBase.cs | 112 +++++------------- .../Components/src/PublicAPI.Unshipped.txt | 5 +- .../Components/src/RenderTree/Renderer.cs | 2 +- .../BlazorServerApp/Shared/ErrorIgnorer.cs | 16 +-- .../src/Circuits/RemoteErrorBoundaryLogger.cs | 11 +- .../Web/src/PublicAPI.Unshipped.txt | 4 +- src/Components/Web/src/Web/ErrorBoundary.cs | 40 ++++--- .../Web/src/Web/IErrorBoundaryLogger.cs | 3 +- .../WebAssemblyErrorBoundaryLogger.cs | 2 +- .../PrerenderingErrorBoundaryLogger.cs | 2 +- 10 files changed, 73 insertions(+), 124 deletions(-) diff --git a/src/Components/Components/src/ErrorBoundaryBase.cs b/src/Components/Components/src/ErrorBoundaryBase.cs index 47c22286f376..d1d4cdae6a28 100644 --- a/src/Components/Components/src/ErrorBoundaryBase.cs +++ b/src/Components/Components/src/ErrorBoundaryBase.cs @@ -4,25 +4,14 @@ using System; using System.Runtime.ExceptionServices; using System.Threading.Tasks; -using Microsoft.AspNetCore.Components.Rendering; namespace Microsoft.AspNetCore.Components { /// /// A base class for error boundary components. /// - public abstract class ErrorBoundaryBase : IComponent, IErrorBoundary + public abstract class ErrorBoundaryBase : ComponentBase, IErrorBoundary { - // This deliberately doesn't inherit from ComponentBase because it's not intended to be - // subclassable using a .razor file. ErrorBoundaryBase shouldn't be used as a base class - // for all components indiscriminately, because that will lead to undesirable error-ignoring - // patterns. We expect that subclassing ErrorBoundaryBase to be done mainly by platform - // authors, providing just a default error UI for their rendering technology and not - // customizing other aspects of the semantics, such as whether to re-render after an error. - - private RenderHandle _renderHandle; - private Exception? _currentException; - /// /// The content to be displayed when there is no error. /// @@ -39,42 +28,20 @@ public abstract class ErrorBoundaryBase : IComponent, IErrorBoundary /// [Parameter] public bool AutoReset { get; set; } - /// - public void Attach(RenderHandle renderHandle) - { - _renderHandle = renderHandle; - } + /// + /// Gets the current exception, or null if there is no exception. + /// + protected Exception? CurrentException { get; private set; } /// - public Task SetParametersAsync(ParameterView parameters) + public override Task SetParametersAsync(ParameterView parameters) { - foreach (var parameter in parameters) - { - if (parameter.Name.Equals(nameof(ChildContent), StringComparison.OrdinalIgnoreCase)) - { - ChildContent = (RenderFragment)parameter.Value; - } - else if (parameter.Name.Equals(nameof(ErrorContent), StringComparison.OrdinalIgnoreCase)) - { - ErrorContent = (RenderFragment)parameter.Value; - } - else if (parameter.Name.Equals(nameof(AutoReset), StringComparison.OrdinalIgnoreCase)) - { - AutoReset = (bool)parameter.Value; - } - else - { - throw new ArgumentException($"The component '{GetType().FullName}' does not accept a parameter with the name '{parameter.Name}'."); - } - } - if (AutoReset) { - _currentException = null; + CurrentException = null; } - _renderHandle.Render(Render); - return Task.CompletedTask; + return base.SetParametersAsync(parameters); } /// @@ -83,17 +50,9 @@ public Task SetParametersAsync(ParameterView parameters) /// The being handled. protected abstract ValueTask LogExceptionAsync(Exception exception); - /// - /// Renders the default error content. This will only be used when - /// was not supplied. - /// - /// The - /// The current exception. - protected abstract void RenderDefaultErrorContent(RenderTreeBuilder builder, Exception exception); - void IErrorBoundary.HandleException(Exception exception) { - if (_currentException is not null) + if (CurrentException is not null) { // If there's an error while we're already displaying error content, then it's the // error content that's failing. Avoid the risk of an infinite error rendering loop. @@ -101,48 +60,39 @@ void IErrorBoundary.HandleException(Exception exception) } var logExceptionTask = LogExceptionAsync(exception); - _currentException = exception; - _renderHandle.Render(Render); - - // If the logger is failing, show its exception in preference to showing the original - // exception, since otherwise the developer has no way to discover it. if (!logExceptionTask.IsCompletedSuccessfully) { - _ = HandleAnyLoggingErrors(logExceptionTask); + _ = HandleLoggingErrors(logExceptionTask); } - } - private async Task HandleAnyLoggingErrors(ValueTask logExceptionTask) - { - try - { - await logExceptionTask; - } - catch (Exception exception) - { - _currentException = exception; - _renderHandle.Render(Render); - } + CurrentException = exception; + StateHasChanged(); } - private void Render(RenderTreeBuilder builder) + private async Task HandleLoggingErrors(ValueTask logExceptionTask) { - if (_currentException is null) - { - builder.AddContent(0, ChildContent); - } - else if (ErrorContent is not null) + if (logExceptionTask.IsFaulted) { - builder.AddContent(1, ErrorContent(_currentException)); + // Synchronous logging exceptions can simply be fatal to the circuit + ExceptionDispatchInfo.Capture(logExceptionTask.AsTask().Exception!).Throw(); } else { - // Even if the subclass tries to re-render the same ChildContent as its default error content, - // we still won't reuse the subtree components because they are in a different region. So we - // can be sure the old tree will be correctly disposed. - builder.OpenRegion(2); - RenderDefaultErrorContent(builder, _currentException); - builder.CloseRegion(); + // Async logging 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 logExceptionTask; + } + catch (Exception exception) + { + CurrentException = exception; + ChildContent = _ => ExceptionDispatchInfo.Capture(exception).Throw(); + ErrorContent = _ => _ => ExceptionDispatchInfo.Capture(exception).Throw(); + StateHasChanged(); + } } } } diff --git a/src/Components/Components/src/PublicAPI.Unshipped.txt b/src/Components/Components/src/PublicAPI.Unshipped.txt index 97fa4530f9db..12e878f1ecfe 100644 --- a/src/Components/Components/src/PublicAPI.Unshipped.txt +++ b/src/Components/Components/src/PublicAPI.Unshipped.txt @@ -10,15 +10,14 @@ Microsoft.AspNetCore.Components.ComponentApplicationState.PersistState(string! k Microsoft.AspNetCore.Components.ComponentApplicationState.TryTakeAsJson(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.Attach(Microsoft.AspNetCore.Components.RenderHandle renderHandle) -> void Microsoft.AspNetCore.Components.ErrorBoundaryBase.AutoReset.get -> bool Microsoft.AspNetCore.Components.ErrorBoundaryBase.AutoReset.set -> void 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? Microsoft.AspNetCore.Components.ErrorBoundaryBase.ErrorContent.set -> void -Microsoft.AspNetCore.Components.ErrorBoundaryBase.SetParametersAsync(Microsoft.AspNetCore.Components.ParameterView parameters) -> System.Threading.Tasks.Task! Microsoft.AspNetCore.Components.Lifetime.ComponentApplicationLifetime Microsoft.AspNetCore.Components.Lifetime.ComponentApplicationLifetime.ComponentApplicationLifetime(Microsoft.Extensions.Logging.ILogger! logger) -> void Microsoft.AspNetCore.Components.Lifetime.ComponentApplicationLifetime.PersistStateAsync(Microsoft.AspNetCore.Components.Lifetime.IComponentApplicationStateStore! store, Microsoft.AspNetCore.Components.RenderTree.Renderer! renderer) -> System.Threading.Tasks.Task! @@ -44,7 +43,7 @@ Microsoft.AspNetCore.Components.CascadingTypeParameterAttribute.CascadingTypePar Microsoft.AspNetCore.Components.CascadingTypeParameterAttribute.Name.get -> string! Microsoft.AspNetCore.Components.RenderTree.Renderer.GetEventArgsType(ulong eventHandlerId) -> System.Type! abstract Microsoft.AspNetCore.Components.ErrorBoundaryBase.LogExceptionAsync(System.Exception! exception) -> System.Threading.Tasks.ValueTask -abstract Microsoft.AspNetCore.Components.ErrorBoundaryBase.RenderDefaultErrorContent(Microsoft.AspNetCore.Components.Rendering.RenderTreeBuilder! builder, System.Exception! exception) -> void +override Microsoft.AspNetCore.Components.ErrorBoundaryBase.SetParametersAsync(Microsoft.AspNetCore.Components.ParameterView parameters) -> 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! 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! diff --git a/src/Components/Components/src/RenderTree/Renderer.cs b/src/Components/Components/src/RenderTree/Renderer.cs index 95db63599a8e..38b7d0f0372c 100644 --- a/src/Components/Components/src/RenderTree/Renderer.cs +++ b/src/Components/Components/src/RenderTree/Renderer.cs @@ -396,7 +396,7 @@ private bool TrySendExceptionToErrorBoundary(IComponent? errorSource, Exception { if (componentState.Component is IErrorBoundary errorBoundary) { - // Even though ErrorBoundaryBase always removes its ChildContent from the tree when + // Even though .Web's ErrorBoundary by default removes its ChildContent from the tree when // switching into an error state, the Renderer doesn't rely on that. To guarantee that // the failed subtree is disposed, forcibly remove it here. If the failed components did // continue to run, it wouldn't harm framework state, but it would be a whole new kind of diff --git a/src/Components/Samples/BlazorServerApp/Shared/ErrorIgnorer.cs b/src/Components/Samples/BlazorServerApp/Shared/ErrorIgnorer.cs index 30928077c0f1..c771c762af90 100644 --- a/src/Components/Samples/BlazorServerApp/Shared/ErrorIgnorer.cs +++ b/src/Components/Samples/BlazorServerApp/Shared/ErrorIgnorer.cs @@ -1,23 +1,15 @@ -using System; -using System.Threading.Tasks; -using Microsoft.AspNetCore.Components; using Microsoft.AspNetCore.Components.Rendering; +using Microsoft.AspNetCore.Components.Web; namespace BlazorServerApp.Shared { // A badly-behaved error boundary that tries to keep using its same ChildContent even after an error // This is to check we still don't retain the descendant component instances - public class ErrorIgnorer : ErrorBoundaryBase + public class ErrorIgnorer : ErrorBoundary { - protected override ValueTask LogExceptionAsync(Exception exception) + protected override void BuildRenderTree(RenderTreeBuilder builder) { - Console.WriteLine($"There was an error, but we'll try to ignore it. La la la la, can't year you. [{exception}]"); - return ValueTask.CompletedTask; - } - - protected override void RenderDefaultErrorContent(RenderTreeBuilder builder, Exception exception) - { - ChildContent!(builder); + builder.AddContent(0, ChildContent); } } } diff --git a/src/Components/Server/src/Circuits/RemoteErrorBoundaryLogger.cs b/src/Components/Server/src/Circuits/RemoteErrorBoundaryLogger.cs index a2e5cff81631..6fa23c40a9b9 100644 --- a/src/Components/Server/src/Circuits/RemoteErrorBoundaryLogger.cs +++ b/src/Components/Server/src/Circuits/RemoteErrorBoundaryLogger.cs @@ -28,15 +28,10 @@ public RemoteErrorBoundaryLogger(ILogger logger, IJSRuntime jsRun _options = options.Value; } - public ValueTask LogErrorAsync(Exception exception, bool clientOnly) + public ValueTask LogErrorAsync(Exception exception) { - // If the end user clicks on the UI to re-log the error, we don't want to spam the - // server logs so we only re-load to the client - if (!clientOnly) - { - // We always log detailed information to the server-side log - _exceptionCaughtByErrorBoundary(_logger, exception.Message, exception); - } + // We always log detailed information to the server-side log + _exceptionCaughtByErrorBoundary(_logger, exception.Message, exception); // We log to the client only if the browser is connected interactively, and even then // we may suppress the details diff --git a/src/Components/Web/src/PublicAPI.Unshipped.txt b/src/Components/Web/src/PublicAPI.Unshipped.txt index fbeafd68bf5e..8c172dbb7cae 100644 --- a/src/Components/Web/src/PublicAPI.Unshipped.txt +++ b/src/Components/Web/src/PublicAPI.Unshipped.txt @@ -17,7 +17,9 @@ Microsoft.AspNetCore.Components.Forms.InputTextArea.Element.set -> void Microsoft.AspNetCore.Components.Web.ErrorBoundary Microsoft.AspNetCore.Components.Web.ErrorBoundary.ErrorBoundary() -> void Microsoft.AspNetCore.Components.Web.IErrorBoundaryLogger -Microsoft.AspNetCore.Components.Web.IErrorBoundaryLogger.LogErrorAsync(System.Exception! exception, bool clientOnly) -> System.Threading.Tasks.ValueTask +Microsoft.AspNetCore.Components.Web.IErrorBoundaryLogger.LogErrorAsync(System.Exception! exception) -> System.Threading.Tasks.ValueTask +override Microsoft.AspNetCore.Components.Web.ErrorBoundary.BuildRenderTree(Microsoft.AspNetCore.Components.Rendering.RenderTreeBuilder! builder) -> void +override Microsoft.AspNetCore.Components.Web.ErrorBoundary.LogExceptionAsync(System.Exception! exception) -> System.Threading.Tasks.ValueTask static Microsoft.AspNetCore.Components.ElementReferenceExtensions.FocusAsync(this Microsoft.AspNetCore.Components.ElementReference elementReference, bool preventScroll) -> System.Threading.Tasks.ValueTask static Microsoft.AspNetCore.Components.Forms.BrowserFileExtensions.RequestImageFileAsync(this Microsoft.AspNetCore.Components.Forms.IBrowserFile! browserFile, string! format, int maxWidth, int maxHeight) -> System.Threading.Tasks.ValueTask Microsoft.AspNetCore.Components.RenderTree.WebEventDescriptor.EventName.get -> string! diff --git a/src/Components/Web/src/Web/ErrorBoundary.cs b/src/Components/Web/src/Web/ErrorBoundary.cs index cf0324794a91..37edc0f73c49 100644 --- a/src/Components/Web/src/Web/ErrorBoundary.cs +++ b/src/Components/Web/src/Web/ErrorBoundary.cs @@ -10,30 +10,42 @@ namespace Microsoft.AspNetCore.Components.Web /// /// Captures errors thrown from its child content. /// - public sealed class ErrorBoundary : ErrorBoundaryBase + public class ErrorBoundary : ErrorBoundaryBase { [Inject] private IErrorBoundaryLogger? ErrorBoundaryLogger { get; set; } /// protected override ValueTask LogExceptionAsync(Exception exception) { - return ErrorBoundaryLogger!.LogErrorAsync(exception, clientOnly: false); + return ErrorBoundaryLogger!.LogErrorAsync(exception); } /// - protected override void RenderDefaultErrorContent(RenderTreeBuilder builder, Exception exception) + protected override void BuildRenderTree(RenderTreeBuilder builder) { - // TODO: Better UI (some kind of language-independent icon, CSS stylability) - // Could it simply be
    , with all the - // content provided in template CSS? Is this even going to be used in the - // template by default? It's probably best have some default UI that doesn't - // rely on there being any CSS, but make it overridable via CSS. - builder.OpenElement(0, "div"); - builder.AddAttribute(1, "onclick", (Func)(_ => - // Re-log, to help the developer figure out which ErrorBoundary issued which log message - ErrorBoundaryLogger!.LogErrorAsync(exception, clientOnly: true))); - builder.AddContent(1, "Error"); - builder.CloseElement(); + if (CurrentException is null) + { + builder.AddContent(0, ChildContent); + } + else if (ErrorContent is not null) + { + builder.AddContent(1, ErrorContent(CurrentException)); + } + else + { + // The default error UI doesn't include any content, because: + // [1] We don't know whether or not you'd be happy to show the stack trace. It depends both on + // whether DetailedErrors is enabled and whether you're in production, because even on WebAssembly + // you likely don't want to put technical data like that in the UI for end users. A reasonable way + // to toggle this is via something like "#if DEBUG" but that can only be done in user code. + // [2] We can't have any other human-readable content by default, because it would need to be valid + // for all languages. + // Instead, the default project template provides locale-specific default content via CSS. This provides + // a quick form of customization even without having to subclass this component. + builder.OpenElement(2, "div"); + builder.AddAttribute(3, "class", "error-boundary"); + builder.CloseElement(); + } } } } diff --git a/src/Components/Web/src/Web/IErrorBoundaryLogger.cs b/src/Components/Web/src/Web/IErrorBoundaryLogger.cs index 6d616a5bca52..5589f9bb731f 100644 --- a/src/Components/Web/src/Web/IErrorBoundaryLogger.cs +++ b/src/Components/Web/src/Web/IErrorBoundaryLogger.cs @@ -21,8 +21,7 @@ public interface IErrorBoundaryLogger /// Logs the supplied . ///     /// The to log. - /// If true, indicates that the error should only be logged to the client (e.g., because it was already logged to the server). /// A representing the completion of the operation. - ValueTask LogErrorAsync(Exception exception, bool clientOnly); + ValueTask LogErrorAsync(Exception exception); } } diff --git a/src/Components/WebAssembly/WebAssembly/src/Services/WebAssemblyErrorBoundaryLogger.cs b/src/Components/WebAssembly/WebAssembly/src/Services/WebAssemblyErrorBoundaryLogger.cs index 736a22254905..eac12ba48e33 100644 --- a/src/Components/WebAssembly/WebAssembly/src/Services/WebAssemblyErrorBoundaryLogger.cs +++ b/src/Components/WebAssembly/WebAssembly/src/Services/WebAssemblyErrorBoundaryLogger.cs @@ -17,7 +17,7 @@ public WebAssemblyErrorBoundaryLogger(ILogger errorBoundaryLogger _errorBoundaryLogger = errorBoundaryLogger ?? throw new ArgumentNullException(nameof(errorBoundaryLogger)); ; } - public ValueTask LogErrorAsync(Exception exception, bool clientOnly) + public ValueTask LogErrorAsync(Exception exception) { // For, client-side code, all internal state is visible to the end user. We can just // log directly to the console. diff --git a/src/Mvc/Mvc.ViewFeatures/src/RazorComponents/PrerenderingErrorBoundaryLogger.cs b/src/Mvc/Mvc.ViewFeatures/src/RazorComponents/PrerenderingErrorBoundaryLogger.cs index 34fae7dd21fd..741bd2f88dd3 100644 --- a/src/Mvc/Mvc.ViewFeatures/src/RazorComponents/PrerenderingErrorBoundaryLogger.cs +++ b/src/Mvc/Mvc.ViewFeatures/src/RazorComponents/PrerenderingErrorBoundaryLogger.cs @@ -22,7 +22,7 @@ public PrerenderingErrorBoundaryLogger(ILogger logger) _logger = logger; } - public ValueTask LogErrorAsync(Exception exception, bool clientOnly) + public ValueTask LogErrorAsync(Exception exception) { _exceptionCaughtByErrorBoundary(_logger, exception.Message, exception); return ValueTask.CompletedTask; From a28c3cabf46f0770cf3d09eaa046597599b42d42 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Tue, 6 Apr 2021 12:29:26 +0100 Subject: [PATCH 25/55] Better default UI --- .../Samples/BlazorServerApp/wwwroot/css/site.css | 10 ++++++++++ src/Components/Web/src/Web/ErrorBoundary.cs | 2 +- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/Components/Samples/BlazorServerApp/wwwroot/css/site.css b/src/Components/Samples/BlazorServerApp/wwwroot/css/site.css index bdca6fbfcb87..b95bf4a241f2 100644 --- a/src/Components/Samples/BlazorServerApp/wwwroot/css/site.css +++ b/src/Components/Samples/BlazorServerApp/wwwroot/css/site.css @@ -118,6 +118,16 @@ app { color: red; } +.blazor-error-boundary { + background: url(data:image/svg+xml;base64,PHN2ZyB3aWR0aD0iNTYiIGhlaWdodD0iNDkiIHhtbG5zPSJodHRwOi8vd3d3LnczLm9yZy8yMDAwL3N2ZyIgeG1sbnM6eGxpbms9Imh0dHA6Ly93d3cudzMub3JnLzE5OTkveGxpbmsiIG92ZXJmbG93PSJoaWRkZW4iPjxkZWZzPjxjbGlwUGF0aCBpZD0iY2xpcDAiPjxyZWN0IHg9IjIzNSIgeT0iNTEiIHdpZHRoPSI1NiIgaGVpZ2h0PSI0OSIvPjwvY2xpcFBhdGg+PC9kZWZzPjxnIGNsaXAtcGF0aD0idXJsKCNjbGlwMCkiIHRyYW5zZm9ybT0idHJhbnNsYXRlKC0yMzUgLTUxKSI+PHBhdGggZD0iTTI2My41MDYgNTFDMjY0LjcxNyA1MSAyNjUuODEzIDUxLjQ4MzcgMjY2LjYwNiA1Mi4yNjU4TDI2Ny4wNTIgNTIuNzk4NyAyNjcuNTM5IDUzLjYyODMgMjkwLjE4NSA5Mi4xODMxIDI5MC41NDUgOTIuNzk1IDI5MC42NTYgOTIuOTk2QzI5MC44NzcgOTMuNTEzIDI5MSA5NC4wODE1IDI5MSA5NC42NzgyIDI5MSA5Ny4wNjUxIDI4OS4wMzggOTkgMjg2LjYxNyA5OUwyNDAuMzgzIDk5QzIzNy45NjMgOTkgMjM2IDk3LjA2NTEgMjM2IDk0LjY3ODIgMjM2IDk0LjM3OTkgMjM2LjAzMSA5NC4wODg2IDIzNi4wODkgOTMuODA3MkwyMzYuMzM4IDkzLjAxNjIgMjM2Ljg1OCA5Mi4xMzE0IDI1OS40NzMgNTMuNjI5NCAyNTkuOTYxIDUyLjc5ODUgMjYwLjQwNyA1Mi4yNjU4QzI2MS4yIDUxLjQ4MzcgMjYyLjI5NiA1MSAyNjMuNTA2IDUxWk0yNjMuNTg2IDY2LjAxODNDMjYwLjczNyA2Ni4wMTgzIDI1OS4zMTMgNjcuMTI0NSAyNTkuMzEzIDY5LjMzNyAyNTkuMzEzIDY5LjYxMDIgMjU5LjMzMiA2OS44NjA4IDI1OS4zNzEgNzAuMDg4N0wyNjEuNzk1IDg0LjAxNjEgMjY1LjM4IDg0LjAxNjEgMjY3LjgyMSA2OS43NDc1QzI2Ny44NiA2OS43MzA5IDI2Ny44NzkgNjkuNTg3NyAyNjcuODc5IDY5LjMxNzkgMjY3Ljg3OSA2Ny4xMTgyIDI2Ni40NDggNjYuMDE4MyAyNjMuNTg2IDY2LjAxODNaTTI2My41NzYgODYuMDU0N0MyNjEuMDQ5IDg2LjA1NDcgMjU5Ljc4NiA4Ny4zMDA1IDI1OS43ODYgODkuNzkyMSAyNTkuNzg2IDkyLjI4MzcgMjYxLjA0OSA5My41Mjk1IDI2My41NzYgOTMuNTI5NSAyNjYuMTE2IDkzLjUyOTUgMjY3LjM4NyA5Mi4yODM3IDI2Ny4zODcgODkuNzkyMSAyNjcuMzg3IDg3LjMwMDUgMjY2LjExNiA4Ni4wNTQ3IDI2My41NzYgODYuMDU0N1oiIGZpbGw9IiNGRkU1MDAiIGZpbGwtcnVsZT0iZXZlbm9kZCIvPjwvZz48L3N2Zz4=) no-repeat 1rem/1.8rem, #b32121; + padding: 1rem 1rem 1rem 3.7rem; + color: white; +} + + .blazor-error-boundary::after { + content: "An error has occurred." + } + #blazor-error-ui { background: lightyellow; bottom: 0; diff --git a/src/Components/Web/src/Web/ErrorBoundary.cs b/src/Components/Web/src/Web/ErrorBoundary.cs index 37edc0f73c49..8ab4c9e30c80 100644 --- a/src/Components/Web/src/Web/ErrorBoundary.cs +++ b/src/Components/Web/src/Web/ErrorBoundary.cs @@ -43,7 +43,7 @@ protected override void BuildRenderTree(RenderTreeBuilder builder) // Instead, the default project template provides locale-specific default content via CSS. This provides // a quick form of customization even without having to subclass this component. builder.OpenElement(2, "div"); - builder.AddAttribute(3, "class", "error-boundary"); + builder.AddAttribute(3, "class", "blazor-error-boundary"); builder.CloseElement(); } } From 275ae5aee2c343204aebc92437a618f38143dd97 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Tue, 6 Apr 2021 12:38:28 +0100 Subject: [PATCH 26/55] Show you can subclass ErrorBoundary helpfully --- .../BlazorServerApp/Pages/ErrorMaker.razor | 7 +++++++ .../Shared/CustomErrorBoundary.razor | 19 +++++++++++++++++++ 2 files changed, 26 insertions(+) create mode 100644 src/Components/Samples/BlazorServerApp/Shared/CustomErrorBoundary.razor diff --git a/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor b/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor index 8e62939bcb54..da8d210a926f 100644 --- a/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor +++ b/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor @@ -46,6 +46,13 @@ +
    + Custom error boundary component + + + +
    +
    Exception inline in error boundary markup diff --git a/src/Components/Samples/BlazorServerApp/Shared/CustomErrorBoundary.razor b/src/Components/Samples/BlazorServerApp/Shared/CustomErrorBoundary.razor new file mode 100644 index 000000000000..5e5feb5476ac --- /dev/null +++ b/src/Components/Samples/BlazorServerApp/Shared/CustomErrorBoundary.razor @@ -0,0 +1,19 @@ +@inherits ErrorBoundary +@if (CurrentException is null) +{ + @ChildContent +} +else if (ErrorContent is not null) +{ + @ErrorContent(CurrentException) +} +else +{ +
    + @{ #if DEBUG } + @CurrentException.ToString() + @{ #else } +

    Sorry, there was a problem.

    + @{ #endif } +
    +} From d319b9fc2bc869f68a9bfac9950bdf1f31ec4cd1 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Tue, 6 Apr 2021 12:41:39 +0100 Subject: [PATCH 27/55] Rename AutoReset->AutoRecover --- src/Components/Components/src/ErrorBoundaryBase.cs | 4 ++-- src/Components/Components/src/PublicAPI.Unshipped.txt | 4 ++-- .../Samples/BlazorServerApp/Shared/MainLayout.razor | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Components/Components/src/ErrorBoundaryBase.cs b/src/Components/Components/src/ErrorBoundaryBase.cs index d1d4cdae6a28..88d713d8ab3b 100644 --- a/src/Components/Components/src/ErrorBoundaryBase.cs +++ b/src/Components/Components/src/ErrorBoundaryBase.cs @@ -26,7 +26,7 @@ public abstract class ErrorBoundaryBase : ComponentBase, IErrorBoundary /// Specifies whether to reset the error state each time this component instance is rendered /// by its parent. This allows the child content to be recreated in an attempt to recover from the error. /// - [Parameter] public bool AutoReset { get; set; } + [Parameter] public bool AutoRecover { get; set; } /// /// Gets the current exception, or null if there is no exception. @@ -36,7 +36,7 @@ public abstract class ErrorBoundaryBase : ComponentBase, IErrorBoundary /// public override Task SetParametersAsync(ParameterView parameters) { - if (AutoReset) + if (AutoRecover) { CurrentException = null; } diff --git a/src/Components/Components/src/PublicAPI.Unshipped.txt b/src/Components/Components/src/PublicAPI.Unshipped.txt index 12e878f1ecfe..c93f5d5ffddc 100644 --- a/src/Components/Components/src/PublicAPI.Unshipped.txt +++ b/src/Components/Components/src/PublicAPI.Unshipped.txt @@ -10,8 +10,8 @@ Microsoft.AspNetCore.Components.ComponentApplicationState.PersistState(string! k Microsoft.AspNetCore.Components.ComponentApplicationState.TryTakeAsJson(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.AutoReset.get -> bool -Microsoft.AspNetCore.Components.ErrorBoundaryBase.AutoReset.set -> void +Microsoft.AspNetCore.Components.ErrorBoundaryBase.AutoRecover.get -> bool +Microsoft.AspNetCore.Components.ErrorBoundaryBase.AutoRecover.set -> void 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? diff --git a/src/Components/Samples/BlazorServerApp/Shared/MainLayout.razor b/src/Components/Samples/BlazorServerApp/Shared/MainLayout.razor index bbd879c05fba..fdf92c2799b2 100644 --- a/src/Components/Samples/BlazorServerApp/Shared/MainLayout.razor +++ b/src/Components/Samples/BlazorServerApp/Shared/MainLayout.razor @@ -10,7 +10,7 @@
    - + @Body
    From 15ab863fdef639e105459c84a1fd3b31b0656ab7 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Tue, 6 Apr 2021 13:38:15 +0100 Subject: [PATCH 28/55] Remove use of linq --- .../Components/src/RenderTree/Renderer.cs | 21 ++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/src/Components/Components/src/RenderTree/Renderer.cs b/src/Components/Components/src/RenderTree/Renderer.cs index 38b7d0f0372c..3f6781e068f3 100644 --- a/src/Components/Components/src/RenderTree/Renderer.cs +++ b/src/Components/Components/src/RenderTree/Renderer.cs @@ -7,7 +7,6 @@ using System.Collections.Generic; using System.Diagnostics; using System.Diagnostics.CodeAnalysis; -using System.Linq; using System.Runtime.ExceptionServices; using System.Threading; using System.Threading.Tasks; @@ -387,11 +386,8 @@ private bool TrySendExceptionToErrorBoundary(IComponent? errorSource, Exception // already on the sync context (and if not, we have a bug we want to know about). Dispatcher.AssertAccess(); - // Slow linear search is fine because this is only during exception handling. - // If it becomes problematic, we can maintain a lookup from component instance to ID. - var componentState = _componentStateById.FirstOrDefault(kvp => kvp.Value.Component == errorSource).Value; - // Find the closest error boundary, if any + var componentState = FindComponentState(errorSource); while (componentState is not null) { if (componentState.Component is IErrorBoundary errorBoundary) @@ -428,6 +424,21 @@ private bool TrySendExceptionToErrorBoundary(IComponent? errorSource, Exception } return false; + + ComponentState? FindComponentState(IComponent errorSource) + { + // Slow linear search is fine because this is only during exception handling. + // If it becomes problematic, we can maintain a lookup from component instance to ID. + foreach (var value in _componentStateById.Values) + { + if (value.Component == errorSource) + { + return value; + } + } + + return null; + } } /// From 990128eee6641fa507a61a8eb70d0f25f4154f4f Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Tue, 6 Apr 2021 13:53:38 +0100 Subject: [PATCH 29/55] Tidy up comments --- .../Components/src/RenderTree/Renderer.cs | 34 ++++--------------- .../src/Rendering/ComponentState.cs | 12 ++----- .../Web/src/Web/IErrorBoundaryLogger.cs | 7 ++-- 3 files changed, 11 insertions(+), 42 deletions(-) diff --git a/src/Components/Components/src/RenderTree/Renderer.cs b/src/Components/Components/src/RenderTree/Renderer.cs index 3f6781e068f3..1c1854590d30 100644 --- a/src/Components/Components/src/RenderTree/Renderer.cs +++ b/src/Components/Components/src/RenderTree/Renderer.cs @@ -337,24 +337,9 @@ public virtual Task DispatchEventAsync(ulong eventHandlerId, EventFieldInfo? fie } catch (Exception e) { - // Exceptions in event handlers don't compromise the stability of the framework, since the - // framework wasn't processing any internal state that might have been left corrupted. The - // risk is that user code is now left in some invalid state. So: - // - // * If we can identify a strong enough candidate for which component subtree is at risk - // of being affected, we'll shut down that subtree. The conditions for this are: - // * The event callback was dispatched to an IComponent - // * ... which is still live in the hierarchy - // * ... and is within an error boundary - // * Otherwise, we're in a more ambiguous case, such as event callbacks being dispatched - // to non-components. In this case, we'll treat it as fatal for the entire renderer. - // - // This is pretty subjective. Even in the "safe" case, it's possible that some non-component - // (domain model) state has been left corrupted. But we have to let developers be responsible - // for keeping their non-component state valid otherwise we couldn't have any error recovery. - var recovered = TrySendExceptionToErrorBoundary(componentReceiver, e); - if (!recovered) + if (!TrySendExceptionToErrorBoundary(componentReceiver, e)) { + // It's unhandled, so treat as fatal HandleException(e); } @@ -401,19 +386,13 @@ private bool TrySendExceptionToErrorBoundary(IComponent? errorSource, Exception try { - // We don't log the error here, and instead leave it up to the IErrorBoundary to do - // what it wants (which could be suppressing the error entirely). Note that the default - // logging behavior has to vary between hosting models. - // TODO: Are we happy with letting IErrorBoundary suppress errors if it wants? errorBoundary.HandleException(error); } catch (Exception errorBoundaryException) { // If *notifying* about an exception fails, it's OK for that to be fatal. This is // different from an IErrorBoundary component throwing during its own rendering or - // lifecycle methods. - // TODO: Should we support rethrowing from inside HandleException? I prefer not to - // unless we get a better justification. See design doc. + // lifecycle methods, which can be handled that error boundary or an ancestor. HandleException(errorBoundaryException); } @@ -477,8 +456,6 @@ internal void InstantiateChildComponentOnFrame(ref RenderTreeFrame frame, int pa frame.ComponentIdField = newComponentState.ComponentId; } - // TODO: Need to reason about all the code paths that lead here and ensure it makes sense - // to treat the exceptions as handleable internal void AddToPendingTasks(Task task, IComponent? owningComponent) { switch (task == null ? TaskStatus.RanToCompletion : task.Status) @@ -839,7 +816,8 @@ private void RenderInExistingBatch(RenderQueueEntry renderQueueEntry) } else { - AddToPendingTasks(GetHandledAsynchronousDisposalErrorsTask(result), null); + // We set owningComponent to null because we don't want exceptions during disposal to be recoverable + AddToPendingTasks(GetHandledAsynchronousDisposalErrorsTask(result), owningComponent: null); async Task GetHandledAsynchronousDisposalErrorsTask(Task result) { @@ -930,7 +908,7 @@ private async Task GetErrorHandledTask(Task taskToHandle, IComponent? owningComp { if (!TrySendExceptionToErrorBoundary(owningComponent, ex)) { - // It's unhandled + // It's unhandled, so treat as fatal HandleException(ex); } } diff --git a/src/Components/Components/src/Rendering/ComponentState.cs b/src/Components/Components/src/Rendering/ComponentState.cs index d496d1186f43..e41fd4f583c0 100644 --- a/src/Components/Components/src/Rendering/ComponentState.cs +++ b/src/Components/Components/src/Rendering/ComponentState.cs @@ -74,11 +74,8 @@ public void RenderIntoBatch(RenderBatchBuilder batchBuilder, RenderFragment rend catch (Exception ex) { // If an exception occurs in the render fragment delegate, we won't process the diff in any way, so child components, - // event handlers, etc., will all be left untouched as if this component didn't re-render at all. We also are careful - // to leave ComponentState in an internally-consistent state so that technically this component could continue to be - // used. However, the Renderer will not allow the component to continue to be used, except if it's the IErrorBoundary, - // because it forcibly clears the descendants of the IErrorBoundary before notifying it. - // TODO: Verify that having a try/catch here doesn't degrade perf noticeably on WebAssembly. It might do. + // event handlers, etc., will all be left untouched as if this component didn't re-render at all. The Renderer will + // then forcibly clear the descendant subtree by rendering an empty fragment for this component. renderFragmentException = ex; return; } @@ -199,10 +196,7 @@ public void NotifyCascadingValueChanged(in ParameterViewLifetime lifetime) // a consistent set to the recipient. private void SupplyCombinedParameters(ParameterView directAndCascadingParameters) { - // Normalise sync and async exceptions into a Task, as there's no value in differentiating - // between "it returned an already-faulted Task" and "it just threw". - // TODO: This is the most invasive part of this whole work area on perf. Need to validate - // that this doesn't regress WebAssembly rendering perf too much in intense cases. + // Normalise sync and async exceptions into a Task Task setParametersAsyncTask; try { diff --git a/src/Components/Web/src/Web/IErrorBoundaryLogger.cs b/src/Components/Web/src/Web/IErrorBoundaryLogger.cs index 5589f9bb731f..69ddd0e45f2a 100644 --- a/src/Components/Web/src/Web/IErrorBoundaryLogger.cs +++ b/src/Components/Web/src/Web/IErrorBoundaryLogger.cs @@ -6,11 +6,8 @@ namespace Microsoft.AspNetCore.Components.Web { - // The rules arround logging differ between environments. - // - For WebAssembly, we always log to the console with detailed information - // - For Server, we log to both the server-side log (with detailed information), and to the - // client (respecting the DetailedError option) - // - In prerendering, we log only to the server-side log + // The reason this abstraction exists is that logging behaviors differ across hosting platforms. + // For example, Blazor Server logs to both the server and client, whereas WebAssembly has only one log. /// /// Logs exception information for a component. From c9c7c6fc9b0b42cda65fd44802353a809b9dce29 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Tue, 6 Apr 2021 14:13:44 +0100 Subject: [PATCH 30/55] Clean up and expand test cases --- .../BlazorServerApp/Pages/ErrorMaker.razor | 176 +++++++----------- 1 file changed, 71 insertions(+), 105 deletions(-) diff --git a/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor b/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor index da8d210a926f..581fc33e726b 100644 --- a/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor +++ b/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor @@ -1,119 +1,85 @@ @page "/errormaker" -
    - Error maker +

    Event handlers

    +

    These errors will be caught by the closest error boundary ancestor.

    + + -
    - Event handlers - - -
    +
    +

    Lifecycle methods

    +

    These errors will be caught by the closest error boundary ancestor.

    +
    +
    +
    +
    +
    +
    -
    - Lifecycle methods -
    -
    -
    -
    -
    -
    + + + + + - - - - - -
    - -
    - Rendering - - @if (throwWhileRendering) - { - throw new InvalidTimeZoneException($"Exception from {nameof(BuildRenderTree)}"); - } -
    - -
    - Trying to ignore errors - - - -
    - -
    - Custom error boundary component - - - -
    +
    +

    Rendering

    +

    These errors will be caught by the closest error boundary ancestor.

    + +@if (throwWhileRendering) +{ + throw new InvalidTimeZoneException($"Exception from {nameof(BuildRenderTree)}"); +} -
    - Exception inline in error boundary markup - - - @if (throwInline) { throw new InvalidTimeZoneException("Inline exception"); } -

    Hello!

    -     - - @if (throwInErrorContent) { throw new InvalidTimeZoneException("Inline exception in error content"); } -

    There was an error: @context

    -     -     - - -
    -
    +
    +

    AutoRecover

    +

    These errors will be caught by the inline error boundary. After recovery, the counter state will be reset, because it's a new instance.

    + + + + + +

    Here is some custom error content. If you re-render the container, I'll try to recover.

    +     +     + -@code { - // A weird subtlety is what happens if you do: - // Some inline content that may throw, either in a event-handling lambda or on render - // Syntactically it looks as if the error boundary will take care of errors in that content inside itself, but - // actually: - // [1] Any error-throwing lambdas in there are mapped to the host component outside the , since - // that's where the delegate lives. So those errors will not be handled by this error boundary. - // [2] Any rendering errors in there are explicitly *not* dispatched to that because it would - // commonly be an infinite error loop. So again, those errors will go up to any boundaries in ancestors. - // People will have to understand that error boundaries are responsible for their descendants, not for themselves. - // However on consideration I think we should probably change rule [2], as long as there is some mechanism to - // detect infinite error loops. +
    +

    Custom error boundary

    +

    This shows how to create a common custom error UI by subclassing ErrorBoundary.

    - // Note that unhandled exceptions inside InvokeAsync are already non-fatal, since we simply return them to the - // upstream caller who decides what to do. We lack any API for transferring execution into the sync context - // that returns void and takes care of its own error handling (besides the complicated trick of stashing an - // exception in a field and triggering a render that then throws during rendering). If we add such an API - // in the future, it would be a method on ComponentBase that internally does InvokeAsync to a delegate that - // does try/catch around your Action/Func, and if there's an exception, calls some new method on RenderHandle - // that passes the exception and IComponent instance onto the Renderer, which then uses TrySendExceptionToErrorBoundary - // and falls back on HandleError (fatal). We could call it something like Execute/ExecuteAsync (both void), - // or maybe InvokeAndHandle/InvokeAndHandleAsync (both void). + + + - // I'm proposing not to allow handling exceptions that occur during either IComponentActivator.CreateInstance - // (e.g., component constructor) or IComponent.Attach, because: - // [1] For most applications, these won't run any developer-supplied code, and will only run the framework's - // built-in logic (e.g., ComponentBase.Attach which can't throw). The value here is pretty low. - // [2] Supporting recovery from these is difficult and leads to entirely new states, such as a "component" RenderTreeFrame - // not actually having an associated ComponentState because we never initialized it in the first place. - // These particular lifecycle points are outside the scope of error recovery that could be achieved in user code today - // (via the "cascading a custom ErrorBoundary-type component" technique), so are outside the scope of this work. We retain - // the option of making component initialization errors recoverable in the future, as I think it's fair to say that would - // not be a breaking change, as we'd be replacing a previously-fatal error with a nonfatal one. - // - Actually we don't need to handle it for these anyway, because in these cases the exception will just bubble - // up to the SetParametersAsync on the parent, and become a handleable error at that level. +
    +

    Custom error boundary that tries to ignore errors

    +

    This shows that, even if a custom error boundary tries to continue rendering in a non-error state after an error, the subtree will be forcibly rebuilt. If there's an error while already in an error state, it's fatal.

    + + + - // After a lot of consideration, I don't think we should allow exceptions during Dispose/DisposeAsync to be recoverable. - // We could do so, since components are only disposed once they are removed from their parents, so even if their disposal - // fails we know they are gone from the component hierarchy anyway, and we can still be sure to clean up all the framework - // managed resources for them and their descendants. However, if disposal throws, then it's very likely that the application - // cleanup logic didn't complete properly, and hence the application is now leaking memory. It's not smart to have a default - // behavior that lets the developer get away with randomly incomplete cleanup. If a developer really wants to opt into this - // on a per-Dispose/DisposeAsync basis, they can use a try/catch there, and in most cases they should not do so. +
    +

    Exception inline in error boundary markup

    +

    This shows that, if an ErrorBoundary itself fails while rendering its own ChildContent, then it can catch its own exception (except if it's already showing an exception, in which case it's fatal).

    + + + @if (throwInline) { throw new InvalidTimeZoneException("Inline exception"); } +

    Hello!

    +     + + @if (throwInErrorContent) { throw new InvalidTimeZoneException("Inline exception in error content"); } +

    There was an error: @context

    +     +     + + +@code { private bool throwInOnParametersSet; private bool throwInOnParametersSetAsync; private bool throwInOnParametersSetViaCascading; From 2f597f4f2d79ebc96c6fe70390a25f818b6c5abc Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Tue, 6 Apr 2021 14:28:43 +0100 Subject: [PATCH 31/55] CR: Add general-purpose OnErrorAsync to base class --- .../Components/src/ErrorBoundaryBase.cs | 24 +++++++++---------- .../Components/src/PublicAPI.Unshipped.txt | 2 +- .../Web/src/PublicAPI.Unshipped.txt | 2 +- src/Components/Web/src/Web/ErrorBoundary.cs | 4 ++-- 4 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/Components/Components/src/ErrorBoundaryBase.cs b/src/Components/Components/src/ErrorBoundaryBase.cs index 88d713d8ab3b..87f7ca5dcf57 100644 --- a/src/Components/Components/src/ErrorBoundaryBase.cs +++ b/src/Components/Components/src/ErrorBoundaryBase.cs @@ -45,10 +45,10 @@ public override Task SetParametersAsync(ParameterView parameters) } /// - /// Logs the exception. + /// Invoked by the base class when an error is being handled. /// /// The being handled. - protected abstract ValueTask LogExceptionAsync(Exception exception); + protected abstract Task OnErrorAsync(Exception exception); void IErrorBoundary.HandleException(Exception exception) { @@ -59,32 +59,32 @@ void IErrorBoundary.HandleException(Exception exception) ExceptionDispatchInfo.Capture(exception).Throw(); } - var logExceptionTask = LogExceptionAsync(exception); - if (!logExceptionTask.IsCompletedSuccessfully) + var onErrorTask = OnErrorAsync(exception); + if (!onErrorTask.IsCompletedSuccessfully) { - _ = HandleLoggingErrors(logExceptionTask); + _ = HandleOnErrorExceptions(onErrorTask); } CurrentException = exception; StateHasChanged(); } - private async Task HandleLoggingErrors(ValueTask logExceptionTask) + private async Task HandleOnErrorExceptions(Task onExceptionTask) { - if (logExceptionTask.IsFaulted) + if (onExceptionTask.IsFaulted) { - // Synchronous logging exceptions can simply be fatal to the circuit - ExceptionDispatchInfo.Capture(logExceptionTask.AsTask().Exception!).Throw(); + // Synchronous error handling exceptions can simply be fatal to the circuit + ExceptionDispatchInfo.Capture(onExceptionTask.Exception!).Throw(); } else { - // Async logging exceptions are tricky because there's no natural way to bring them - // back onto the sync context within their original circuit. The closest approximation + // 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 logExceptionTask; + await onExceptionTask; } catch (Exception exception) { diff --git a/src/Components/Components/src/PublicAPI.Unshipped.txt b/src/Components/Components/src/PublicAPI.Unshipped.txt index c93f5d5ffddc..08da9abc29a3 100644 --- a/src/Components/Components/src/PublicAPI.Unshipped.txt +++ b/src/Components/Components/src/PublicAPI.Unshipped.txt @@ -42,7 +42,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.LogExceptionAsync(System.Exception! exception) -> System.Threading.Tasks.ValueTask +abstract Microsoft.AspNetCore.Components.ErrorBoundaryBase.OnErrorAsync(System.Exception! exception) -> System.Threading.Tasks.Task! override Microsoft.AspNetCore.Components.ErrorBoundaryBase.SetParametersAsync(Microsoft.AspNetCore.Components.ParameterView parameters) -> 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! parameters) -> Microsoft.AspNetCore.Components.ParameterView diff --git a/src/Components/Web/src/PublicAPI.Unshipped.txt b/src/Components/Web/src/PublicAPI.Unshipped.txt index 8c172dbb7cae..5cdd936a61ec 100644 --- a/src/Components/Web/src/PublicAPI.Unshipped.txt +++ b/src/Components/Web/src/PublicAPI.Unshipped.txt @@ -19,7 +19,7 @@ Microsoft.AspNetCore.Components.Web.ErrorBoundary.ErrorBoundary() -> void Microsoft.AspNetCore.Components.Web.IErrorBoundaryLogger Microsoft.AspNetCore.Components.Web.IErrorBoundaryLogger.LogErrorAsync(System.Exception! exception) -> System.Threading.Tasks.ValueTask override Microsoft.AspNetCore.Components.Web.ErrorBoundary.BuildRenderTree(Microsoft.AspNetCore.Components.Rendering.RenderTreeBuilder! builder) -> void -override Microsoft.AspNetCore.Components.Web.ErrorBoundary.LogExceptionAsync(System.Exception! exception) -> System.Threading.Tasks.ValueTask +override Microsoft.AspNetCore.Components.Web.ErrorBoundary.OnErrorAsync(System.Exception! exception) -> System.Threading.Tasks.Task! static Microsoft.AspNetCore.Components.ElementReferenceExtensions.FocusAsync(this Microsoft.AspNetCore.Components.ElementReference elementReference, bool preventScroll) -> System.Threading.Tasks.ValueTask static Microsoft.AspNetCore.Components.Forms.BrowserFileExtensions.RequestImageFileAsync(this Microsoft.AspNetCore.Components.Forms.IBrowserFile! browserFile, string! format, int maxWidth, int maxHeight) -> System.Threading.Tasks.ValueTask Microsoft.AspNetCore.Components.RenderTree.WebEventDescriptor.EventName.get -> string! diff --git a/src/Components/Web/src/Web/ErrorBoundary.cs b/src/Components/Web/src/Web/ErrorBoundary.cs index 8ab4c9e30c80..cf42bc4acee5 100644 --- a/src/Components/Web/src/Web/ErrorBoundary.cs +++ b/src/Components/Web/src/Web/ErrorBoundary.cs @@ -15,9 +15,9 @@ public class ErrorBoundary : ErrorBoundaryBase [Inject] private IErrorBoundaryLogger? ErrorBoundaryLogger { get; set; } /// - protected override ValueTask LogExceptionAsync(Exception exception) + protected override async Task OnErrorAsync(Exception exception) { - return ErrorBoundaryLogger!.LogErrorAsync(exception); + await ErrorBoundaryLogger!.LogErrorAsync(exception); } /// From 29e47600d6e5f99935a7f8be21b93cd06d0c1299 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Tue, 6 Apr 2021 15:10:25 +0100 Subject: [PATCH 32/55] Remove AutoRecover feature because it can be done in user code now we have Recover --- .../Components/src/ErrorBoundaryBase.cs | 18 +++++++----------- .../Components/src/PublicAPI.Unshipped.txt | 4 +--- .../BlazorServerApp/Pages/ErrorMaker.razor | 17 +++-------------- .../BlazorServerApp/Shared/MainLayout.razor | 12 +++++++++++- 4 files changed, 22 insertions(+), 29 deletions(-) diff --git a/src/Components/Components/src/ErrorBoundaryBase.cs b/src/Components/Components/src/ErrorBoundaryBase.cs index 87f7ca5dcf57..80d6799969c4 100644 --- a/src/Components/Components/src/ErrorBoundaryBase.cs +++ b/src/Components/Components/src/ErrorBoundaryBase.cs @@ -22,26 +22,22 @@ public abstract class ErrorBoundaryBase : ComponentBase, IErrorBoundary ///     [Parameter] public RenderFragment? ErrorContent { get; set; } - /// - /// Specifies whether to reset the error state each time this component instance is rendered - /// by its parent. This allows the child content to be recreated in an attempt to recover from the error. - /// - [Parameter] public bool AutoRecover { get; set; } - /// /// Gets the current exception, or null if there is no exception. /// protected Exception? CurrentException { get; private set; } - /// - public override Task SetParametersAsync(ParameterView parameters) + /// + /// 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. + /// + public void Recover() { - if (AutoRecover) + if (CurrentException is not null) { CurrentException = null; + StateHasChanged(); } - - return base.SetParametersAsync(parameters); } /// diff --git a/src/Components/Components/src/PublicAPI.Unshipped.txt b/src/Components/Components/src/PublicAPI.Unshipped.txt index 08da9abc29a3..2f2ae9872198 100644 --- a/src/Components/Components/src/PublicAPI.Unshipped.txt +++ b/src/Components/Components/src/PublicAPI.Unshipped.txt @@ -10,14 +10,13 @@ Microsoft.AspNetCore.Components.ComponentApplicationState.PersistState(string! k Microsoft.AspNetCore.Components.ComponentApplicationState.TryTakeAsJson(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.AutoRecover.get -> bool -Microsoft.AspNetCore.Components.ErrorBoundaryBase.AutoRecover.set -> void 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? Microsoft.AspNetCore.Components.ErrorBoundaryBase.ErrorContent.set -> void +Microsoft.AspNetCore.Components.ErrorBoundaryBase.Recover() -> void Microsoft.AspNetCore.Components.Lifetime.ComponentApplicationLifetime Microsoft.AspNetCore.Components.Lifetime.ComponentApplicationLifetime.ComponentApplicationLifetime(Microsoft.Extensions.Logging.ILogger! logger) -> void Microsoft.AspNetCore.Components.Lifetime.ComponentApplicationLifetime.PersistStateAsync(Microsoft.AspNetCore.Components.Lifetime.IComponentApplicationStateStore! store, Microsoft.AspNetCore.Components.RenderTree.Renderer! renderer) -> System.Threading.Tasks.Task! @@ -43,7 +42,6 @@ Microsoft.AspNetCore.Components.CascadingTypeParameterAttribute.CascadingTypePar 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.ErrorBoundaryBase.SetParametersAsync(Microsoft.AspNetCore.Components.ParameterView parameters) -> 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! 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! diff --git a/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor b/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor index 581fc33e726b..aca20d966e8f 100644 --- a/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor +++ b/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor @@ -35,26 +35,14 @@ throw new InvalidTimeZoneException($"Exception from {nameof(BuildRenderTree)}"); } -
    -

    AutoRecover

    -

    These errors will be caught by the inline error boundary. After recovery, the counter state will be reset, because it's a new instance.

    - - - - - -

    Here is some custom error content. If you re-render the container, I'll try to recover.

    -     -     - -

    Custom error boundary

    This shows how to create a common custom error UI by subclassing ErrorBoundary.

    - + +

    Custom error boundary that tries to ignore errors

    @@ -89,6 +77,7 @@ private bool throwWhileRendering; private bool throwInline; private bool throwInErrorContent; + private ErrorBoundaryBase? customErrorBoundary; void EventHandlerErrorSync() => throw new InvalidTimeZoneException("Synchronous error from event handler"); diff --git a/src/Components/Samples/BlazorServerApp/Shared/MainLayout.razor b/src/Components/Samples/BlazorServerApp/Shared/MainLayout.razor index fdf92c2799b2..0737841d2f64 100644 --- a/src/Components/Samples/BlazorServerApp/Shared/MainLayout.razor +++ b/src/Components/Samples/BlazorServerApp/Shared/MainLayout.razor @@ -10,7 +10,7 @@
    - + @Body
    @@ -21,3 +21,13 @@ Reload X + +@code { + ErrorBoundary? pageErrorBoundary; + + protected override void OnParametersSet() + { + // On each navigation, reset any error state so the user can move to a different page + pageErrorBoundary?.Recover(); + } +} From 08d4595a00f32fb2e4f14f402e7eea1ac612678a Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Tue, 6 Apr 2021 16:35:36 +0100 Subject: [PATCH 33/55] Unit tests for component error boundary dispatch logic --- .../Components/test/RendererTest.cs | 271 ++++++++++++++++++ 1 file changed, 271 insertions(+) diff --git a/src/Components/Components/test/RendererTest.cs b/src/Components/Components/test/RendererTest.cs index 3c12040f9a0d..c94ed58f4a8b 100644 --- a/src/Components/Components/test/RendererTest.cs +++ b/src/Components/Components/test/RendererTest.cs @@ -4170,6 +4170,191 @@ public async Task ThrowsIfComponentProducesInvalidRenderTree() Assert.StartsWith($"Render output is invalid for component of type '{typeof(TestComponent).FullName}'. A frame of type 'Element' was left unclosed.", ex.Message); } + [Fact] + public void RenderingExceptionsCanBeHandledByClosestErrorBoundary() + { + // Arrange + var renderer = new TestRenderer(); + var exception = new InvalidTimeZoneException("Error during render"); + var rootComponentId = renderer.AssignRootComponentId(new TestComponent(builder => + { + TestErrorBoundary.RenderNestedErrorBoundaries(builder, builder => + { + builder.OpenComponent(0); + builder.AddAttribute(1, nameof(ErrorThrowingComponent.ThrowDuringRender), exception); + builder.CloseComponent(); + }); + })); + + // Act + renderer.RenderRootComponent(rootComponentId); + + // Assert + var batch = renderer.Batches.Single(); + var errorThrowingComponentId = batch.GetComponentFrames().Single().ComponentId; + var componentFrames = batch.GetComponentFrames(); + Assert.Collection(componentFrames.Select(f => (TestErrorBoundary)f.Component), + component => Assert.Null(component.ReceivedException), + component => Assert.Same(exception, component.ReceivedException)); + + // The failed subtree is disposed + Assert.Equal(errorThrowingComponentId, batch.DisposedComponentIDs.Single()); + } + + [Fact] + public void SetParametersAsyncExceptionsCanBeHandledByClosestErrorBoundary_Sync() + { + // Arrange + var renderer = new TestRenderer(); + Exception exception = null; + var rootComponent = new TestComponent(builder => + { + TestErrorBoundary.RenderNestedErrorBoundaries(builder, builder => + { + builder.OpenComponent(0); + builder.AddAttribute(1, nameof(ErrorThrowingComponent.ThrowDuringParameterSettingSync), exception); + builder.CloseComponent(); + }); + }); + var rootComponentId = renderer.AssignRootComponentId(rootComponent); + renderer.RenderRootComponent(rootComponentId); + var errorBoundaries = renderer.Batches.Single().GetComponentFrames() + .Select(f => (TestErrorBoundary)f.Component); + var errorThrowingComponentId = renderer.Batches.Single() + .GetComponentFrames().Single().ComponentId; + + // Act + exception = new InvalidTimeZoneException("Error during SetParametersAsync"); + rootComponent.TriggerRender(); + + // Assert + Assert.Equal(2, renderer.Batches.Count); + Assert.Collection(errorBoundaries, + component => Assert.Null(component.ReceivedException), + component => Assert.Same(exception, component.ReceivedException)); + + // The failed subtree is disposed + Assert.Equal(errorThrowingComponentId, renderer.Batches[1].DisposedComponentIDs.Single()); + } + + [Fact] + public async Task SetParametersAsyncExceptionsCanBeHandledByClosestErrorBoundary_Async() + { + // Arrange + var renderer = new TestRenderer(); + Exception exception = null; + var rootComponent = new TestComponent(builder => + { + TestErrorBoundary.RenderNestedErrorBoundaries(builder, builder => + { + builder.OpenComponent(0); + builder.AddAttribute(1, nameof(ErrorThrowingComponent.ThrowDuringParameterSettingAsync), exception); + builder.CloseComponent(); + }); + }); + var rootComponentId = renderer.AssignRootComponentId(rootComponent); + renderer.RenderRootComponent(rootComponentId); + var errorBoundaries = renderer.Batches.Single().GetComponentFrames() + .Select(f => (TestErrorBoundary)f.Component).ToArray(); + var errorThrowingComponentId = renderer.Batches.Single() + .GetComponentFrames().Single().ComponentId; + + // Act/Assert 1: No synchronous errors + exception = new InvalidTimeZoneException("Error during SetParametersAsync"); + rootComponent.TriggerRender(); + Assert.Equal(2, renderer.Batches.Count); + + // Act/Assert 2: Asynchronous error + await errorBoundaries[1].ReceivedErrorTask; + Assert.Equal(3, renderer.Batches.Count); + Assert.Collection(errorBoundaries, + component => Assert.Null(component.ReceivedException), + component => Assert.Same(exception, component.ReceivedException)); + + // The failed subtree is disposed + Assert.Equal(errorThrowingComponentId, renderer.Batches[2].DisposedComponentIDs.Single()); + } + + [Fact] + public void EventDispatchExceptionsCanBeHandledByClosestErrorBoundary_Sync() + { + // Arrange + var renderer = new TestRenderer(); + var exception = new InvalidTimeZoneException("Error during event"); + var rootComponentId = renderer.AssignRootComponentId(new TestComponent(builder => + { + TestErrorBoundary.RenderNestedErrorBoundaries(builder, builder => + { + builder.OpenComponent(0); + builder.AddAttribute(1, nameof(ErrorThrowingComponent.ThrowDuringEventSync), exception); + builder.CloseComponent(); + }); + })); + renderer.RenderRootComponent(rootComponentId); + var errorBoundaries = renderer.Batches.Single().GetComponentFrames() + .Select(f => (TestErrorBoundary)f.Component); + var errorThrowingComponentId = renderer.Batches.Single() + .GetComponentFrames().Single().ComponentId; + var eventHandlerId = renderer.Batches.Single().ReferenceFrames + .Single(f => f.FrameType == RenderTreeFrameType.Attribute && f.AttributeName == "onmakeerror") + .AttributeEventHandlerId; + + // Act + var task = renderer.DispatchEventAsync(eventHandlerId, new EventArgs()); + + // Assert + Assert.True(task.IsCompletedSuccessfully); + Assert.Equal(2, renderer.Batches.Count); + Assert.Collection(errorBoundaries, + component => Assert.Null(component.ReceivedException), + component => Assert.Same(exception, component.ReceivedException)); + + // The failed subtree is disposed + Assert.Equal(errorThrowingComponentId, renderer.Batches[1].DisposedComponentIDs.Single()); + } + + [Fact] + public async Task EventDispatchExceptionsCanBeHandledByClosestErrorBoundary_Async() + { + // Arrange + var renderer = new TestRenderer(); + var exception = new InvalidTimeZoneException("Error during event"); + var rootComponentId = renderer.AssignRootComponentId(new TestComponent(builder => + { + TestErrorBoundary.RenderNestedErrorBoundaries(builder, builder => + { + builder.OpenComponent(0); + builder.AddAttribute(1, nameof(ErrorThrowingComponent.ThrowDuringEventAsync), exception); + builder.CloseComponent(); + }); + })); + renderer.RenderRootComponent(rootComponentId); + var errorBoundaries = renderer.Batches.Single().GetComponentFrames() + .Select(f => (TestErrorBoundary)f.Component); + var errorThrowingComponentId = renderer.Batches.Single() + .GetComponentFrames().Single().ComponentId; + var eventHandlerId = renderer.Batches.Single().ReferenceFrames + .Single(f => f.FrameType == RenderTreeFrameType.Attribute && f.AttributeName == "onmakeerror") + .AttributeEventHandlerId; + + // Act/Assert 1: No error synchronously + var task = renderer.DispatchEventAsync(eventHandlerId, new EventArgs()); + Assert.Single(renderer.Batches); + Assert.Collection(errorBoundaries, + component => Assert.Null(component.ReceivedException), + component => Assert.Null(component.ReceivedException)); + + // Act/Assert 2: Error is handled asynchronously + await task; + Assert.Equal(2, renderer.Batches.Count); + Assert.Collection(errorBoundaries, + component => Assert.Null(component.ReceivedException), + component => Assert.Same(exception, component.ReceivedException)); + + // The failed subtree is disposed + Assert.Equal(errorThrowingComponentId, renderer.Batches[1].DisposedComponentIDs.Single()); + } + private class TestComponentActivator : IComponentActivator where TResult : IComponent, new() { public List RequestedComponentTypes { get; } = new List(); @@ -4970,5 +5155,91 @@ public Task SetParametersAsync(ParameterView parameters) return new TaskCompletionSource().Task; } } + + private class TestErrorBoundary : AutoRenderComponent, IErrorBoundary + { + private TaskCompletionSource receivedErrorTaskCompletionSource = new(); + + public Exception ReceivedException { get; private set; } + public Task ReceivedErrorTask => receivedErrorTaskCompletionSource.Task; + + [Parameter] public RenderFragment ChildContent { get; set; } + + protected override void BuildRenderTree(RenderTreeBuilder builder) + => ChildContent(builder); + + public void HandleException(Exception error) + { + ReceivedException = error; + receivedErrorTaskCompletionSource.SetResult(); + } + + public static void RenderNestedErrorBoundaries(RenderTreeBuilder builder, RenderFragment innerContent) + { + // Create an error boundary + builder.OpenComponent(0); + builder.AddAttribute(1, nameof(TestErrorBoundary.ChildContent), (RenderFragment)(builder => + { + // ... containing another error boundary, containing the content + builder.OpenComponent(0); + builder.AddAttribute(1, nameof(TestErrorBoundary.ChildContent), innerContent); + builder.CloseComponent(); + })); + builder.CloseComponent(); + } + } + + private class ErrorThrowingComponent : AutoRenderComponent, IHandleEvent + { + [Parameter] public Exception ThrowDuringRender { get; set; } + [Parameter] public Exception ThrowDuringEventSync { get; set; } + [Parameter] public Exception ThrowDuringEventAsync { get; set; } + [Parameter] public Exception ThrowDuringParameterSettingSync { get; set; } + [Parameter] public Exception ThrowDuringParameterSettingAsync { get; set; } + + public override async Task SetParametersAsync(ParameterView parameters) + { + _ = base.SetParametersAsync(parameters); + + if (ThrowDuringParameterSettingSync is not null) + { + throw ThrowDuringParameterSettingSync; + } + + if (ThrowDuringParameterSettingAsync is not null) + { + await Task.Yield(); + throw ThrowDuringParameterSettingAsync; + } + } + + protected override void BuildRenderTree(RenderTreeBuilder builder) + { + if (ThrowDuringRender is not null) + { + throw ThrowDuringRender; + } + + builder.OpenElement(0, "someelem"); + builder.AddAttribute(1, "onmakeerror", EventCallback.Factory.Create(this, () => { })); + builder.AddContent(1, "Hello"); + builder.CloseElement(); + } + + public async Task HandleEventAsync(EventCallbackWorkItem item, object arg) + { + if (ThrowDuringEventSync is not null) + { + throw ThrowDuringEventSync; + } + + await Task.Yield(); + + if (ThrowDuringEventAsync is not null) + { + throw ThrowDuringEventAsync; + } + } + } } } From 7540d3a4493239de6540ab4cfe338fc61960d2c9 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Tue, 6 Apr 2021 16:42:31 +0100 Subject: [PATCH 34/55] Make async flow clearer --- src/Components/Components/test/RendererTest.cs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/Components/Components/test/RendererTest.cs b/src/Components/Components/test/RendererTest.cs index c94ed58f4a8b..66ee918ca2e4 100644 --- a/src/Components/Components/test/RendererTest.cs +++ b/src/Components/Components/test/RendererTest.cs @@ -5208,7 +5208,7 @@ public override async Task SetParametersAsync(ParameterView parameters) if (ThrowDuringParameterSettingAsync is not null) { - await Task.Yield(); + await Task.Delay(100); throw ThrowDuringParameterSettingAsync; } } @@ -5233,10 +5233,9 @@ public async Task HandleEventAsync(EventCallbackWorkItem item, object arg) throw ThrowDuringEventSync; } - await Task.Yield(); - if (ThrowDuringEventAsync is not null) { + await Task.Delay(100); throw ThrowDuringEventAsync; } } From 34e6519a71875c3544c34818a6e0ed65e3609f76 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Tue, 6 Apr 2021 16:44:31 +0100 Subject: [PATCH 35/55] Fix unrelated test code badness --- .../Components/test/RendererTest.cs | 23 ++++++++----------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/src/Components/Components/test/RendererTest.cs b/src/Components/Components/test/RendererTest.cs index 66ee918ca2e4..46d3a066f155 100644 --- a/src/Components/Components/test/RendererTest.cs +++ b/src/Components/Components/test/RendererTest.cs @@ -4586,10 +4586,9 @@ protected override void BuildRenderTree(RenderTreeBuilder builder) builder.OpenComponent(1); if (ChildParameters != null) { - var sequence = 2; foreach (var kvp in ChildParameters) { - builder.AddAttribute(sequence++, kvp.Key, kvp.Value); + builder.AddAttribute(2, kvp.Key, kvp.Value); } } builder.CloseComponent(); @@ -4805,9 +4804,8 @@ public async Task SetParametersAsync(ParameterView parameters) // Cheap closure void CreateFragment(RenderTreeBuilder builder) { - var s = 0; - builder.OpenElement(s++, "p"); - builder.AddContent(s++, n); + builder.OpenElement(0, "p"); + builder.AddContent(1, n); builder.CloseElement(); } } @@ -4886,16 +4884,15 @@ private Func CreateRenderFactory(int[] chi return component => builder => { - var s = 0; - builder.OpenElement(s++, "div"); - builder.AddContent(s++, $"Id: {component.TestId} BuildRenderTree, {Guid.NewGuid()}"); + builder.OpenElement(0, "div"); + builder.AddContent(1, $"Id: {component.TestId} BuildRenderTree, {Guid.NewGuid()}"); foreach (var child in childrenToRender) { - builder.OpenComponent(s++); - builder.AddAttribute(s++, eventActionsName, component.EventActions); - builder.AddAttribute(s++, whatToRenderName, component.WhatToRender); - builder.AddAttribute(s++, testIdName, child); - builder.AddAttribute(s++, logName, component.Log); + builder.OpenComponent(2); + builder.AddAttribute(3, eventActionsName, component.EventActions); + builder.AddAttribute(4, whatToRenderName, component.WhatToRender); + builder.AddAttribute(5, testIdName, child); + builder.AddAttribute(6, logName, component.Log); builder.CloseComponent(); } From 06da1ef29c9ad8479b8a619a51e3478699b48528 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Tue, 6 Apr 2021 17:08:41 +0100 Subject: [PATCH 36/55] Add E2E test UI --- .../Components/src/ErrorBoundaryBase.cs | 5 ++++- .../Components/src/PublicAPI.Unshipped.txt | 2 +- .../BlazorServerApp/Shared/ErrorIgnorer.cs | 15 --------------- .../BlazorServerApp/Shared/MainLayout.razor | 14 +------------- .../ErrorBoundaryTest}/CustomErrorBoundary.razor | 6 +----- .../ErrorBoundaryTest/ErrorBoundaryCases.razor} | 6 ++---- .../ErrorBoundaryContainer.razor | 5 +++++ .../ErrorBoundaryTest}/ErrorCausingChild.razor | 0 .../ErrorBoundaryTest}/ErrorCausingCounter.razor | 0 .../ErrorBoundaryTest/ErrorIgnorer.razor | 6 ++++++ .../test/testassets/BasicTestApp/Index.razor | 1 + .../testassets/BasicTestApp/wwwroot/style.css | 10 ++++++++++ 12 files changed, 31 insertions(+), 39 deletions(-) delete mode 100644 src/Components/Samples/BlazorServerApp/Shared/ErrorIgnorer.cs rename src/Components/{Samples/BlazorServerApp/Shared => test/testassets/BasicTestApp/ErrorBoundaryTest}/CustomErrorBoundary.razor (67%) rename src/Components/{Samples/BlazorServerApp/Pages/ErrorMaker.razor => test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryCases.razor} (97%) create mode 100644 src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryContainer.razor rename src/Components/{Samples/BlazorServerApp/Shared => test/testassets/BasicTestApp/ErrorBoundaryTest}/ErrorCausingChild.razor (100%) rename src/Components/{Samples/BlazorServerApp/Shared => test/testassets/BasicTestApp/ErrorBoundaryTest}/ErrorCausingCounter.razor (100%) create mode 100644 src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorIgnorer.razor diff --git a/src/Components/Components/src/ErrorBoundaryBase.cs b/src/Components/Components/src/ErrorBoundaryBase.cs index 80d6799969c4..a945cebfe301 100644 --- a/src/Components/Components/src/ErrorBoundaryBase.cs +++ b/src/Components/Components/src/ErrorBoundaryBase.cs @@ -44,7 +44,10 @@ public void Recover() /// Invoked by the base class when an error is being handled. /// /// The being handled. - protected abstract Task OnErrorAsync(Exception exception); + protected virtual Task OnErrorAsync(Exception exception) + { + return Task.CompletedTask; + } void IErrorBoundary.HandleException(Exception exception) { diff --git a/src/Components/Components/src/PublicAPI.Unshipped.txt b/src/Components/Components/src/PublicAPI.Unshipped.txt index 2f2ae9872198..f049a3420469 100644 --- a/src/Components/Components/src/PublicAPI.Unshipped.txt +++ b/src/Components/Components/src/PublicAPI.Unshipped.txt @@ -41,8 +41,8 @@ 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! parameters) -> Microsoft.AspNetCore.Components.ParameterView +virtual Microsoft.AspNetCore.Components.ErrorBoundaryBase.OnErrorAsync(System.Exception! exception) -> System.Threading.Tasks.Task! virtual Microsoft.AspNetCore.Components.RenderTree.Renderer.DispatchEventAsync(ulong eventHandlerId, Microsoft.AspNetCore.Components.RenderTree.EventFieldInfo? fieldInfo, System.EventArgs! eventArgs) -> System.Threading.Tasks.Task! readonly Microsoft.AspNetCore.Components.RenderTree.RenderTreeEdit.RemovedAttributeName -> string? diff --git a/src/Components/Samples/BlazorServerApp/Shared/ErrorIgnorer.cs b/src/Components/Samples/BlazorServerApp/Shared/ErrorIgnorer.cs deleted file mode 100644 index c771c762af90..000000000000 --- a/src/Components/Samples/BlazorServerApp/Shared/ErrorIgnorer.cs +++ /dev/null @@ -1,15 +0,0 @@ -using Microsoft.AspNetCore.Components.Rendering; -using Microsoft.AspNetCore.Components.Web; - -namespace BlazorServerApp.Shared -{ - // A badly-behaved error boundary that tries to keep using its same ChildContent even after an error - // This is to check we still don't retain the descendant component instances - public class ErrorIgnorer : ErrorBoundary - { - protected override void BuildRenderTree(RenderTreeBuilder builder) - { - builder.AddContent(0, ChildContent); - } - } -} diff --git a/src/Components/Samples/BlazorServerApp/Shared/MainLayout.razor b/src/Components/Samples/BlazorServerApp/Shared/MainLayout.razor index 0737841d2f64..d353bfa08fa7 100644 --- a/src/Components/Samples/BlazorServerApp/Shared/MainLayout.razor +++ b/src/Components/Samples/BlazorServerApp/Shared/MainLayout.razor @@ -10,9 +10,7 @@
    - - @Body - + @Body
    @@ -21,13 +19,3 @@ Reload X - -@code { - ErrorBoundary? pageErrorBoundary; - - protected override void OnParametersSet() - { - // On each navigation, reset any error state so the user can move to a different page - pageErrorBoundary?.Recover(); - } -} diff --git a/src/Components/Samples/BlazorServerApp/Shared/CustomErrorBoundary.razor b/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/CustomErrorBoundary.razor similarity index 67% rename from src/Components/Samples/BlazorServerApp/Shared/CustomErrorBoundary.razor rename to src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/CustomErrorBoundary.razor index 5e5feb5476ac..4ddfa234d706 100644 --- a/src/Components/Samples/BlazorServerApp/Shared/CustomErrorBoundary.razor +++ b/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/CustomErrorBoundary.razor @@ -10,10 +10,6 @@ else if (ErrorContent is not null) else {
    - @{ #if DEBUG } - @CurrentException.ToString() - @{ #else } -

    Sorry, there was a problem.

    - @{ #endif } + @CurrentException.ToString()
    } diff --git a/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor b/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryCases.razor similarity index 97% rename from src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor rename to src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryCases.razor index aca20d966e8f..90a32866388b 100644 --- a/src/Components/Samples/BlazorServerApp/Pages/ErrorMaker.razor +++ b/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryCases.razor @@ -1,6 +1,4 @@ -@page "/errormaker" - -

    Event handlers

    +

    Event handlers

    These errors will be caught by the closest error boundary ancestor.

    @@ -77,7 +75,7 @@ private bool throwWhileRendering; private bool throwInline; private bool throwInErrorContent; - private ErrorBoundaryBase? customErrorBoundary; + private ErrorBoundaryBase customErrorBoundary; void EventHandlerErrorSync() => throw new InvalidTimeZoneException("Synchronous error from event handler"); diff --git a/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryContainer.razor b/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryContainer.razor new file mode 100644 index 000000000000..7ffcdf5af647 --- /dev/null +++ b/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryContainer.razor @@ -0,0 +1,5 @@ +
    + + + +
    diff --git a/src/Components/Samples/BlazorServerApp/Shared/ErrorCausingChild.razor b/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorCausingChild.razor similarity index 100% rename from src/Components/Samples/BlazorServerApp/Shared/ErrorCausingChild.razor rename to src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorCausingChild.razor diff --git a/src/Components/Samples/BlazorServerApp/Shared/ErrorCausingCounter.razor b/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorCausingCounter.razor similarity index 100% rename from src/Components/Samples/BlazorServerApp/Shared/ErrorCausingCounter.razor rename to src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorCausingCounter.razor diff --git a/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorIgnorer.razor b/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorIgnorer.razor new file mode 100644 index 000000000000..3a43d7a985a2 --- /dev/null +++ b/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorIgnorer.razor @@ -0,0 +1,6 @@ +@inherits ErrorBoundaryBase +@* + A badly-behaved error boundary that tries to keep using its same ChildContent even after an error. + This is to check we still don't retain the descendant component instances. +*@ +@ChildContent diff --git a/src/Components/test/testassets/BasicTestApp/Index.razor b/src/Components/test/testassets/BasicTestApp/Index.razor index 34a05f91dac4..ab3e6cdae04b 100644 --- a/src/Components/test/testassets/BasicTestApp/Index.razor +++ b/src/Components/test/testassets/BasicTestApp/Index.razor @@ -25,6 +25,7 @@ + diff --git a/src/Components/test/testassets/BasicTestApp/wwwroot/style.css b/src/Components/test/testassets/BasicTestApp/wwwroot/style.css index ea9900430b1c..232be009974f 100644 --- a/src/Components/test/testassets/BasicTestApp/wwwroot/style.css +++ b/src/Components/test/testassets/BasicTestApp/wwwroot/style.css @@ -29,3 +29,13 @@ .validation-message { color: red; } + +.blazor-error-boundary { + background: url(data:image/svg+xml;base64,PHN2ZyB3aWR0aD0iNTYiIGhlaWdodD0iNDkiIHhtbG5zPSJodHRwOi8vd3d3LnczLm9yZy8yMDAwL3N2ZyIgeG1sbnM6eGxpbms9Imh0dHA6Ly93d3cudzMub3JnLzE5OTkveGxpbmsiIG92ZXJmbG93PSJoaWRkZW4iPjxkZWZzPjxjbGlwUGF0aCBpZD0iY2xpcDAiPjxyZWN0IHg9IjIzNSIgeT0iNTEiIHdpZHRoPSI1NiIgaGVpZ2h0PSI0OSIvPjwvY2xpcFBhdGg+PC9kZWZzPjxnIGNsaXAtcGF0aD0idXJsKCNjbGlwMCkiIHRyYW5zZm9ybT0idHJhbnNsYXRlKC0yMzUgLTUxKSI+PHBhdGggZD0iTTI2My41MDYgNTFDMjY0LjcxNyA1MSAyNjUuODEzIDUxLjQ4MzcgMjY2LjYwNiA1Mi4yNjU4TDI2Ny4wNTIgNTIuNzk4NyAyNjcuNTM5IDUzLjYyODMgMjkwLjE4NSA5Mi4xODMxIDI5MC41NDUgOTIuNzk1IDI5MC42NTYgOTIuOTk2QzI5MC44NzcgOTMuNTEzIDI5MSA5NC4wODE1IDI5MSA5NC42NzgyIDI5MSA5Ny4wNjUxIDI4OS4wMzggOTkgMjg2LjYxNyA5OUwyNDAuMzgzIDk5QzIzNy45NjMgOTkgMjM2IDk3LjA2NTEgMjM2IDk0LjY3ODIgMjM2IDk0LjM3OTkgMjM2LjAzMSA5NC4wODg2IDIzNi4wODkgOTMuODA3MkwyMzYuMzM4IDkzLjAxNjIgMjM2Ljg1OCA5Mi4xMzE0IDI1OS40NzMgNTMuNjI5NCAyNTkuOTYxIDUyLjc5ODUgMjYwLjQwNyA1Mi4yNjU4QzI2MS4yIDUxLjQ4MzcgMjYyLjI5NiA1MSAyNjMuNTA2IDUxWk0yNjMuNTg2IDY2LjAxODNDMjYwLjczNyA2Ni4wMTgzIDI1OS4zMTMgNjcuMTI0NSAyNTkuMzEzIDY5LjMzNyAyNTkuMzEzIDY5LjYxMDIgMjU5LjMzMiA2OS44NjA4IDI1OS4zNzEgNzAuMDg4N0wyNjEuNzk1IDg0LjAxNjEgMjY1LjM4IDg0LjAxNjEgMjY3LjgyMSA2OS43NDc1QzI2Ny44NiA2OS43MzA5IDI2Ny44NzkgNjkuNTg3NyAyNjcuODc5IDY5LjMxNzkgMjY3Ljg3OSA2Ny4xMTgyIDI2Ni40NDggNjYuMDE4MyAyNjMuNTg2IDY2LjAxODNaTTI2My41NzYgODYuMDU0N0MyNjEuMDQ5IDg2LjA1NDcgMjU5Ljc4NiA4Ny4zMDA1IDI1OS43ODYgODkuNzkyMSAyNTkuNzg2IDkyLjI4MzcgMjYxLjA0OSA5My41Mjk1IDI2My41NzYgOTMuNTI5NSAyNjYuMTE2IDkzLjUyOTUgMjY3LjM4NyA5Mi4yODM3IDI2Ny4zODcgODkuNzkyMSAyNjcuMzg3IDg3LjMwMDUgMjY2LjExNiA4Ni4wNTQ3IDI2My41NzYgODYuMDU0N1oiIGZpbGw9IiNGRkU1MDAiIGZpbGwtcnVsZT0iZXZlbm9kZCIvPjwvZz48L3N2Zz4=) no-repeat 1rem/1.8rem, #b32121; + padding: 1rem 1rem 1rem 3.7rem; + color: white; +} + + .blazor-error-boundary::after { + content: "An error has occurred." + } From 259f5d5637882edf87c70b1c29b2415c903310ac Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Tue, 6 Apr 2021 17:30:45 +0100 Subject: [PATCH 37/55] Begin scripting E2E cases --- .../ServerExecutionTests/TestSubclasses.cs | 8 +++ .../test/E2ETest/Tests/ErrorBoundaryTest.cs | 57 +++++++++++++++++++ .../ErrorBoundaryCases.razor | 18 +++--- 3 files changed, 74 insertions(+), 9 deletions(-) create mode 100644 src/Components/test/E2ETest/Tests/ErrorBoundaryTest.cs diff --git a/src/Components/test/E2ETest/ServerExecutionTests/TestSubclasses.cs b/src/Components/test/E2ETest/ServerExecutionTests/TestSubclasses.cs index bcc5e55c0fa7..72ac00a77a53 100644 --- a/src/Components/test/E2ETest/ServerExecutionTests/TestSubclasses.cs +++ b/src/Components/test/E2ETest/ServerExecutionTests/TestSubclasses.cs @@ -107,4 +107,12 @@ public ServerEventCustomArgsTest(BrowserFixture browserFixture, ToggleExecutionM { } } + + public class ServerErrorBoundaryBase : ErrorBoundaryTest + { + public ServerErrorBoundaryBase(BrowserFixture browserFixture, ToggleExecutionModeServerFixture serverFixture, ITestOutputHelper output) + : base(browserFixture, serverFixture.WithServerExecution(), output) + { + } + } } diff --git a/src/Components/test/E2ETest/Tests/ErrorBoundaryTest.cs b/src/Components/test/E2ETest/Tests/ErrorBoundaryTest.cs new file mode 100644 index 000000000000..7113f107333b --- /dev/null +++ b/src/Components/test/E2ETest/Tests/ErrorBoundaryTest.cs @@ -0,0 +1,57 @@ +// 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 BasicTestApp; +using BasicTestApp.ErrorBoundaryTest; +using Microsoft.AspNetCore.Components.E2ETest.Infrastructure; +using Microsoft.AspNetCore.Components.E2ETest.Infrastructure.ServerFixtures; +using Microsoft.AspNetCore.E2ETesting; +using OpenQA.Selenium; +using Xunit; +using Xunit.Abstractions; + +namespace Microsoft.AspNetCore.Components.E2ETest.Tests +{ + public class ErrorBoundaryTest : ServerTestBase> + { + public ErrorBoundaryTest(BrowserFixture browserFixture, ToggleExecutionModeServerFixture serverFixture, ITestOutputHelper output) + : base(browserFixture, serverFixture, output) + { + } + + protected override void InitializeAsyncCore() + { + // Many of these tests trigger fatal exceptions, so we always have to reload + Navigate(ServerPathBase, noReload: false); + Browser.MountTestComponent(); + } + + [Theory] + [InlineData("event-sync")] + [InlineData("event-async")] + [InlineData("parametersset-sync")] + [InlineData("parametersset-async")] + [InlineData("parametersset-cascade-sync")] + [InlineData("parametersset-cascade-async")] + [InlineData("afterrender-sync")] + [InlineData("afterrender-async")] + [InlineData("while-rendering")] + public void CanHandleExceptions(string triggerId) + { + var container = Browser.Exists(By.Id("error-boundary-container")); + container.FindElement(By.Id(triggerId)).Click(); + + // The whole UI within the container is replaced by the default error UI + Browser.Collection(() => container.FindElements(By.CssSelector("*")), + elem => + { + Assert.Equal("blazor-error-boundary", elem.GetAttribute("class")); + Assert.Empty(elem.FindElements(By.CssSelector("*"))); + }); + + // It wasn't a fatal error + var globalErrorUi = Browser.Exists(By.Id("blazor-error-ui")); + Assert.Equal("none", globalErrorUi.GetCssValue("display")); + } + } +} diff --git a/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryCases.razor b/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryCases.razor index 90a32866388b..761484f69be4 100644 --- a/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryCases.razor +++ b/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryCases.razor @@ -1,17 +1,17 @@ 

    Event handlers

    These errors will be caught by the closest error boundary ancestor.

    - - + +

    Lifecycle methods

    These errors will be caught by the closest error boundary ancestor.

    -
    -
    -
    -
    -
    -
    +
    +
    +
    +
    +
    +
    @@ -27,7 +27,7 @@

    Rendering

    These errors will be caught by the closest error boundary ancestor.

    - + @if (throwWhileRendering) { throw new InvalidTimeZoneException($"Exception from {nameof(BuildRenderTree)}"); From 63d950b205826a92c0428f552209578a64f3f65d Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Wed, 7 Apr 2021 10:31:35 +0100 Subject: [PATCH 38/55] Add some E2E tests for more difficult cases --- .../CustomErrorBoundary.razor | 25 ++++++++- .../DelayedErrorCausingChild.razor | 14 +++++ .../ErrorBoundaryCases.razor | 51 ++++++++++++++++++- .../ErrorBoundaryContainer.razor | 10 +++- 4 files changed, 96 insertions(+), 4 deletions(-) create mode 100644 src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/DelayedErrorCausingChild.razor diff --git a/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/CustomErrorBoundary.razor b/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/CustomErrorBoundary.razor index 4ddfa234d706..5e9252db76d5 100644 --- a/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/CustomErrorBoundary.razor +++ b/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/CustomErrorBoundary.razor @@ -9,7 +9,28 @@ else if (ErrorContent is not null) } else { -
    - @CurrentException.ToString() +
    + @foreach (var exception in receivedExceptions) + { +
    + @exception.Message +
    + }
    } + +@code { + List receivedExceptions = new(); + + protected override Task OnErrorAsync(Exception exception) + { + receivedExceptions.Add(exception); + return base.OnErrorAsync(exception); + } + + public new void Recover() + { + receivedExceptions.Clear(); + base.Recover(); + } +} diff --git a/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/DelayedErrorCausingChild.razor b/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/DelayedErrorCausingChild.razor new file mode 100644 index 000000000000..8af2915db75d --- /dev/null +++ b/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/DelayedErrorCausingChild.razor @@ -0,0 +1,14 @@ +

    This is a child component

    + +@code { + [Parameter] public bool ThrowOnParametersSetAsync { get; set; } + + protected override async Task OnParametersSetAsync() + { + if (ThrowOnParametersSetAsync) + { + await Task.Delay(500); + throw new InvalidTimeZoneException("Delayed asynchronous exception in OnParametersSetAsync"); + } + } +} diff --git a/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryCases.razor b/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryCases.razor index 761484f69be4..0896a6840b0a 100644 --- a/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryCases.razor +++ b/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryCases.razor @@ -65,6 +65,31 @@ +
    +

    Errors after disposal

    +

    Long-running tasks could fail after the component has been removed from the tree. We still want these failures to be captured by the error boundary they were inside when the task began, even if that error boundary itself has also since been disposed. Otherwise, error handling would behave differently based on whether the user has navigated away while processing was in flight, which would be very unexpected and hard to handle.

    +@if (!disposalTestRemoveErrorBoundary) +{ + + @if (!disposalTestRemoveComponent) + { + + } + +} + + + +
    +

    Multiple child errors

    +

    If several child components trigger asynchronous errors, the error boundary will receive error notifications even when it's already in an errored state. This needs to behave sensibly.

    + + + + + + + @code { private bool throwInOnParametersSet; private bool throwInOnParametersSetAsync; @@ -75,7 +100,11 @@ private bool throwWhileRendering; private bool throwInline; private bool throwInErrorContent; - private ErrorBoundaryBase customErrorBoundary; + private CustomErrorBoundary customErrorBoundary; + private bool disposalTestRemoveComponent; + private bool disposalTestRemoveErrorBoundary; + private bool disposalTestBeginDelayedError; + private bool multipleChildrenBeginDelayedError; void EventHandlerErrorSync() => throw new InvalidTimeZoneException("Synchronous error from event handler"); @@ -85,4 +114,24 @@ await Task.Yield(); throw new InvalidTimeZoneException("Asynchronous error from event handler"); } + + async Task ThrowAfterDisposalOfComponent() + { + // Begin an async process that will result in an exception from a child component + disposalTestBeginDelayedError = true; + await Task.Yield(); + + // Before it completes, dispose that child component + disposalTestRemoveComponent = true; + } + + async Task ThrowAfterDisposalOfErrorBoundary() + { + // Begin an async process that will result in an exception from a child component + disposalTestBeginDelayedError = true; + await Task.Yield(); + + // Before it completes, dispose its enclosing error boundary + disposalTestRemoveErrorBoundary = true; + } } diff --git a/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryContainer.razor b/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryContainer.razor index 7ffcdf5af647..2f6d6f1daa79 100644 --- a/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryContainer.razor +++ b/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryContainer.razor @@ -1,5 +1,13 @@ 
    - +
    + +
    + +Recover + +@code { + ErrorBoundary errorBoundary; +} From ae4cdc4bab2e9acb0c2e12f4007bd1d2dedd48f6 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Wed, 7 Apr 2021 10:40:50 +0100 Subject: [PATCH 39/55] Cope with receiving errors while already in an error state --- .../Components/src/ErrorBoundaryBase.cs | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/Components/Components/src/ErrorBoundaryBase.cs b/src/Components/Components/src/ErrorBoundaryBase.cs index a945cebfe301..a77587efb665 100644 --- a/src/Components/Components/src/ErrorBoundaryBase.cs +++ b/src/Components/Components/src/ErrorBoundaryBase.cs @@ -51,21 +51,23 @@ protected virtual Task OnErrorAsync(Exception exception) void IErrorBoundary.HandleException(Exception exception) { - if (CurrentException is not null) - { - // If there's an error while we're already displaying error content, then it's the - // error content that's failing. Avoid the risk of an infinite error rendering loop. - ExceptionDispatchInfo.Capture(exception).Throw(); - } - var onErrorTask = OnErrorAsync(exception); if (!onErrorTask.IsCompletedSuccessfully) { _ = HandleOnErrorExceptions(onErrorTask); } - CurrentException = exception; - StateHasChanged(); + // If there's an error while we're already displaying error content, then it may be either of: + // (a) the error content that is failing + // (b) some earlier child content failing an asynchronous task that began before we entered an error state + // In case it's (a), we don't want to risk triggering an infinite error rendering loop by re-rendering. + // This isn't harmful in case (b) because we're already in an error state, so don't need to update the UI further. + // So, only re-render if we're not currently in an error state. + if (CurrentException == null) + { + CurrentException = exception; + StateHasChanged(); + } } private async Task HandleOnErrorExceptions(Task onExceptionTask) From 76f41fe1f6d2a12a4868a03458880bb92400b6e7 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Wed, 7 Apr 2021 10:50:20 +0100 Subject: [PATCH 40/55] Add failing unit test for errors after disposal --- .../Components/test/RendererTest.cs | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/src/Components/Components/test/RendererTest.cs b/src/Components/Components/test/RendererTest.cs index 46d3a066f155..d0f3278fa466 100644 --- a/src/Components/Components/test/RendererTest.cs +++ b/src/Components/Components/test/RendererTest.cs @@ -4355,6 +4355,56 @@ public async Task EventDispatchExceptionsCanBeHandledByClosestErrorBoundary_Asyn Assert.Equal(errorThrowingComponentId, renderer.Batches[1].DisposedComponentIDs.Single()); } + [Fact] + public async Task EventDispatchExceptionsCanBeHandledByClosestErrorBoundary_AfterDisposal() + { + // Arrange + var renderer = new TestRenderer(); + var disposeChildren = false; + var exception = new InvalidTimeZoneException("Error during event"); + var rootComponent = new TestComponent(builder => + { + if (!disposeChildren) + { + TestErrorBoundary.RenderNestedErrorBoundaries(builder, builder => + { + builder.OpenComponent(0); + builder.AddAttribute(1, nameof(ErrorThrowingComponent.ThrowDuringEventAsync), exception); + builder.CloseComponent(); + }); + } + }); + var rootComponentId = renderer.AssignRootComponentId(rootComponent); + renderer.RenderRootComponent(rootComponentId); + var errorBoundaries = renderer.Batches.Single().GetComponentFrames() + .Select(f => (TestErrorBoundary)f.Component); + var errorThrowingComponentId = renderer.Batches.Single() + .GetComponentFrames().Single().ComponentId; + var eventHandlerId = renderer.Batches.Single().ReferenceFrames + .Single(f => f.FrameType == RenderTreeFrameType.Attribute && f.AttributeName == "onmakeerror") + .AttributeEventHandlerId; + + // Act/Assert 1: No error synchronously + var task = renderer.DispatchEventAsync(eventHandlerId, new EventArgs()); + Assert.Single(renderer.Batches); + Assert.Collection(errorBoundaries, + component => Assert.Null(component.ReceivedException), + component => Assert.Null(component.ReceivedException)); + + // Act 2: Before the async error occurs, dispose the hierarchy containing the error boundary and erroring component + disposeChildren = true; + rootComponent.TriggerRender(); + Assert.Equal(2, renderer.Batches.Count); + Assert.Contains(errorThrowingComponentId, renderer.Batches.Last().DisposedComponentIDs); + + // Assert 2: Error is still handled + await task; + Assert.Equal(3, renderer.Batches.Count); + Assert.Collection(errorBoundaries, + component => Assert.Null(component.ReceivedException), + component => Assert.Same(exception, component.ReceivedException)); + } + private class TestComponentActivator : IComponentActivator where TResult : IComponent, new() { public List RequestedComponentTypes { get; } = new List(); From f5be9f1fa203a0d32e3d8fc0ed1e9d72ff6baecd Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Wed, 7 Apr 2021 11:05:42 +0100 Subject: [PATCH 41/55] Have renderer cope with errors after disposal --- .../Components/src/RenderTree/Renderer.cs | 66 +++++++++---------- .../src/Rendering/ComponentState.cs | 2 +- .../Components/test/RendererTest.cs | 2 +- 3 files changed, 34 insertions(+), 36 deletions(-) diff --git a/src/Components/Components/src/RenderTree/Renderer.cs b/src/Components/Components/src/RenderTree/Renderer.cs index 1c1854590d30..25b10f971575 100644 --- a/src/Components/Components/src/RenderTree/Renderer.cs +++ b/src/Components/Components/src/RenderTree/Renderer.cs @@ -29,6 +29,7 @@ public abstract partial class Renderer : IDisposable, IAsyncDisposable { private readonly IServiceProvider _serviceProvider; private readonly Dictionary _componentStateById = new Dictionary(); + private readonly Dictionary _componentStateByComponent = new Dictionary(); private readonly RenderBatchBuilder _batchBuilder = new RenderBatchBuilder(); private readonly Dictionary _eventBindings = new Dictionary(); private readonly Dictionary _eventHandlerIdReplacements = new Dictionary(); @@ -291,6 +292,7 @@ private ComponentState AttachAndInitComponent(IComponent component, int parentCo var componentState = new ComponentState(this, componentId, component, parentComponentState); Log.InitializingComponent(_logger, componentState, parentComponentState); _componentStateById.Add(componentId, componentState); + _componentStateByComponent.Add(component, componentState); component.Attach(new RenderHandle(this, componentId)); return componentState; } @@ -317,9 +319,18 @@ public virtual Task DispatchEventAsync(ulong eventHandlerId, EventFieldInfo? fie Dispatcher.AssertAccess(); var callback = GetRequiredEventCallback(eventHandlerId); - IComponent? componentReceiver = callback.Receiver as IComponent; // Null if the receiver is null or isn't a component Log.HandlingEvent(_logger, eventHandlerId, eventArgs); + // Try to match it up with a receiver so that, if the event handler later throws, we can route the error to the + // correct error boundary (even if the receiving component got disposed in the meantime). + ComponentState? receiverComponentState = null; + if (callback.Receiver is IComponent receiverComponent) // The receiver might be null or not an IComponent + { + // Even if the receiver is an IComponent, it might not be one of ours, or might be disposed already + // We can only route errors to error boundaries if the receiver is known and not yet disposed at this stage + _componentStateByComponent.TryGetValue(receiverComponent, out receiverComponentState); + } + if (fieldInfo != null) { var latestEquivalentEventHandlerId = FindLatestEventHandlerIdInChain(eventHandlerId); @@ -337,7 +348,7 @@ public virtual Task DispatchEventAsync(ulong eventHandlerId, EventFieldInfo? fie } catch (Exception e) { - if (!TrySendExceptionToErrorBoundary(componentReceiver, e)) + if (!TrySendExceptionToErrorBoundary(receiverComponentState, e)) { // It's unhandled, so treat as fatal HandleException(e); @@ -356,11 +367,11 @@ public virtual Task DispatchEventAsync(ulong eventHandlerId, EventFieldInfo? fie // Task completed synchronously or is still running. We already processed all of the rendering // work that was queued so let our error handler deal with it. - var result = GetErrorHandledTask(task, componentReceiver); + var result = GetErrorHandledTask(task, receiverComponentState); return result; } - private bool TrySendExceptionToErrorBoundary(IComponent? errorSource, Exception error) + private bool TrySendExceptionToErrorBoundary(ComponentState? errorSource, Exception error) { if (errorSource is null) { @@ -372,17 +383,17 @@ private bool TrySendExceptionToErrorBoundary(IComponent? errorSource, Exception Dispatcher.AssertAccess(); // Find the closest error boundary, if any - var componentState = FindComponentState(errorSource); - while (componentState is not null) + var candidate = errorSource; + while (candidate is not null) { - if (componentState.Component is IErrorBoundary errorBoundary) + if (candidate.Component is IErrorBoundary errorBoundary) { // Even though .Web's ErrorBoundary by default removes its ChildContent from the tree when // switching into an error state, the Renderer doesn't rely on that. To guarantee that // the failed subtree is disposed, forcibly remove it here. If the failed components did // continue to run, it wouldn't harm framework state, but it would be a whole new kind of // edge case to support forever. - AddToRenderQueue(componentState.ComponentId, builder => { }); + AddToRenderQueue(candidate.ComponentId, builder => { }); try { @@ -399,25 +410,10 @@ private bool TrySendExceptionToErrorBoundary(IComponent? errorSource, Exception return true; } - componentState = componentState.ParentComponentState; + candidate = candidate.ParentComponentState; } return false; - - ComponentState? FindComponentState(IComponent errorSource) - { - // Slow linear search is fine because this is only during exception handling. - // If it becomes problematic, we can maintain a lookup from component instance to ID. - foreach (var value in _componentStateById.Values) - { - if (value.Component == errorSource) - { - return value; - } - } - - return null; - } } /// @@ -456,7 +452,7 @@ internal void InstantiateChildComponentOnFrame(ref RenderTreeFrame frame, int pa frame.ComponentIdField = newComponentState.ComponentId; } - internal void AddToPendingTasks(Task task, IComponent? owningComponent) + internal void AddToPendingTasks(Task task, ComponentState? owningComponentState) { switch (task == null ? TaskStatus.RanToCompletion : task.Status) { @@ -473,7 +469,7 @@ internal void AddToPendingTasks(Task task, IComponent? owningComponent) // the synchronous exceptions will get captured and converted into a faulted // task. var baseException = task.Exception.GetBaseException(); - if (!TrySendExceptionToErrorBoundary(owningComponent, baseException)) + if (!TrySendExceptionToErrorBoundary(owningComponentState, baseException)) { HandleException(baseException); } @@ -481,7 +477,7 @@ internal void AddToPendingTasks(Task task, IComponent? owningComponent) default: // It's important to evaluate the following even if we're not going to use // handledErrorTask below, because it has the side-effect of calling HandleException. - var handledErrorTask = GetErrorHandledTask(task, owningComponent); + var handledErrorTask = GetErrorHandledTask(task, owningComponentState); // The pendingTasks collection is only used during prerendering to track quiescence, // so will be null at other times. @@ -762,7 +758,7 @@ private void NotifyRenderCompleted(ComponentState state, ref List batch) } else if (task.Status == TaskStatus.Faulted) { - if (!TrySendExceptionToErrorBoundary(state.Component, task.Exception)) + if (!TrySendExceptionToErrorBoundary(state, task.Exception)) { HandleException(task.Exception); } @@ -773,7 +769,7 @@ private void NotifyRenderCompleted(ComponentState state, ref List batch) // The Task is incomplete. // Queue up the task and we can inspect it later. batch = batch ?? new List(); - batch.Add(GetErrorHandledTask(task, state.Component)); + batch.Add(GetErrorHandledTask(task, state)); } private void RenderInExistingBatch(RenderQueueEntry renderQueueEntry) @@ -781,7 +777,7 @@ private void RenderInExistingBatch(RenderQueueEntry renderQueueEntry) var componentState = renderQueueEntry.ComponentState; Log.RenderingComponent(_logger, componentState); componentState.RenderIntoBatch(_batchBuilder, renderQueueEntry.RenderFragment, out var renderFragmentException); - if (renderFragmentException != null && !TrySendExceptionToErrorBoundary(componentState.Component, renderFragmentException)) + if (renderFragmentException != null && !TrySendExceptionToErrorBoundary(componentState, renderFragmentException)) { // Exception from render fragment could not be handled by an error boundary, so treat as unhandled (fatal) ExceptionDispatchInfo.Capture(renderFragmentException).Throw(); @@ -816,8 +812,8 @@ private void RenderInExistingBatch(RenderQueueEntry renderQueueEntry) } else { - // We set owningComponent to null because we don't want exceptions during disposal to be recoverable - AddToPendingTasks(GetHandledAsynchronousDisposalErrorsTask(result), owningComponent: null); + // We set owningComponentState to null because we don't want exceptions during disposal to be recoverable + AddToPendingTasks(GetHandledAsynchronousDisposalErrorsTask(result), owningComponentState: null); async Task GetHandledAsynchronousDisposalErrorsTask(Task result) { @@ -834,6 +830,7 @@ async Task GetHandledAsynchronousDisposalErrorsTask(Task result) } _componentStateById.Remove(disposeComponentId); + _componentStateByComponent.Remove(disposeComponentState.Component); _batchBuilder.DisposedComponentIds.Append(disposeComponentId); } @@ -895,7 +892,7 @@ async Task ContinueAfterTask(ArrayRange eventHandlerIds, Task afterTaskIg } } - private async Task GetErrorHandledTask(Task taskToHandle, IComponent? owningComponent) + private async Task GetErrorHandledTask(Task taskToHandle, ComponentState? owningComponentState) { try { @@ -906,7 +903,7 @@ private async Task GetErrorHandledTask(Task taskToHandle, IComponent? owningComp // Ignore errors due to task cancellations. if (!taskToHandle.IsCanceled) { - if (!TrySendExceptionToErrorBoundary(owningComponent, ex)) + if (!TrySendExceptionToErrorBoundary(owningComponentState, ex)) { // It's unhandled, so treat as fatal HandleException(ex); @@ -980,6 +977,7 @@ protected virtual void Dispose(bool disposing) } _componentStateById.Clear(); // So we know they were all disposed + _componentStateByComponent.Clear(); _batchBuilder.Dispose(); NotifyExceptions(exceptions); diff --git a/src/Components/Components/src/Rendering/ComponentState.cs b/src/Components/Components/src/Rendering/ComponentState.cs index e41fd4f583c0..f5692f83e76b 100644 --- a/src/Components/Components/src/Rendering/ComponentState.cs +++ b/src/Components/Components/src/Rendering/ComponentState.cs @@ -207,7 +207,7 @@ private void SupplyCombinedParameters(ParameterView directAndCascadingParameters setParametersAsyncTask = Task.FromException(ex); } - _renderer.AddToPendingTasks(setParametersAsyncTask, Component); + _renderer.AddToPendingTasks(setParametersAsyncTask, owningComponentState: this); } private bool AddCascadingParameterSubscriptions() diff --git a/src/Components/Components/test/RendererTest.cs b/src/Components/Components/test/RendererTest.cs index d0f3278fa466..05cbb4f85b94 100644 --- a/src/Components/Components/test/RendererTest.cs +++ b/src/Components/Components/test/RendererTest.cs @@ -4399,7 +4399,7 @@ public async Task EventDispatchExceptionsCanBeHandledByClosestErrorBoundary_Afte // Assert 2: Error is still handled await task; - Assert.Equal(3, renderer.Batches.Count); + Assert.Equal(2, renderer.Batches.Count); // Didn't re-render as the error boundary was already gone Assert.Collection(errorBoundaries, component => Assert.Null(component.ReceivedException), component => Assert.Same(exception, component.ReceivedException)); From 31def5959c54259f0482faecd24d7e7d4aba747f Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Wed, 7 Apr 2021 11:45:52 +0100 Subject: [PATCH 42/55] Better handling of error loops --- .../Components/src/ErrorBoundaryBase.cs | 50 +++++++++++++++---- .../Components/src/PublicAPI.Unshipped.txt | 2 + .../ErrorBoundaryCases.razor | 4 +- 3 files changed, 43 insertions(+), 13 deletions(-) diff --git a/src/Components/Components/src/ErrorBoundaryBase.cs b/src/Components/Components/src/ErrorBoundaryBase.cs index a77587efb665..ef3969680a45 100644 --- a/src/Components/Components/src/ErrorBoundaryBase.cs +++ b/src/Components/Components/src/ErrorBoundaryBase.cs @@ -12,6 +12,8 @@ namespace Microsoft.AspNetCore.Components /// public abstract class ErrorBoundaryBase : ComponentBase, IErrorBoundary { + private int _errorCount; + /// /// The content to be displayed when there is no error. /// @@ -22,6 +24,12 @@ public abstract class ErrorBoundaryBase : ComponentBase, IErrorBoundary /// [Parameter] public RenderFragment? ErrorContent { get; set; } + /// + /// The maximum number of errors that can be handled. If more errors are received, + /// they will be treated as fatal. Calling resets the count. + /// + [Parameter] public int MaximumErrorCount { get; set; } = 100; + /// /// Gets the current exception, or null if there is no exception. /// @@ -35,6 +43,7 @@ public void Recover() { if (CurrentException is not null) { + _errorCount = 0; CurrentException = null; StateHasChanged(); } @@ -51,23 +60,42 @@ protected virtual 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: + // + // [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); } - // If there's an error while we're already displaying error content, then it may be either of: - // (a) the error content that is failing - // (b) some earlier child content failing an asynchronous task that began before we entered an error state - // In case it's (a), we don't want to risk triggering an infinite error rendering loop by re-rendering. - // This isn't harmful in case (b) because we're already in an error state, so don't need to update the UI further. - // So, only re-render if we're not currently in an error state. - if (CurrentException == null) - { - CurrentException = exception; - StateHasChanged(); - } + CurrentException = exception; + StateHasChanged(); } private async Task HandleOnErrorExceptions(Task onExceptionTask) diff --git a/src/Components/Components/src/PublicAPI.Unshipped.txt b/src/Components/Components/src/PublicAPI.Unshipped.txt index f049a3420469..eddd0c088816 100644 --- a/src/Components/Components/src/PublicAPI.Unshipped.txt +++ b/src/Components/Components/src/PublicAPI.Unshipped.txt @@ -16,6 +16,8 @@ Microsoft.AspNetCore.Components.ErrorBoundaryBase.CurrentException.get -> System Microsoft.AspNetCore.Components.ErrorBoundaryBase.ErrorBoundaryBase() -> void Microsoft.AspNetCore.Components.ErrorBoundaryBase.ErrorContent.get -> Microsoft.AspNetCore.Components.RenderFragment? 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! logger) -> void diff --git a/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryCases.razor b/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryCases.razor index 0896a6840b0a..67f53d47d50c 100644 --- a/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryCases.razor +++ b/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryCases.razor @@ -51,7 +51,7 @@

    Exception inline in error boundary markup

    -

    This shows that, if an ErrorBoundary itself fails while rendering its own ChildContent, then it can catch its own exception (except if it's already showing an exception, in which case it's fatal).

    +

    This shows that, if an ErrorBoundary itself fails while rendering its own ChildContent, then it can catch its own exception. But if the error comes from the error content, this triggers the "infinite error loop" detection logic and becomes fatal.

    @if (throwInline) { throw new InvalidTimeZoneException("Inline exception"); } @@ -63,7 +63,7 @@ - +

    Errors after disposal

    From 015b4e25d11a9da4629e8f3713faf6bbb53532df Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Wed, 7 Apr 2021 11:58:56 +0100 Subject: [PATCH 43/55] Fix description --- .../BasicTestApp/ErrorBoundaryTest/ErrorBoundaryCases.razor | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryCases.razor b/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryCases.razor index 67f53d47d50c..dec42da004e1 100644 --- a/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryCases.razor +++ b/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryCases.razor @@ -44,7 +44,7 @@

    Custom error boundary that tries to ignore errors

    -

    This shows that, even if a custom error boundary tries to continue rendering in a non-error state after an error, the subtree will be forcibly rebuilt. If there's an error while already in an error state, it's fatal.

    +

    This shows that, even if a custom error boundary tries to continue rendering in a non-error state after an error, the subtree will be forcibly rebuilt.

    From 832f72ab6844f2c1c1263c35983d875699b9d46a Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Wed, 7 Apr 2021 12:13:53 +0100 Subject: [PATCH 44/55] Tidyup --- .../Components/src/RenderTree/Renderer.cs | 119 ++++++++---------- 1 file changed, 52 insertions(+), 67 deletions(-) diff --git a/src/Components/Components/src/RenderTree/Renderer.cs b/src/Components/Components/src/RenderTree/Renderer.cs index 25b10f971575..44604fe711a3 100644 --- a/src/Components/Components/src/RenderTree/Renderer.cs +++ b/src/Components/Components/src/RenderTree/Renderer.cs @@ -267,6 +267,51 @@ private void CaptureRootComponentForHotReload(ParameterView initialParameters, C /// The . protected abstract void HandleException(Exception exception); + /// + /// If the exception can be routed to an error boundary around , do so. + /// Otherwise handle it as fatal. + /// + private void HandleExceptionViaErrorBoundary(Exception error, ComponentState? errorSourceOrNull) + { + // We only get here in specific situations. Currently, all of them are when we're + // already on the sync context (and if not, we have a bug we want to know about). + Dispatcher.AssertAccess(); + + // Find the closest error boundary, if any + var candidate = errorSourceOrNull; + while (candidate is not null) + { + if (candidate.Component is IErrorBoundary errorBoundary) + { + // Even though .Web's ErrorBoundary by default removes its ChildContent from the tree when + // switching into an error state, the Renderer doesn't rely on that. To guarantee that + // the failed subtree is disposed, forcibly remove it here. If the failed components did + // continue to run, it wouldn't harm framework state, but it would be a whole new kind of + // edge case to support forever. + AddToRenderQueue(candidate.ComponentId, builder => { }); + + try + { + errorBoundary.HandleException(error); + } + catch (Exception errorBoundaryException) + { + // If *notifying* about an exception fails, it's OK for that to be fatal. This is + // different from an IErrorBoundary component throwing during its own rendering or + // lifecycle methods, which can be handled that error boundary or an ancestor. + HandleException(errorBoundaryException); + } + + return; // Handled successfully + } + + candidate = candidate.ParentComponentState; + } + + // It's unhandled, so treat as fatal + HandleException(error); + } + private async Task ProcessAsynchronousWork() { // Child components SetParametersAsync are stored in the queue of pending tasks, @@ -348,12 +393,7 @@ public virtual Task DispatchEventAsync(ulong eventHandlerId, EventFieldInfo? fie } catch (Exception e) { - if (!TrySendExceptionToErrorBoundary(receiverComponentState, e)) - { - // It's unhandled, so treat as fatal - HandleException(e); - } - + HandleExceptionViaErrorBoundary(e, receiverComponentState); return Task.CompletedTask; } finally @@ -371,51 +411,6 @@ public virtual Task DispatchEventAsync(ulong eventHandlerId, EventFieldInfo? fie return result; } - private bool TrySendExceptionToErrorBoundary(ComponentState? errorSource, Exception error) - { - if (errorSource is null) - { - return false; - } - - // We only get here in specific situations. Currently, all of them are when we're - // already on the sync context (and if not, we have a bug we want to know about). - Dispatcher.AssertAccess(); - - // Find the closest error boundary, if any - var candidate = errorSource; - while (candidate is not null) - { - if (candidate.Component is IErrorBoundary errorBoundary) - { - // Even though .Web's ErrorBoundary by default removes its ChildContent from the tree when - // switching into an error state, the Renderer doesn't rely on that. To guarantee that - // the failed subtree is disposed, forcibly remove it here. If the failed components did - // continue to run, it wouldn't harm framework state, but it would be a whole new kind of - // edge case to support forever. - AddToRenderQueue(candidate.ComponentId, builder => { }); - - try - { - errorBoundary.HandleException(error); - } - catch (Exception errorBoundaryException) - { - // If *notifying* about an exception fails, it's OK for that to be fatal. This is - // different from an IErrorBoundary component throwing during its own rendering or - // lifecycle methods, which can be handled that error boundary or an ancestor. - HandleException(errorBoundaryException); - } - - return true; - } - - candidate = candidate.ParentComponentState; - } - - return false; - } - /// /// Gets the event arguments type for the specified event handler. /// @@ -469,10 +464,7 @@ internal void AddToPendingTasks(Task task, ComponentState? owningComponentState) // the synchronous exceptions will get captured and converted into a faulted // task. var baseException = task.Exception.GetBaseException(); - if (!TrySendExceptionToErrorBoundary(owningComponentState, baseException)) - { - HandleException(baseException); - } + HandleExceptionViaErrorBoundary(baseException, owningComponentState); break; default: // It's important to evaluate the following even if we're not going to use @@ -758,10 +750,7 @@ private void NotifyRenderCompleted(ComponentState state, ref List batch) } else if (task.Status == TaskStatus.Faulted) { - if (!TrySendExceptionToErrorBoundary(state, task.Exception)) - { - HandleException(task.Exception); - } + HandleExceptionViaErrorBoundary(task.Exception, state); return; } } @@ -777,10 +766,10 @@ private void RenderInExistingBatch(RenderQueueEntry renderQueueEntry) var componentState = renderQueueEntry.ComponentState; Log.RenderingComponent(_logger, componentState); componentState.RenderIntoBatch(_batchBuilder, renderQueueEntry.RenderFragment, out var renderFragmentException); - if (renderFragmentException != null && !TrySendExceptionToErrorBoundary(componentState, renderFragmentException)) + if (renderFragmentException != null) { - // Exception from render fragment could not be handled by an error boundary, so treat as unhandled (fatal) - ExceptionDispatchInfo.Capture(renderFragmentException).Throw(); + // If this returns, the error was handled by an error boundary. Otherwise it throws. + HandleExceptionViaErrorBoundary(renderFragmentException, componentState); } List exceptions = null; @@ -903,11 +892,7 @@ private async Task GetErrorHandledTask(Task taskToHandle, ComponentState? owning // Ignore errors due to task cancellations. if (!taskToHandle.IsCanceled) { - if (!TrySendExceptionToErrorBoundary(owningComponentState, ex)) - { - // It's unhandled, so treat as fatal - HandleException(ex); - } + HandleExceptionViaErrorBoundary(ex, owningComponentState); } } } From 50c9ddb57d4d6e375ae702294457a1cc2d8ff9d6 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Wed, 7 Apr 2021 12:16:50 +0100 Subject: [PATCH 45/55] Revert some unwanted changes --- .../BlazorServerApp/Pages/FetchData.razor | 46 +++++++++++++++++++ .../BlazorServerApp/Shared/NavMenu.razor | 4 +- .../BlazorServerApp/wwwroot/css/site.css | 10 ---- 3 files changed, 48 insertions(+), 12 deletions(-) create mode 100644 src/Components/Samples/BlazorServerApp/Pages/FetchData.razor diff --git a/src/Components/Samples/BlazorServerApp/Pages/FetchData.razor b/src/Components/Samples/BlazorServerApp/Pages/FetchData.razor new file mode 100644 index 000000000000..aefc6d9cc01b --- /dev/null +++ b/src/Components/Samples/BlazorServerApp/Pages/FetchData.razor @@ -0,0 +1,46 @@ +@page "/fetchdata" + +@using BlazorServerApp.Data +@inject WeatherForecastService ForecastService + +

    Weather forecast

    + +

    This component demonstrates fetching data from a service.

    + +@if (forecasts == null) +{ +

    Loading...

    +} +else +{ + + + + + + + + + + + @foreach (var forecast in forecasts) + { + + + + + + + } + +
    DateTemp. (C)Temp. (F)Summary
    @forecast.Date.ToShortDateString()@forecast.TemperatureC@forecast.TemperatureF@forecast.Summary
    +} + +@code { + WeatherForecast[]? forecasts; + + protected override async Task OnInitializedAsync() + { + forecasts = await ForecastService.GetForecastAsync(DateTime.Now); + } +} diff --git a/src/Components/Samples/BlazorServerApp/Shared/NavMenu.razor b/src/Components/Samples/BlazorServerApp/Shared/NavMenu.razor index b4d145836c93..5f6f4ae39140 100644 --- a/src/Components/Samples/BlazorServerApp/Shared/NavMenu.razor +++ b/src/Components/Samples/BlazorServerApp/Shared/NavMenu.razor @@ -18,8 +18,8 @@
  • - - Errors + + Fetch data
    • diff --git a/src/Components/Samples/BlazorServerApp/wwwroot/css/site.css b/src/Components/Samples/BlazorServerApp/wwwroot/css/site.css index b95bf4a241f2..bdca6fbfcb87 100644 --- a/src/Components/Samples/BlazorServerApp/wwwroot/css/site.css +++ b/src/Components/Samples/BlazorServerApp/wwwroot/css/site.css @@ -118,16 +118,6 @@ app { color: red; } -.blazor-error-boundary { - background: url(data:image/svg+xml;base64,PHN2ZyB3aWR0aD0iNTYiIGhlaWdodD0iNDkiIHhtbG5zPSJodHRwOi8vd3d3LnczLm9yZy8yMDAwL3N2ZyIgeG1sbnM6eGxpbms9Imh0dHA6Ly93d3cudzMub3JnLzE5OTkveGxpbmsiIG92ZXJmbG93PSJoaWRkZW4iPjxkZWZzPjxjbGlwUGF0aCBpZD0iY2xpcDAiPjxyZWN0IHg9IjIzNSIgeT0iNTEiIHdpZHRoPSI1NiIgaGVpZ2h0PSI0OSIvPjwvY2xpcFBhdGg+PC9kZWZzPjxnIGNsaXAtcGF0aD0idXJsKCNjbGlwMCkiIHRyYW5zZm9ybT0idHJhbnNsYXRlKC0yMzUgLTUxKSI+PHBhdGggZD0iTTI2My41MDYgNTFDMjY0LjcxNyA1MSAyNjUuODEzIDUxLjQ4MzcgMjY2LjYwNiA1Mi4yNjU4TDI2Ny4wNTIgNTIuNzk4NyAyNjcuNTM5IDUzLjYyODMgMjkwLjE4NSA5Mi4xODMxIDI5MC41NDUgOTIuNzk1IDI5MC42NTYgOTIuOTk2QzI5MC44NzcgOTMuNTEzIDI5MSA5NC4wODE1IDI5MSA5NC42NzgyIDI5MSA5Ny4wNjUxIDI4OS4wMzggOTkgMjg2LjYxNyA5OUwyNDAuMzgzIDk5QzIzNy45NjMgOTkgMjM2IDk3LjA2NTEgMjM2IDk0LjY3ODIgMjM2IDk0LjM3OTkgMjM2LjAzMSA5NC4wODg2IDIzNi4wODkgOTMuODA3MkwyMzYuMzM4IDkzLjAxNjIgMjM2Ljg1OCA5Mi4xMzE0IDI1OS40NzMgNTMuNjI5NCAyNTkuOTYxIDUyLjc5ODUgMjYwLjQwNyA1Mi4yNjU4QzI2MS4yIDUxLjQ4MzcgMjYyLjI5NiA1MSAyNjMuNTA2IDUxWk0yNjMuNTg2IDY2LjAxODNDMjYwLjczNyA2Ni4wMTgzIDI1OS4zMTMgNjcuMTI0NSAyNTkuMzEzIDY5LjMzNyAyNTkuMzEzIDY5LjYxMDIgMjU5LjMzMiA2OS44NjA4IDI1OS4zNzEgNzAuMDg4N0wyNjEuNzk1IDg0LjAxNjEgMjY1LjM4IDg0LjAxNjEgMjY3LjgyMSA2OS43NDc1QzI2Ny44NiA2OS43MzA5IDI2Ny44NzkgNjkuNTg3NyAyNjcuODc5IDY5LjMxNzkgMjY3Ljg3OSA2Ny4xMTgyIDI2Ni40NDggNjYuMDE4MyAyNjMuNTg2IDY2LjAxODNaTTI2My41NzYgODYuMDU0N0MyNjEuMDQ5IDg2LjA1NDcgMjU5Ljc4NiA4Ny4zMDA1IDI1OS43ODYgODkuNzkyMSAyNTkuNzg2IDkyLjI4MzcgMjYxLjA0OSA5My41Mjk1IDI2My41NzYgOTMuNTI5NSAyNjYuMTE2IDkzLjUyOTUgMjY3LjM4NyA5Mi4yODM3IDI2Ny4zODcgODkuNzkyMSAyNjcuMzg3IDg3LjMwMDUgMjY2LjExNiA4Ni4wNTQ3IDI2My41NzYgODYuMDU0N1oiIGZpbGw9IiNGRkU1MDAiIGZpbGwtcnVsZT0iZXZlbm9kZCIvPjwvZz48L3N2Zz4=) no-repeat 1rem/1.8rem, #b32121; - padding: 1rem 1rem 1rem 3.7rem; - color: white; -} - - .blazor-error-boundary::after { - content: "An error has occurred." - } - #blazor-error-ui { background: lightyellow; bottom: 0; From 7df7d5a6c6c712800d70692b1ae5dbcde2035e31 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Wed, 7 Apr 2021 12:20:56 +0100 Subject: [PATCH 46/55] More cleanup --- .../Components/src/RenderTree/Renderer.cs | 86 +++++++++---------- 1 file changed, 41 insertions(+), 45 deletions(-) diff --git a/src/Components/Components/src/RenderTree/Renderer.cs b/src/Components/Components/src/RenderTree/Renderer.cs index 44604fe711a3..06e1724b7b5c 100644 --- a/src/Components/Components/src/RenderTree/Renderer.cs +++ b/src/Components/Components/src/RenderTree/Renderer.cs @@ -267,51 +267,6 @@ private void CaptureRootComponentForHotReload(ParameterView initialParameters, C /// The . protected abstract void HandleException(Exception exception); - /// - /// If the exception can be routed to an error boundary around , do so. - /// Otherwise handle it as fatal. - /// - private void HandleExceptionViaErrorBoundary(Exception error, ComponentState? errorSourceOrNull) - { - // We only get here in specific situations. Currently, all of them are when we're - // already on the sync context (and if not, we have a bug we want to know about). - Dispatcher.AssertAccess(); - - // Find the closest error boundary, if any - var candidate = errorSourceOrNull; - while (candidate is not null) - { - if (candidate.Component is IErrorBoundary errorBoundary) - { - // Even though .Web's ErrorBoundary by default removes its ChildContent from the tree when - // switching into an error state, the Renderer doesn't rely on that. To guarantee that - // the failed subtree is disposed, forcibly remove it here. If the failed components did - // continue to run, it wouldn't harm framework state, but it would be a whole new kind of - // edge case to support forever. - AddToRenderQueue(candidate.ComponentId, builder => { }); - - try - { - errorBoundary.HandleException(error); - } - catch (Exception errorBoundaryException) - { - // If *notifying* about an exception fails, it's OK for that to be fatal. This is - // different from an IErrorBoundary component throwing during its own rendering or - // lifecycle methods, which can be handled that error boundary or an ancestor. - HandleException(errorBoundaryException); - } - - return; // Handled successfully - } - - candidate = candidate.ParentComponentState; - } - - // It's unhandled, so treat as fatal - HandleException(error); - } - private async Task ProcessAsynchronousWork() { // Child components SetParametersAsync are stored in the queue of pending tasks, @@ -909,6 +864,47 @@ private void UpdateRenderTreeToMatchClientState(ulong eventHandlerId, EventField } } + /// + /// If the exception can be routed to an error boundary around , do so. + /// Otherwise handle it as fatal. + /// + private void HandleExceptionViaErrorBoundary(Exception error, ComponentState? errorSourceOrNull) + { + // We only get here in specific situations. Currently, all of them are when we're + // already on the sync context (and if not, we have a bug we want to know about). + Dispatcher.AssertAccess(); + + // Find the closest error boundary, if any + var candidate = errorSourceOrNull; + while (candidate is not null) + { + if (candidate.Component is IErrorBoundary errorBoundary) + { + // Don't just trust the error boundary to dispose its subtree - force it to do so by + // making it render an empty fragment. Ensures that failed components don't continue to + // operate, which would be a whole new kind of edge case to support forever. + AddToRenderQueue(candidate.ComponentId, builder => { }); + + try + { + errorBoundary.HandleException(error); + } + catch (Exception errorBoundaryException) + { + // If *notifying* about an exception fails, it's OK for that to be fatal + HandleException(errorBoundaryException); + } + + return; // Handled successfully + } + + candidate = candidate.ParentComponentState; + } + + // It's unhandled, so treat as fatal + HandleException(error); + } + /// /// Releases all resources currently used by this instance. /// From 0a61aaa2aaae91bfa87548b9f8620e20cfe2970b Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Wed, 7 Apr 2021 12:22:43 +0100 Subject: [PATCH 47/55] Add default error boundary content/style to project templates --- .../BlazorServerWeb-CSharp/wwwroot/css/site.css | 10 ++++++++++ .../Client/wwwroot/css/app.css | 10 ++++++++++ 2 files changed, 20 insertions(+) diff --git a/src/ProjectTemplates/Web.ProjectTemplates/content/BlazorServerWeb-CSharp/wwwroot/css/site.css b/src/ProjectTemplates/Web.ProjectTemplates/content/BlazorServerWeb-CSharp/wwwroot/css/site.css index caebf2a4630d..3281f5f5aa65 100644 --- a/src/ProjectTemplates/Web.ProjectTemplates/content/BlazorServerWeb-CSharp/wwwroot/css/site.css +++ b/src/ProjectTemplates/Web.ProjectTemplates/content/BlazorServerWeb-CSharp/wwwroot/css/site.css @@ -48,3 +48,13 @@ a, .btn-link { right: 0.75rem; top: 0.5rem; } + +.blazor-error-boundary { + background: url(data:image/svg+xml;base64,PHN2ZyB3aWR0aD0iNTYiIGhlaWdodD0iNDkiIHhtbG5zPSJodHRwOi8vd3d3LnczLm9yZy8yMDAwL3N2ZyIgeG1sbnM6eGxpbms9Imh0dHA6Ly93d3cudzMub3JnLzE5OTkveGxpbmsiIG92ZXJmbG93PSJoaWRkZW4iPjxkZWZzPjxjbGlwUGF0aCBpZD0iY2xpcDAiPjxyZWN0IHg9IjIzNSIgeT0iNTEiIHdpZHRoPSI1NiIgaGVpZ2h0PSI0OSIvPjwvY2xpcFBhdGg+PC9kZWZzPjxnIGNsaXAtcGF0aD0idXJsKCNjbGlwMCkiIHRyYW5zZm9ybT0idHJhbnNsYXRlKC0yMzUgLTUxKSI+PHBhdGggZD0iTTI2My41MDYgNTFDMjY0LjcxNyA1MSAyNjUuODEzIDUxLjQ4MzcgMjY2LjYwNiA1Mi4yNjU4TDI2Ny4wNTIgNTIuNzk4NyAyNjcuNTM5IDUzLjYyODMgMjkwLjE4NSA5Mi4xODMxIDI5MC41NDUgOTIuNzk1IDI5MC42NTYgOTIuOTk2QzI5MC44NzcgOTMuNTEzIDI5MSA5NC4wODE1IDI5MSA5NC42NzgyIDI5MSA5Ny4wNjUxIDI4OS4wMzggOTkgMjg2LjYxNyA5OUwyNDAuMzgzIDk5QzIzNy45NjMgOTkgMjM2IDk3LjA2NTEgMjM2IDk0LjY3ODIgMjM2IDk0LjM3OTkgMjM2LjAzMSA5NC4wODg2IDIzNi4wODkgOTMuODA3MkwyMzYuMzM4IDkzLjAxNjIgMjM2Ljg1OCA5Mi4xMzE0IDI1OS40NzMgNTMuNjI5NCAyNTkuOTYxIDUyLjc5ODUgMjYwLjQwNyA1Mi4yNjU4QzI2MS4yIDUxLjQ4MzcgMjYyLjI5NiA1MSAyNjMuNTA2IDUxWk0yNjMuNTg2IDY2LjAxODNDMjYwLjczNyA2Ni4wMTgzIDI1OS4zMTMgNjcuMTI0NSAyNTkuMzEzIDY5LjMzNyAyNTkuMzEzIDY5LjYxMDIgMjU5LjMzMiA2OS44NjA4IDI1OS4zNzEgNzAuMDg4N0wyNjEuNzk1IDg0LjAxNjEgMjY1LjM4IDg0LjAxNjEgMjY3LjgyMSA2OS43NDc1QzI2Ny44NiA2OS43MzA5IDI2Ny44NzkgNjkuNTg3NyAyNjcuODc5IDY5LjMxNzkgMjY3Ljg3OSA2Ny4xMTgyIDI2Ni40NDggNjYuMDE4MyAyNjMuNTg2IDY2LjAxODNaTTI2My41NzYgODYuMDU0N0MyNjEuMDQ5IDg2LjA1NDcgMjU5Ljc4NiA4Ny4zMDA1IDI1OS43ODYgODkuNzkyMSAyNTkuNzg2IDkyLjI4MzcgMjYxLjA0OSA5My41Mjk1IDI2My41NzYgOTMuNTI5NSAyNjYuMTE2IDkzLjUyOTUgMjY3LjM4NyA5Mi4yODM3IDI2Ny4zODcgODkuNzkyMSAyNjcuMzg3IDg3LjMwMDUgMjY2LjExNiA4Ni4wNTQ3IDI2My41NzYgODYuMDU0N1oiIGZpbGw9IiNGRkU1MDAiIGZpbGwtcnVsZT0iZXZlbm9kZCIvPjwvZz48L3N2Zz4=) no-repeat 1rem/1.8rem, #b32121; + padding: 1rem 1rem 1rem 3.7rem; + color: white; +} + + .blazor-error-boundary::after { + content: "An error has occurred." + } diff --git a/src/ProjectTemplates/Web.ProjectTemplates/content/ComponentsWebAssembly-CSharp/Client/wwwroot/css/app.css b/src/ProjectTemplates/Web.ProjectTemplates/content/ComponentsWebAssembly-CSharp/Client/wwwroot/css/app.css index 82fc22a39385..b6e17eec7e49 100644 --- a/src/ProjectTemplates/Web.ProjectTemplates/content/ComponentsWebAssembly-CSharp/Client/wwwroot/css/app.css +++ b/src/ProjectTemplates/Web.ProjectTemplates/content/ComponentsWebAssembly-CSharp/Client/wwwroot/css/app.css @@ -48,3 +48,13 @@ a, .btn-link { right: 0.75rem; top: 0.5rem; } + +.blazor-error-boundary { + background: url(data:image/svg+xml;base64,PHN2ZyB3aWR0aD0iNTYiIGhlaWdodD0iNDkiIHhtbG5zPSJodHRwOi8vd3d3LnczLm9yZy8yMDAwL3N2ZyIgeG1sbnM6eGxpbms9Imh0dHA6Ly93d3cudzMub3JnLzE5OTkveGxpbmsiIG92ZXJmbG93PSJoaWRkZW4iPjxkZWZzPjxjbGlwUGF0aCBpZD0iY2xpcDAiPjxyZWN0IHg9IjIzNSIgeT0iNTEiIHdpZHRoPSI1NiIgaGVpZ2h0PSI0OSIvPjwvY2xpcFBhdGg+PC9kZWZzPjxnIGNsaXAtcGF0aD0idXJsKCNjbGlwMCkiIHRyYW5zZm9ybT0idHJhbnNsYXRlKC0yMzUgLTUxKSI+PHBhdGggZD0iTTI2My41MDYgNTFDMjY0LjcxNyA1MSAyNjUuODEzIDUxLjQ4MzcgMjY2LjYwNiA1Mi4yNjU4TDI2Ny4wNTIgNTIuNzk4NyAyNjcuNTM5IDUzLjYyODMgMjkwLjE4NSA5Mi4xODMxIDI5MC41NDUgOTIuNzk1IDI5MC42NTYgOTIuOTk2QzI5MC44NzcgOTMuNTEzIDI5MSA5NC4wODE1IDI5MSA5NC42NzgyIDI5MSA5Ny4wNjUxIDI4OS4wMzggOTkgMjg2LjYxNyA5OUwyNDAuMzgzIDk5QzIzNy45NjMgOTkgMjM2IDk3LjA2NTEgMjM2IDk0LjY3ODIgMjM2IDk0LjM3OTkgMjM2LjAzMSA5NC4wODg2IDIzNi4wODkgOTMuODA3MkwyMzYuMzM4IDkzLjAxNjIgMjM2Ljg1OCA5Mi4xMzE0IDI1OS40NzMgNTMuNjI5NCAyNTkuOTYxIDUyLjc5ODUgMjYwLjQwNyA1Mi4yNjU4QzI2MS4yIDUxLjQ4MzcgMjYyLjI5NiA1MSAyNjMuNTA2IDUxWk0yNjMuNTg2IDY2LjAxODNDMjYwLjczNyA2Ni4wMTgzIDI1OS4zMTMgNjcuMTI0NSAyNTkuMzEzIDY5LjMzNyAyNTkuMzEzIDY5LjYxMDIgMjU5LjMzMiA2OS44NjA4IDI1OS4zNzEgNzAuMDg4N0wyNjEuNzk1IDg0LjAxNjEgMjY1LjM4IDg0LjAxNjEgMjY3LjgyMSA2OS43NDc1QzI2Ny44NiA2OS43MzA5IDI2Ny44NzkgNjkuNTg3NyAyNjcuODc5IDY5LjMxNzkgMjY3Ljg3OSA2Ny4xMTgyIDI2Ni40NDggNjYuMDE4MyAyNjMuNTg2IDY2LjAxODNaTTI2My41NzYgODYuMDU0N0MyNjEuMDQ5IDg2LjA1NDcgMjU5Ljc4NiA4Ny4zMDA1IDI1OS43ODYgODkuNzkyMSAyNTkuNzg2IDkyLjI4MzcgMjYxLjA0OSA5My41Mjk1IDI2My41NzYgOTMuNTI5NSAyNjYuMTE2IDkzLjUyOTUgMjY3LjM4NyA5Mi4yODM3IDI2Ny4zODcgODkuNzkyMSAyNjcuMzg3IDg3LjMwMDUgMjY2LjExNiA4Ni4wNTQ3IDI2My41NzYgODYuMDU0N1oiIGZpbGw9IiNGRkU1MDAiIGZpbGwtcnVsZT0iZXZlbm9kZCIvPjwvZz48L3N2Zz4=) no-repeat 1rem/1.8rem, #b32121; + padding: 1rem 1rem 1rem 3.7rem; + color: white; +} + + .blazor-error-boundary::after { + content: "An error has occurred." + } From 4955cc3155664dacb800d77512b5773810918ee7 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Wed, 7 Apr 2021 12:27:59 +0100 Subject: [PATCH 48/55] Fix server E2E tests --- .../test/E2ETest/ServerExecutionTests/TestSubclasses.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Components/test/E2ETest/ServerExecutionTests/TestSubclasses.cs b/src/Components/test/E2ETest/ServerExecutionTests/TestSubclasses.cs index 72ac00a77a53..6646b1f53751 100644 --- a/src/Components/test/E2ETest/ServerExecutionTests/TestSubclasses.cs +++ b/src/Components/test/E2ETest/ServerExecutionTests/TestSubclasses.cs @@ -108,9 +108,9 @@ public ServerEventCustomArgsTest(BrowserFixture browserFixture, ToggleExecutionM } } - public class ServerErrorBoundaryBase : ErrorBoundaryTest + public class ServerErrorBoundaryTest : ErrorBoundaryTest { - public ServerErrorBoundaryBase(BrowserFixture browserFixture, ToggleExecutionModeServerFixture serverFixture, ITestOutputHelper output) + public ServerErrorBoundaryTest(BrowserFixture browserFixture, ToggleExecutionModeServerFixture serverFixture, ITestOutputHelper output) : base(browserFixture, serverFixture.WithServerExecution(), output) { } From 92e506bbe3438a564d41c6f24c8e2cff9544d70d Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Wed, 7 Apr 2021 12:42:28 +0100 Subject: [PATCH 49/55] Test for custom error boundary --- .../test/E2ETest/Tests/ErrorBoundaryTest.cs | 32 ++++++++++++++++++- .../ErrorBoundaryCases.razor | 11 ++++--- .../ErrorCausingCounter.razor | 6 ++-- 3 files changed, 40 insertions(+), 9 deletions(-) diff --git a/src/Components/test/E2ETest/Tests/ErrorBoundaryTest.cs b/src/Components/test/E2ETest/Tests/ErrorBoundaryTest.cs index 7113f107333b..fa89a53216de 100644 --- a/src/Components/test/E2ETest/Tests/ErrorBoundaryTest.cs +++ b/src/Components/test/E2ETest/Tests/ErrorBoundaryTest.cs @@ -1,6 +1,7 @@ // 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 BasicTestApp; using BasicTestApp.ErrorBoundaryTest; using Microsoft.AspNetCore.Components.E2ETest.Infrastructure; @@ -49,7 +50,36 @@ public void CanHandleExceptions(string triggerId) Assert.Empty(elem.FindElements(By.CssSelector("*"))); }); - // It wasn't a fatal error + AssertNoGlobalError(); + } + + [Fact] + public void CanCreateCustomErrorBoundary() + { + var container = Browser.Exists(By.Id("custom-error-boundary-test")); + Func incrementButtonAccessor = () => container.FindElement(By.ClassName("increment-count")); + Func currentCountAccessor = () => container.FindElement(By.ClassName("current-count")).Text; + + incrementButtonAccessor().Click(); + incrementButtonAccessor().Click(); + Browser.Equal("2", currentCountAccessor); + + // If it throws, we see the custom error boundary + container.FindElement(By.ClassName("throw-counter-exception")).Click(); + Browser.Collection(() => container.FindElements(By.ClassName("received-exception")), + elem => Assert.Equal($"Exception from {nameof(ErrorCausingCounter)}", elem.Text)); + AssertNoGlobalError(); + + // On recovery, the count is reset, because it's a new instance + container.FindElement(By.ClassName("recover")).Click(); + Browser.Equal("0", currentCountAccessor); + incrementButtonAccessor().Click(); + Browser.Equal("1", currentCountAccessor); + Browser.Empty(() => container.FindElements(By.ClassName("received-exception"))); + } + + void AssertNoGlobalError() + { var globalErrorUi = Browser.Exists(By.Id("blazor-error-ui")); Assert.Equal("none", globalErrorUi.GetCssValue("display")); } diff --git a/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryCases.razor b/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryCases.razor index dec42da004e1..3110a450f3e4 100644 --- a/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryCases.razor +++ b/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryCases.razor @@ -36,11 +36,12 @@

    Custom error boundary

    This shows how to create a common custom error UI by subclassing ErrorBoundary.

    - - - - - +
    + + + + +

    Custom error boundary that tries to ignore errors

    diff --git a/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorCausingCounter.razor b/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorCausingCounter.razor index efc03677955e..b4fdf4f82797 100644 --- a/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorCausingCounter.razor +++ b/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorCausingCounter.razor @@ -1,7 +1,7 @@ 

    - Count: @currentCount - - + Count: @currentCount + +

    @code { From 8f18c40156a10b02f6812f9fdaec0b06054f9339 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Wed, 7 Apr 2021 12:46:56 +0100 Subject: [PATCH 50/55] Test for error ignorer --- .../test/E2ETest/Tests/ErrorBoundaryTest.cs | 19 +++++++++++++++++++ .../ErrorBoundaryCases.razor | 8 +++++--- 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/src/Components/test/E2ETest/Tests/ErrorBoundaryTest.cs b/src/Components/test/E2ETest/Tests/ErrorBoundaryTest.cs index fa89a53216de..db25665a15b0 100644 --- a/src/Components/test/E2ETest/Tests/ErrorBoundaryTest.cs +++ b/src/Components/test/E2ETest/Tests/ErrorBoundaryTest.cs @@ -78,6 +78,25 @@ public void CanCreateCustomErrorBoundary() Browser.Empty(() => container.FindElements(By.ClassName("received-exception"))); } + [Fact] + public void HandleCustomErrorBoundaryThatIgnoresErrors() + { + var container = Browser.Exists(By.Id("error-ignorer-test")); + Func incrementButtonAccessor = () => container.FindElement(By.ClassName("increment-count")); + Func currentCountAccessor = () => container.FindElement(By.ClassName("current-count")).Text; + + incrementButtonAccessor().Click(); + incrementButtonAccessor().Click(); + Browser.Equal("2", currentCountAccessor); + + // If it throws, the child content gets forcibly rebuilt even if the error boundary tries to retain it + container.FindElement(By.ClassName("throw-counter-exception")).Click(); + Browser.Equal("0", currentCountAccessor); + incrementButtonAccessor().Click(); + Browser.Equal("1", currentCountAccessor); + AssertNoGlobalError(); + } + void AssertNoGlobalError() { var globalErrorUi = Browser.Exists(By.Id("blazor-error-ui")); diff --git a/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryCases.razor b/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryCases.razor index 3110a450f3e4..64b90ac19dc6 100644 --- a/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryCases.razor +++ b/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryCases.razor @@ -46,9 +46,11 @@

    Custom error boundary that tries to ignore errors

    This shows that, even if a custom error boundary tries to continue rendering in a non-error state after an error, the subtree will be forcibly rebuilt.

    - - - +
    + + + +

    Exception inline in error boundary markup

    From d151976fb5469b988ff3c53f572cc7d584372381 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Wed, 7 Apr 2021 12:59:09 +0100 Subject: [PATCH 51/55] Test for inline errors --- .../test/E2ETest/Tests/ErrorBoundaryTest.cs | 27 +++++++++++++++---- .../ErrorBoundaryCases.razor | 26 +++++++++--------- 2 files changed, 36 insertions(+), 17 deletions(-) diff --git a/src/Components/test/E2ETest/Tests/ErrorBoundaryTest.cs b/src/Components/test/E2ETest/Tests/ErrorBoundaryTest.cs index db25665a15b0..f1ebbf70aafd 100644 --- a/src/Components/test/E2ETest/Tests/ErrorBoundaryTest.cs +++ b/src/Components/test/E2ETest/Tests/ErrorBoundaryTest.cs @@ -50,7 +50,7 @@ public void CanHandleExceptions(string triggerId) Assert.Empty(elem.FindElements(By.CssSelector("*"))); }); - AssertNoGlobalError(); + AssertGlobalErrorState(false); } [Fact] @@ -68,7 +68,7 @@ public void CanCreateCustomErrorBoundary() container.FindElement(By.ClassName("throw-counter-exception")).Click(); Browser.Collection(() => container.FindElements(By.ClassName("received-exception")), elem => Assert.Equal($"Exception from {nameof(ErrorCausingCounter)}", elem.Text)); - AssertNoGlobalError(); + AssertGlobalErrorState(false); // On recovery, the count is reset, because it's a new instance container.FindElement(By.ClassName("recover")).Click(); @@ -94,13 +94,30 @@ public void HandleCustomErrorBoundaryThatIgnoresErrors() Browser.Equal("0", currentCountAccessor); incrementButtonAccessor().Click(); Browser.Equal("1", currentCountAccessor); - AssertNoGlobalError(); + AssertGlobalErrorState(false); } - void AssertNoGlobalError() + [Fact] + public void CanHandleErrorsInlineInErrorBoundaryContent() + { + var container = Browser.Exists(By.Id("inline-error-test")); + Browser.Equal("Hello!", () => container.FindElement(By.ClassName("normal-content")).Text); + Assert.Empty(container.FindElements(By.ClassName("error-message"))); + + // If ChildContent throws during rendering, the error boundary handles it + container.FindElement(By.ClassName("throw-in-childcontent")).Click(); + Browser.Contains("There was an error: System.InvalidTimeZoneException: Inline exception", () => container.FindElement(By.ClassName("error-message")).Text); + AssertGlobalErrorState(false); + + // If the ErrorContent throws during rendering, it gets caught by the "infinite error loop" detection logic and is fatal + container.FindElement(By.ClassName("throw-in-errorcontent")).Click(); + AssertGlobalErrorState(true); + } + + void AssertGlobalErrorState(bool hasGlobalError) { var globalErrorUi = Browser.Exists(By.Id("blazor-error-ui")); - Assert.Equal("none", globalErrorUi.GetCssValue("display")); + Browser.Equal(hasGlobalError ? "block" : "none", () => globalErrorUi.GetCssValue("display")); } } } diff --git a/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryCases.razor b/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryCases.razor index 64b90ac19dc6..e514cbe322f1 100644 --- a/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryCases.razor +++ b/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryCases.razor @@ -55,18 +55,20 @@

    Exception inline in error boundary markup

    This shows that, if an ErrorBoundary itself fails while rendering its own ChildContent, then it can catch its own exception. But if the error comes from the error content, this triggers the "infinite error loop" detection logic and becomes fatal.

    - - - @if (throwInline) { throw new InvalidTimeZoneException("Inline exception"); } -

    Hello!

    -     - - @if (throwInErrorContent) { throw new InvalidTimeZoneException("Inline exception in error content"); } -

    There was an error: @context

    -     -     - - +
    + + + @if (throwInline) { throw new InvalidTimeZoneException("Inline exception"); } +

    Hello!

    +     + + @if (throwInErrorContent) { throw new InvalidTimeZoneException("Inline exception in error content"); } +

    There was an error: @context

    +     +     + + +

    Errors after disposal

    From d85c8d244735cb50e93929d0770d9d51c046025b Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Wed, 7 Apr 2021 13:08:46 +0100 Subject: [PATCH 52/55] Test for errors after disposal --- .../test/E2ETest/Tests/ErrorBoundaryTest.cs | 30 +++++++++++++++++++ .../ErrorBoundaryCases.razor | 24 ++++++++------- 2 files changed, 43 insertions(+), 11 deletions(-) diff --git a/src/Components/test/E2ETest/Tests/ErrorBoundaryTest.cs b/src/Components/test/E2ETest/Tests/ErrorBoundaryTest.cs index f1ebbf70aafd..2d8f2c394750 100644 --- a/src/Components/test/E2ETest/Tests/ErrorBoundaryTest.cs +++ b/src/Components/test/E2ETest/Tests/ErrorBoundaryTest.cs @@ -2,6 +2,7 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; +using System.Threading.Tasks; using BasicTestApp; using BasicTestApp.ErrorBoundaryTest; using Microsoft.AspNetCore.Components.E2ETest.Infrastructure; @@ -114,6 +115,35 @@ public void CanHandleErrorsInlineInErrorBoundaryContent() AssertGlobalErrorState(true); } + [Fact] + public void CanHandleErrorsAfterDisposingComponent() + { + var container = Browser.Exists(By.Id("error-after-disposal-test")); + + container.FindElement(By.ClassName("throw-after-disposing-component")).Click(); + Browser.Collection(() => container.FindElements(By.ClassName("received-exception")), + elem => Assert.Equal($"Delayed asynchronous exception in OnParametersSetAsync", elem.Text)); + + AssertGlobalErrorState(false); + } + + [Fact] + public async Task CanHandleErrorsAfterDisposingErrorBoundary() + { + var container = Browser.Exists(By.Id("error-after-disposal-test")); + container.FindElement(By.ClassName("throw-after-disposing-errorboundary")).Click(); + + // Because we've actually removed the error boundary, there isn't any UI for us to assert about. + // The following delay is a cheap way to check for that - in the worst case, we could get a false + // test pass here if the delay is somehow not long enough, but this should never lead to a false + // failure (i.e., flakiness). + await Task.Delay(1000); // The test exception occurs after 500ms + + // We succeed as long as there's no global error and the rest of the UI is still there + Browser.Exists(By.Id("error-after-disposal-test")); + AssertGlobalErrorState(false); + } + void AssertGlobalErrorState(bool hasGlobalError) { var globalErrorUi = Browser.Exists(By.Id("blazor-error-ui")); diff --git a/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryCases.razor b/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryCases.razor index e514cbe322f1..94c8a805487d 100644 --- a/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryCases.razor +++ b/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryCases.razor @@ -73,17 +73,19 @@

    Errors after disposal

    Long-running tasks could fail after the component has been removed from the tree. We still want these failures to be captured by the error boundary they were inside when the task began, even if that error boundary itself has also since been disposed. Otherwise, error handling would behave differently based on whether the user has navigated away while processing was in flight, which would be very unexpected and hard to handle.

    -@if (!disposalTestRemoveErrorBoundary) -{ - - @if (!disposalTestRemoveComponent) - { - - } - -} - - +
    + @if (!disposalTestRemoveErrorBoundary) + { + + @if (!disposalTestRemoveComponent) + { + + } + + } + + +

    Multiple child errors

    From 00e5e50eb080c43746a2a8dab8c4916c7d709493 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Wed, 7 Apr 2021 13:12:44 +0100 Subject: [PATCH 53/55] Test for multiple concurrent errors --- .../test/E2ETest/Tests/ErrorBoundaryTest.cs | 17 ++++++++++++++++- .../ErrorBoundaryTest/ErrorBoundaryCases.razor | 14 ++++++++------ 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/src/Components/test/E2ETest/Tests/ErrorBoundaryTest.cs b/src/Components/test/E2ETest/Tests/ErrorBoundaryTest.cs index 2d8f2c394750..d1fcb7b7727d 100644 --- a/src/Components/test/E2ETest/Tests/ErrorBoundaryTest.cs +++ b/src/Components/test/E2ETest/Tests/ErrorBoundaryTest.cs @@ -122,7 +122,7 @@ public void CanHandleErrorsAfterDisposingComponent() container.FindElement(By.ClassName("throw-after-disposing-component")).Click(); Browser.Collection(() => container.FindElements(By.ClassName("received-exception")), - elem => Assert.Equal($"Delayed asynchronous exception in OnParametersSetAsync", elem.Text)); + elem => Assert.Equal("Delayed asynchronous exception in OnParametersSetAsync", elem.Text)); AssertGlobalErrorState(false); } @@ -144,6 +144,21 @@ public async Task CanHandleErrorsAfterDisposingErrorBoundary() AssertGlobalErrorState(false); } + [Fact] + public void CanHandleMultipleAsyncErrorsFromDescendants() + { + var container = Browser.Exists(By.Id("multiple-child-errors-test")); + var message = "Delayed asynchronous exception in OnParametersSetAsync"; + + container.FindElement(By.ClassName("throw-in-children")).Click(); + Browser.Collection(() => container.FindElements(By.ClassName("received-exception")), + elem => Assert.Equal(message, elem.Text), + elem => Assert.Equal(message, elem.Text), + elem => Assert.Equal(message, elem.Text)); + + AssertGlobalErrorState(false); + } + void AssertGlobalErrorState(bool hasGlobalError) { var globalErrorUi = Browser.Exists(By.Id("blazor-error-ui")); diff --git a/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryCases.razor b/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryCases.razor index 94c8a805487d..3b87c1e7c54f 100644 --- a/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryCases.razor +++ b/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorBoundaryCases.razor @@ -90,12 +90,14 @@

    Multiple child errors

    If several child components trigger asynchronous errors, the error boundary will receive error notifications even when it's already in an errored state. This needs to behave sensibly.

    - - - - - - +
    + + + + + + +
    @code { private bool throwInOnParametersSet; From 5eae0b110b6e158f22103e403e2224f88d48f0fa Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Wed, 7 Apr 2021 13:48:05 +0100 Subject: [PATCH 54/55] Stronger guidance to override OnErrorAsync --- src/Components/Components/src/ErrorBoundaryBase.cs | 8 +++----- src/Components/Components/src/PublicAPI.Unshipped.txt | 2 +- src/Components/Web/src/Web/ErrorBoundary.cs | 6 +++++- .../BasicTestApp/ErrorBoundaryTest/ErrorIgnorer.razor | 8 ++++++++ 4 files changed, 17 insertions(+), 7 deletions(-) diff --git a/src/Components/Components/src/ErrorBoundaryBase.cs b/src/Components/Components/src/ErrorBoundaryBase.cs index ef3969680a45..f35441db7e8d 100644 --- a/src/Components/Components/src/ErrorBoundaryBase.cs +++ b/src/Components/Components/src/ErrorBoundaryBase.cs @@ -50,13 +50,11 @@ public void Recover() } /// - /// Invoked by the base class when an error is being handled. + /// Invoked by the base class when an error is being handled. Typically, derived classes + /// should log the exception from this method. /// /// The being handled. - protected virtual Task OnErrorAsync(Exception exception) - { - return Task.CompletedTask; - } + protected abstract Task OnErrorAsync(Exception exception); void IErrorBoundary.HandleException(Exception exception) { diff --git a/src/Components/Components/src/PublicAPI.Unshipped.txt b/src/Components/Components/src/PublicAPI.Unshipped.txt index eddd0c088816..506b89211a67 100644 --- a/src/Components/Components/src/PublicAPI.Unshipped.txt +++ b/src/Components/Components/src/PublicAPI.Unshipped.txt @@ -43,8 +43,8 @@ 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! parameters) -> Microsoft.AspNetCore.Components.ParameterView -virtual Microsoft.AspNetCore.Components.ErrorBoundaryBase.OnErrorAsync(System.Exception! exception) -> System.Threading.Tasks.Task! virtual Microsoft.AspNetCore.Components.RenderTree.Renderer.DispatchEventAsync(ulong eventHandlerId, Microsoft.AspNetCore.Components.RenderTree.EventFieldInfo? fieldInfo, System.EventArgs! eventArgs) -> System.Threading.Tasks.Task! readonly Microsoft.AspNetCore.Components.RenderTree.RenderTreeEdit.RemovedAttributeName -> string? diff --git a/src/Components/Web/src/Web/ErrorBoundary.cs b/src/Components/Web/src/Web/ErrorBoundary.cs index cf42bc4acee5..85eef462f243 100644 --- a/src/Components/Web/src/Web/ErrorBoundary.cs +++ b/src/Components/Web/src/Web/ErrorBoundary.cs @@ -14,7 +14,11 @@ public class ErrorBoundary : ErrorBoundaryBase { [Inject] private IErrorBoundaryLogger? ErrorBoundaryLogger { get; set; } - /// + /// + /// Invoked by the base class when an error is being handled. The default implementation + /// logs the error. + /// + /// The being handled. protected override async Task OnErrorAsync(Exception exception) { await ErrorBoundaryLogger!.LogErrorAsync(exception); diff --git a/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorIgnorer.razor b/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorIgnorer.razor index 3a43d7a985a2..1bb5bdfacb92 100644 --- a/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorIgnorer.razor +++ b/src/Components/test/testassets/BasicTestApp/ErrorBoundaryTest/ErrorIgnorer.razor @@ -4,3 +4,11 @@ This is to check we still don't retain the descendant component instances. *@ @ChildContent + +@code { + protected override Task OnErrorAsync(Exception exception) + { + // Ignore it + return Task.CompletedTask; + } +} From 9ace45d8dffb0c89026df3cd989b49f87c61344c Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Thu, 8 Apr 2021 16:59:09 +0100 Subject: [PATCH 55/55] CR: Improve unit test --- .../Components/test/RendererTest.cs | 34 +++++++++++-------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/src/Components/Components/test/RendererTest.cs b/src/Components/Components/test/RendererTest.cs index 05cbb4f85b94..85185a710e84 100644 --- a/src/Components/Components/test/RendererTest.cs +++ b/src/Components/Components/test/RendererTest.cs @@ -4242,13 +4242,14 @@ public async Task SetParametersAsyncExceptionsCanBeHandledByClosestErrorBoundary { // Arrange var renderer = new TestRenderer(); - Exception exception = null; + var exception = new InvalidTimeZoneException("Error during SetParametersAsync"); + TaskCompletionSource exceptionTcs = null; var rootComponent = new TestComponent(builder => { TestErrorBoundary.RenderNestedErrorBoundaries(builder, builder => { builder.OpenComponent(0); - builder.AddAttribute(1, nameof(ErrorThrowingComponent.ThrowDuringParameterSettingAsync), exception); + builder.AddAttribute(1, nameof(ErrorThrowingComponent.ThrowDuringParameterSettingAsync), exceptionTcs?.Task); builder.CloseComponent(); }); }); @@ -4260,11 +4261,12 @@ public async Task SetParametersAsyncExceptionsCanBeHandledByClosestErrorBoundary .GetComponentFrames().Single().ComponentId; // Act/Assert 1: No synchronous errors - exception = new InvalidTimeZoneException("Error during SetParametersAsync"); + exceptionTcs = new TaskCompletionSource(); rootComponent.TriggerRender(); Assert.Equal(2, renderer.Batches.Count); // Act/Assert 2: Asynchronous error + exceptionTcs.SetException(exception); await errorBoundaries[1].ReceivedErrorTask; Assert.Equal(3, renderer.Batches.Count); Assert.Collection(errorBoundaries, @@ -4319,12 +4321,13 @@ public async Task EventDispatchExceptionsCanBeHandledByClosestErrorBoundary_Asyn // Arrange var renderer = new TestRenderer(); var exception = new InvalidTimeZoneException("Error during event"); + var exceptionTcs = new TaskCompletionSource(); var rootComponentId = renderer.AssignRootComponentId(new TestComponent(builder => { TestErrorBoundary.RenderNestedErrorBoundaries(builder, builder => { builder.OpenComponent(0); - builder.AddAttribute(1, nameof(ErrorThrowingComponent.ThrowDuringEventAsync), exception); + builder.AddAttribute(1, nameof(ErrorThrowingComponent.ThrowDuringEventAsync), exceptionTcs.Task); builder.CloseComponent(); }); })); @@ -4338,14 +4341,15 @@ public async Task EventDispatchExceptionsCanBeHandledByClosestErrorBoundary_Asyn .AttributeEventHandlerId; // Act/Assert 1: No error synchronously - var task = renderer.DispatchEventAsync(eventHandlerId, new EventArgs()); + var dispatchEventTask = renderer.DispatchEventAsync(eventHandlerId, new EventArgs()); Assert.Single(renderer.Batches); Assert.Collection(errorBoundaries, component => Assert.Null(component.ReceivedException), component => Assert.Null(component.ReceivedException)); // Act/Assert 2: Error is handled asynchronously - await task; + exceptionTcs.SetException(exception); + await dispatchEventTask; Assert.Equal(2, renderer.Batches.Count); Assert.Collection(errorBoundaries, component => Assert.Null(component.ReceivedException), @@ -4362,6 +4366,7 @@ public async Task EventDispatchExceptionsCanBeHandledByClosestErrorBoundary_Afte var renderer = new TestRenderer(); var disposeChildren = false; var exception = new InvalidTimeZoneException("Error during event"); + var exceptionTcs = new TaskCompletionSource(); var rootComponent = new TestComponent(builder => { if (!disposeChildren) @@ -4369,7 +4374,7 @@ public async Task EventDispatchExceptionsCanBeHandledByClosestErrorBoundary_Afte TestErrorBoundary.RenderNestedErrorBoundaries(builder, builder => { builder.OpenComponent(0); - builder.AddAttribute(1, nameof(ErrorThrowingComponent.ThrowDuringEventAsync), exception); + builder.AddAttribute(1, nameof(ErrorThrowingComponent.ThrowDuringEventAsync), exceptionTcs.Task); builder.CloseComponent(); }); } @@ -4385,7 +4390,7 @@ public async Task EventDispatchExceptionsCanBeHandledByClosestErrorBoundary_Afte .AttributeEventHandlerId; // Act/Assert 1: No error synchronously - var task = renderer.DispatchEventAsync(eventHandlerId, new EventArgs()); + var dispatchEventTask = renderer.DispatchEventAsync(eventHandlerId, new EventArgs()); Assert.Single(renderer.Batches); Assert.Collection(errorBoundaries, component => Assert.Null(component.ReceivedException), @@ -4398,7 +4403,8 @@ public async Task EventDispatchExceptionsCanBeHandledByClosestErrorBoundary_Afte Assert.Contains(errorThrowingComponentId, renderer.Batches.Last().DisposedComponentIDs); // Assert 2: Error is still handled - await task; + exceptionTcs.SetException(exception); + await dispatchEventTask; Assert.Equal(2, renderer.Batches.Count); // Didn't re-render as the error boundary was already gone Assert.Collection(errorBoundaries, component => Assert.Null(component.ReceivedException), @@ -5240,9 +5246,9 @@ private class ErrorThrowingComponent : AutoRenderComponent, IHandleEvent { [Parameter] public Exception ThrowDuringRender { get; set; } [Parameter] public Exception ThrowDuringEventSync { get; set; } - [Parameter] public Exception ThrowDuringEventAsync { get; set; } + [Parameter] public Task ThrowDuringEventAsync { get; set; } [Parameter] public Exception ThrowDuringParameterSettingSync { get; set; } - [Parameter] public Exception ThrowDuringParameterSettingAsync { get; set; } + [Parameter] public Task ThrowDuringParameterSettingAsync { get; set; } public override async Task SetParametersAsync(ParameterView parameters) { @@ -5255,8 +5261,7 @@ public override async Task SetParametersAsync(ParameterView parameters) if (ThrowDuringParameterSettingAsync is not null) { - await Task.Delay(100); - throw ThrowDuringParameterSettingAsync; + await ThrowDuringParameterSettingAsync; } } @@ -5282,8 +5287,7 @@ public async Task HandleEventAsync(EventCallbackWorkItem item, object arg) if (ThrowDuringEventAsync is not null) { - await Task.Delay(100); - throw ThrowDuringEventAsync; + await ThrowDuringEventAsync; } } }