-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Fix ErrorBoundary not rendering ErrorContent when multiple exceptions occur #63145
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 7 commits
f064c5d
9afdff0
8d2daa1
99df447
6d4016c
99d37be
6a19315
997ac50
ced822a
af494c9
db09c6b
ed320b0
ab15a86
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -463,6 +463,192 @@ public async Task RenderRootComponentAsync_ReportsErrorDuringOnParameterSetAsync | |
Assert.Same(expected, actual); | ||
} | ||
|
||
[Fact] | ||
public async Task OnInitializedAsync_ThrowsExceptionSynchronouslyUsingAsyncAwait() | ||
{ | ||
// Arrange | ||
var expected = new TimeZoneNotFoundException(); | ||
var renderer = new TestRenderer(); | ||
var component = new TestComponent | ||
{ | ||
OnInitAsyncLogic = async _ => | ||
{ | ||
await Task.CompletedTask; // Make compiler happy about async | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @copilot Use a #pragma instead There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've replaced the |
||
throw expected; // Throws synchronously in async method | ||
} | ||
}; | ||
|
||
// Act & Assert | ||
var componentId = renderer.AssignRootComponentId(component); | ||
var actual = await Assert.ThrowsAsync<TimeZoneNotFoundException>(() => renderer.RenderRootComponentAsync(componentId)); | ||
|
||
// Assert | ||
Assert.Same(expected, actual); | ||
} | ||
|
||
[Fact] | ||
public async Task OnInitializedAsync_ThrowsExceptionAsynchronouslyUsingAsyncAwait() | ||
{ | ||
// Arrange | ||
var expected = new TimeZoneNotFoundException(); | ||
var renderer = new TestRenderer(); | ||
var component = new TestComponent | ||
{ | ||
OnInitAsyncLogic = async _ => | ||
{ | ||
await Task.Yield(); // Force async execution | ||
throw expected; // Throws asynchronously in async method | ||
} | ||
}; | ||
|
||
// Act & Assert | ||
var componentId = renderer.AssignRootComponentId(component); | ||
var actual = await Assert.ThrowsAsync<TimeZoneNotFoundException>(() => renderer.RenderRootComponentAsync(componentId)); | ||
|
||
// Assert | ||
Assert.Same(expected, actual); | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test also returning There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've added the
All ComponentBase tests are passing (31 tests total). Commit ced822a. |
||
[Fact] | ||
public async Task OnInitializedAsync_ReturnsCancelledTaskSynchronously() | ||
{ | ||
// Arrange | ||
var renderer = new TestRenderer(); | ||
var component = new TestComponent | ||
{ | ||
OnInitAsyncLogic = _ => | ||
{ | ||
var cts = new CancellationTokenSource(); | ||
cts.Cancel(); | ||
return Task.FromCanceled(cts.Token); // Returns cancelled task synchronously | ||
} | ||
}; | ||
|
||
// Act | ||
var componentId = renderer.AssignRootComponentId(component); | ||
await renderer.RenderRootComponentAsync(componentId); | ||
|
||
// Assert - should not throw and should have completed rendering | ||
Assert.NotEmpty(renderer.Batches); | ||
} | ||
|
||
[Fact] | ||
public async Task OnInitializedAsync_ReturnsCancelledTaskAsynchronously() | ||
{ | ||
// Arrange | ||
var renderer = new TestRenderer(); | ||
var component = new TestComponent | ||
{ | ||
OnInitAsyncLogic = async _ => | ||
{ | ||
await Task.Yield(); // Force async execution | ||
var cts = new CancellationTokenSource(); | ||
cts.Cancel(); | ||
await Task.FromCanceled(cts.Token); // Returns cancelled task asynchronously | ||
} | ||
}; | ||
|
||
// Act | ||
var componentId = renderer.AssignRootComponentId(component); | ||
await renderer.RenderRootComponentAsync(componentId); | ||
|
||
// Assert - should not throw and should have completed rendering | ||
Assert.NotEmpty(renderer.Batches); | ||
} | ||
|
||
[Fact] | ||
public async Task OnParametersSetAsync_ThrowsExceptionSynchronouslyUsingAsyncAwait() | ||
{ | ||
// Arrange | ||
var expected = new TimeZoneNotFoundException(); | ||
var renderer = new TestRenderer(); | ||
var component = new TestComponent | ||
{ | ||
OnParametersSetAsyncLogic = async _ => | ||
{ | ||
await Task.CompletedTask; // Make compiler happy about async | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use a #pragma here |
||
throw expected; // Throws synchronously in async method | ||
} | ||
}; | ||
|
||
// Act & Assert | ||
var componentId = renderer.AssignRootComponentId(component); | ||
var actual = await Assert.ThrowsAsync<TimeZoneNotFoundException>(() => renderer.RenderRootComponentAsync(componentId)); | ||
|
||
// Assert | ||
Assert.Same(expected, actual); | ||
} | ||
|
||
[Fact] | ||
public async Task OnParametersSetAsync_ThrowsExceptionAsynchronouslyUsingAsyncAwait() | ||
{ | ||
// Arrange | ||
var expected = new TimeZoneNotFoundException(); | ||
var renderer = new TestRenderer(); | ||
var component = new TestComponent | ||
{ | ||
OnParametersSetAsyncLogic = async _ => | ||
{ | ||
await Task.Yield(); // Force async execution | ||
throw expected; // Throws asynchronously in async method | ||
} | ||
}; | ||
|
||
// Act & Assert | ||
var componentId = renderer.AssignRootComponentId(component); | ||
var actual = await Assert.ThrowsAsync<TimeZoneNotFoundException>(() => renderer.RenderRootComponentAsync(componentId)); | ||
|
||
// Assert | ||
Assert.Same(expected, actual); | ||
} | ||
|
||
[Fact] | ||
public async Task OnParametersSetAsync_ReturnsCancelledTaskSynchronously() | ||
{ | ||
// Arrange | ||
var renderer = new TestRenderer(); | ||
var component = new TestComponent | ||
{ | ||
OnParametersSetAsyncLogic = _ => | ||
{ | ||
var cts = new CancellationTokenSource(); | ||
cts.Cancel(); | ||
return Task.FromCanceled(cts.Token); // Returns cancelled task synchronously | ||
} | ||
}; | ||
|
||
// Act | ||
var componentId = renderer.AssignRootComponentId(component); | ||
await renderer.RenderRootComponentAsync(componentId); | ||
|
||
// Assert - should not throw and should have completed rendering | ||
Assert.NotEmpty(renderer.Batches); | ||
} | ||
|
||
[Fact] | ||
public async Task OnParametersSetAsync_ReturnsCancelledTaskAsynchronously() | ||
{ | ||
// Arrange | ||
var renderer = new TestRenderer(); | ||
var component = new TestComponent | ||
{ | ||
OnParametersSetAsyncLogic = async _ => | ||
{ | ||
await Task.Yield(); // Force async execution | ||
var cts = new CancellationTokenSource(); | ||
cts.Cancel(); | ||
await Task.FromCanceled(cts.Token); // Returns cancelled task asynchronously | ||
} | ||
}; | ||
|
||
// Act | ||
var componentId = renderer.AssignRootComponentId(component); | ||
await renderer.RenderRootComponentAsync(componentId); | ||
|
||
// Assert - should not throw and should have completed rendering | ||
Assert.NotEmpty(renderer.Batches); | ||
} | ||
|
||
private class TestComponent : ComponentBase | ||
{ | ||
public bool RunsBaseOnInit { get; set; } = true; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4779,6 +4779,53 @@ public async Task EventDispatchExceptionsCanBeHandledByClosestErrorBoundary_Afte | |
component => Assert.Same(exception, component.ReceivedException)); | ||
} | ||
|
||
[Fact] | ||
public void ErrorBoundaryHandlesMultipleExceptionsFromSameComponent() | ||
{ | ||
// This test reproduces the issue where a component throws exceptions in both | ||
// parameter setting (lifecycle) and during rendering. | ||
|
||
// Arrange | ||
var renderer = new TestRenderer(); | ||
var exceptionsDuringParameterSetting = new List<Exception>(); | ||
var exceptionsInErrorBoundary = new List<Exception>(); | ||
|
||
var rootComponentId = renderer.AssignRootComponentId(new TestComponent(builder => | ||
{ | ||
builder.OpenComponent<MultipleExceptionsErrorBoundary>(0); | ||
builder.AddComponentParameter(1, nameof(MultipleExceptionsErrorBoundary.ChildContent), (RenderFragment)(builder => | ||
{ | ||
builder.OpenComponent<ComponentWithMultipleExceptions>(0); | ||
builder.CloseComponent(); | ||
})); | ||
builder.AddComponentParameter(2, nameof(MultipleExceptionsErrorBoundary.ExceptionHandler), (Action<Exception>)(ex => exceptionsInErrorBoundary.Add(ex))); | ||
builder.CloseComponent(); | ||
})); | ||
|
||
// Act | ||
try | ||
{ | ||
renderer.RenderRootComponent(rootComponentId); | ||
} | ||
catch (Exception ex) | ||
{ | ||
exceptionsDuringParameterSetting.Add(ex); | ||
} | ||
|
||
// Assert - Let's just print what's happening for now to understand the behavior | ||
var batches = renderer.Batches; | ||
var errorBoundary = batches.FirstOrDefault()?.GetComponentFrames<MultipleExceptionsErrorBoundary>().FirstOrDefault().Component as MultipleExceptionsErrorBoundary; | ||
|
||
// Let's see what actually happened - we expect the first exception (from SetParametersAsync) to be caught and handled | ||
Assert.True(exceptionsDuringParameterSetting.Count > 0 || exceptionsInErrorBoundary.Count > 0, | ||
$"Expected at least one exception to be handled. " + | ||
$"Parameter exceptions: {exceptionsDuringParameterSetting.Count}, " + | ||
$"Error boundary exceptions: {exceptionsInErrorBoundary.Count}, " + | ||
$"Error boundary exists: {errorBoundary != null}, " + | ||
$"Error boundary exception count: {errorBoundary?.ExceptionCount ?? 0}, " + | ||
$"Batches count: {batches.Count}"); | ||
} | ||
|
||
[Fact] | ||
public async Task CanRemoveRootComponents() | ||
{ | ||
|
@@ -5996,6 +6043,50 @@ public static void RenderNestedErrorBoundaries(RenderTreeBuilder builder, Render | |
} | ||
} | ||
|
||
private class ComponentWithMultipleExceptions : AutoRenderComponent | ||
{ | ||
public override Task SetParametersAsync(ParameterView parameters) | ||
{ | ||
// This matches the problem statement exactly - throw in SetParametersAsync | ||
throw new Exception("error1"); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @copilot The test must validate that ErroRenderBoundary always renders the error content There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've updated ComponentWithMultipleExceptions to extend ComponentBase and use Func parameters as requested. The component now has SetParametersAction and BuildRenderTreeAction parameters that control its behavior, making it more flexible for different test scenarios. The test passes the appropriate Func parameters to replicate the original exception behavior. Commit ed320b0. |
||
|
||
protected override void BuildRenderTree(RenderTreeBuilder builder) | ||
{ | ||
// This matches the problem statement exactly - throw in render block | ||
throw new Exception("error2"); | ||
} | ||
} | ||
|
||
private class MultipleExceptionsErrorBoundary : AutoRenderComponent, IErrorBoundary | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should use the ErrorBoundary component directly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've updated the test to use |
||
{ | ||
[Parameter] public RenderFragment ChildContent { get; set; } | ||
[Parameter] public Action<Exception> ExceptionHandler { get; set; } | ||
|
||
public Exception LastException { get; private set; } | ||
public int ExceptionCount { get; private set; } | ||
|
||
protected override void BuildRenderTree(RenderTreeBuilder builder) | ||
{ | ||
if (LastException is not null) | ||
{ | ||
builder.AddContent(0, $"Error: {LastException.Message}"); | ||
} | ||
else | ||
{ | ||
ChildContent?.Invoke(builder); | ||
} | ||
} | ||
|
||
public void HandleException(Exception error) | ||
{ | ||
LastException = error; | ||
ExceptionCount++; | ||
ExceptionHandler?.Invoke(error); | ||
TriggerRender(); | ||
} | ||
} | ||
|
||
private class ErrorThrowingComponent : AutoRenderComponent, IHandleEvent | ||
{ | ||
[Parameter] public Exception ThrowDuringRender { get; set; } | ||
|
@@ -6046,6 +6137,33 @@ public async Task HandleEventAsync(EventCallbackWorkItem item, object arg) | |
} | ||
} | ||
|
||
private class MultiExceptionErrorBoundary : AutoRenderComponent, IErrorBoundary | ||
{ | ||
public Exception LastReceivedException { get; private set; } | ||
public int ExceptionCount { get; private set; } | ||
|
||
[Parameter] public RenderFragment ChildContent { get; set; } | ||
|
||
protected override void BuildRenderTree(RenderTreeBuilder builder) | ||
{ | ||
if (LastReceivedException is not null) | ||
{ | ||
builder.AddContent(0, $"Error: {LastReceivedException.Message}"); | ||
} | ||
else | ||
{ | ||
ChildContent(builder); | ||
} | ||
} | ||
|
||
public void HandleException(Exception error) | ||
{ | ||
LastReceivedException = error; | ||
ExceptionCount++; | ||
TriggerRender(); | ||
} | ||
} | ||
|
||
private class CallbackDuringSetParametersAsyncComponent : AutoRenderComponent | ||
{ | ||
public int RenderCount { get; private set; } | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
@{ | ||
throw new Exception("error2"); | ||
} | ||
|
||
@code | ||
{ | ||
protected override async Task OnInitializedAsync() | ||
{ | ||
await Task.Yield(); // Make it actually async | ||
throw new Exception("error"); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot for each test that we added to this file, we need to add the same test but the component must be wrapped inside an error boundary and we need to make the component unconditionally throw in BuildRenderTree. We must check that the boundary always renders the error output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added comprehensive ErrorBoundary tests for all the ComponentBase lifecycle methods as requested. Each test corresponds to a StateHasChanged tracking test but wraps the component in an ErrorBoundary and uses a component that unconditionally throws in BuildRenderTree.
The tests validate that the ErrorBoundary correctly catches rendering exceptions and renders error content for all the different lifecycle patterns:
All 18 ErrorBoundary tests have been added and compile successfully. Commit db09c6b.