dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.15k stars 9.92k forks source link

AsNonRenderingEventHandler + ErrorBoundary = unexpected behavior #54543

Closed ScarletKuro closed 3 weeks ago

ScarletKuro commented 5 months ago

Is there an existing issue for this?

Describe the bug

Hello.

In MudBlazor we recently did this change: https://github.com/MudBlazor/MudBlazor/pull/8203/files#diff-7258903903d30bdc973979ca0d64c61adbee3f4441e47749100907f522060a47 We added AsNonRenderingEventHandler (that was copied from official MS document) to our internal onclick and now the ErrorBoundary doesn't catch the exceptions that are fired within the AsNonRenderingEventHandler event:

<ErrorBoundary>
    <ChildContent>
        <MudButton OnClick="ProcessSomething" Variant="Variant.Filled" Color="Color.Primary">
            <MudText>Click me</MudText>
        </MudButton>
    </ChildContent>
    <ErrorContent Context="exc">
      <MudAlert Severity="Severity.Error">Exception: @exc.Message</MudAlert>
    </ErrorContent>
</ErrorBoundary>
@code {
    async Task ProcessSomething()
    {
        throw new ApplicationException("TESTING");
    }
}

You will see in console:

crit: Microsoft.AspNetCore.Components.WebAssembly.Rendering.WebAssemblyRenderer[100]
      Unhandled exception rendering component: TESTING
System.ApplicationException: TESTING
   at Try.UserComponents.__Main.ProcessSomething()
   at Microsoft.AspNetCore.Components.ComponentBase.CallStateHasChangedOnAsyncCompletion(Task task)
   at MudBlazor.MudBaseButton.OnClickHandler(MouseEventArgs ev)
   at Microsoft.AspNetCore.Components.RenderTree.Renderer.GetErrorHandledTask(Task taskToHandle, ComponentState owningComponentState)

rather than the ErrorContent.

The weird thing about it if that I use alternative to AsNonRenderingEventHandler:

public abstract class MudBaseButton : MudComponentBase, IHandleEvent
{
    Task IHandleEvent.HandleEventAsync(EventCallbackWorkItem callback, object? arg) => callback.InvokeAsync(arg);
}

Then everything will work as expected and ErrorBoundary is working another weird thing is that if I change this:

public static Func<TValue, Task> AsNonRenderingEventHandler<TValue>(Func<TValue, Task> callback) => new AsyncReceiver<TValue>(callback).Invoke;

to this:

public static Func<TValue, Task> AsNonRenderingEventHandler<TValue>(Func<TValue, Task> callback)
{
    var receiver = new AsyncReceiver<TValue>(callback);
    return async value => await receiver.Invoke(value);
}

Then the ErrorBoundary will catch the exception as well, but the problem is that this return async value => await receiver.Invoke(value); and this return new AsyncReceiver<TValue>(callback).Invoke should be EQUIVALENT in C# (except that for first one an asyncstatemachine is created, but end result should be the same and no side effects).

upd: well, with this snippet https://try.mudblazor.com/snippet/cOQSERlIheRAyAJe I just found out that async value => await receiver.Invoke(value) doesn't prevents re-rendering, but the IHandleEvent implementation in component works, so this doesn't explain few moments: 1) Why with the original EventUtil from docs the ErrorBoundary doesn't work. 2) Why with my offered version the exception flows, but the re-render is not prevented. 3) Why if I implement IHandleEvent in the component it works great - the exception flows and no re-render is happening, but the EventUtil in 1. or 2. doesn't work as I expect?

I described the same in this in MudBlazor issue: https://github.com/MudBlazor/MudBlazor/issues/8365#issuecomment-1997916812

@SteveSandersonMS could you please explain, as I'm very confused on what's going on.

Expected Behavior

ErrorBoundary to work with AsNonRenderingEventHandler

Steps To Reproduce

This would require to fork the https://github.com/MudBlazor/MudBlazor repository and play with the MudButton.razor and MudButtonBase and the snippet from https://try.mudblazor.com/snippet/caQSaxvSoRhvMaSP (added very minimal repro below, so not relevant anymore)

upd: Minimal reproduction without MudBlazor: BlazorEventUtilsIssue.zip

Exceptions (if any)

crit: Microsoft.AspNetCore.Components.WebAssembly.Rendering.WebAssemblyRenderer[100]
      Unhandled exception rendering component: TESTING
System.ApplicationException: TESTING
   at Try.UserComponents.__Main.ProcessSomething()
   at Microsoft.AspNetCore.Components.ComponentBase.CallStateHasChangedOnAsyncCompletion(Task task)
   at MudBlazor.MudBaseButton.OnClickHandler(MouseEventArgs ev)
   at Microsoft.AspNetCore.Components.RenderTree.Renderer.GetErrorHandledTask(Task taskToHandle, ComponentState owningComponentState)

.NET Version

8.0.103

Anything else?

No response

ScarletKuro commented 5 months ago

Added minimal reproduction project to "Steps To Reproduce" without MudBlazor.

danielchalmers commented 5 months ago

@mkArtakMSFT @SteveSandersonMS This is disruptive because Blazor has a big visual bug during navigation (#53863) caused by extra renders, but marking it non-rendering isn't compatible with ErrorBoundary and this is causing problems for several users in MudBlazor. Do we have any other viable options to work around this in .NET 8? I imagine it's also bad for performance to have the extra renders.

ScarletKuro commented 3 months ago

Another thing I tried:

public static Func<TValue, Task> AsNonRenderingEventHandler<TValue>(Func<TValue, Task> callback)
{
    var receiver = new AsyncReceiver<TValue>(callback);
    return value =>
    {
        var callbackWorkItem = new EventCallbackWorkItem(receiver.Invoke);
        return ((IHandleEvent)receiver).HandleEventAsync(callbackWorkItem, value);
    };
}

The exception flows, but the re-render occurs. (and not like it's possible to use this for the Action versions)

Then I tried just a plain class:

public class NoRender<TValue> : IHandleEvent
{
    private readonly Func<TValue, Task> _callback;

    public NoRender(Func<TValue, Task> callback)
    {
        _callback = callback;
    }

    public Task Invoke(TValue arg) => _callback(arg);

    public Task HandleEventAsync(EventCallbackWorkItem item, object? arg) => item.InvokeAsync(arg);
}

@onclick="new NoRender<MouseEventArgs>(OnClickHandler).Invoke"

But the exception doesn't flow.

I also tried to play with ExecutionContext restore, but this gave no result either.

This is very broken, and I give up on trying to find the solution myself. Only implementing IHandleEvent on a component gives the desired result: no render and exception flow in ErrorBoundary, but this disables the re-render globally for the given component, whereas I want to disable it for a specific EventCallback.

MackinnonBuck commented 3 weeks ago

Thanks for reaching out, @ScarletKuro.

When an event handler is created from a delegate, the event's receiver is the delegate's Target, if it implements IHandleEvent. If the Target is not an IHandleEvent, then the receiver is the component instance.

An event's receiver is used within the framework when determining how to handle exceptions thrown from within that event. If the receiver is an IComponent, then the renderer walks up the component hierarchy to find the first ancestor IErrorBoundary to report the exception to. If the receiver is not an IComponent (as would be the case with AsNonRenderingEventHandler), then the renderer has no idea which error boundary the exception should be associated with.

However, another way to dispatch an exception to an error boundary is via ComponentBase.DispatchExceptionAsync. So, we can update our custom IHandleEvent implementation to catch exceptions thrown by the event and dispatch them to the associated component.

Here's an example that achieves this by defining a custom base component type:

public abstract class CustomComponent : ComponentBase
{
    private protected Action AsNonRenderingEventHandler(Action callback)
        => new SyncReceiver(this, callback).Invoke;

    private protected Action<TValue> AsNonRenderingEventHandler<TValue>(
            Action<TValue> callback)
        => new SyncReceiver<TValue>(this, callback).Invoke;

    private protected Func<Task> AsNonRenderingEventHandler(Func<Task> callback)
        => new AsyncReceiver(this, callback).Invoke;

    private protected Func<TValue, Task> AsNonRenderingEventHandler<TValue>(
            Func<TValue, Task> callback)
        => new AsyncReceiver<TValue>(this, callback).Invoke;

    private sealed class SyncReceiver(CustomComponent component, Action callback) : ReceiverBase(component)
    {
        public void Invoke() => callback();
    }

    private sealed class SyncReceiver<T>(CustomComponent component, Action<T> callback) : ReceiverBase(component)
    {
        public void Invoke(T arg) => callback(arg);
    }

    private sealed class AsyncReceiver(CustomComponent component, Func<Task> callback) : ReceiverBase(component)
    {
        public Task Invoke() => callback();
    }

    private sealed class AsyncReceiver<T>(CustomComponent component, Func<T, Task> callback) : ReceiverBase(component)
    {
        public Task Invoke(T arg) => callback(arg);
    }

    private abstract class ReceiverBase(CustomComponent component) : IHandleEvent
    {
        public async Task HandleEventAsync(EventCallbackWorkItem item, object arg)
        {
            try
            {
                await item.InvokeAsync(arg);
            }
            catch (Exception ex)
            {
                await component.DispatchExceptionAsync(ex);
            }
        }
    }
}

...but you could also achieve this by following the EventUtil pattern if you have a way to access the component's RenderHandle (or call its DispatchExceptionAsync method, which is protected in ComponentBase).

Hope this answers your questions! Please let us know if you think there's a framework issue that needs to be addressed. If not, we'll close this out.

ScarletKuro commented 3 weeks ago

@MackinnonBuck thanks. Your code seems to work, I didn't find any issues with it. The exception flows to the ErrorBoundary and prevents re-rendering. The only problem is that DispatchExceptionAsync doesn't exist in .NET 7, and we still support it in MudBlazor for now. If there are no other suggestions, feel free to close the issue.

Also, it would be helpful if the documentation at https://learn.microsoft.com/en-us/aspnet/core/blazor/performance?view=aspnetcore-8.0#avoid-rerendering-after-handling-events-without-state-changes could be updated to mention that the current implementation doesn't flow exceptions. If developers want this behavior, they should call DispatchExceptionAsync in the IHandleEvent implementation.

MackinnonBuck commented 3 weeks ago

Thanks @ScarletKuro, glad to hear it works. Yeah, unfortunately these APIs didn't exist in .NET 7. There might be a creative way to reimplement IHandleEvent in a custom base component class that conditionally re-renders after the event (maybe based on the value of an AsyncLocal<bool> that gets set in the event callback?).

@guardrex, could we add a note in the docs that the example doesn't flow exceptions through to error boundaries, but you can call ComponentBase.DispatchExceptionAsync() if you do want support them? I'm not sure if it's worth adding the entire example above in the docs. Maybe we could link to my GitHub comment if anything (if that's okay to do in docs). Your call! :)

ScarletKuro commented 3 weeks ago

Docs were updated, thanks everyone.