Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[API Proposal] Allow Dispatching to Blazor Renderer Sync Context #46068

Closed
Nick-Stanton opened this issue Jan 12, 2023 · 5 comments · Fixed by #46074
Closed

[API Proposal] Allow Dispatching to Blazor Renderer Sync Context #46068

Nick-Stanton opened this issue Jan 12, 2023 · 5 comments · Fixed by #46074
Labels
api-approved API was approved in API review, it can be implemented area-blazor Includes: Blazor, Razor Components
Milestone

Comments

@Nick-Stanton
Copy link
Member

Background and Motivation

Currently, there is not an easy or clean way to marshal exceptions occurring off the Blazor sync context back onto it. The following API would allow developers to directly dispatch exceptions to the renderer.

Fix for #44920

Related community requests: #44871, #27716

Proposed API

namespace Microsoft.AspNetCore.Components;

public abstract class ComponentBase : IComponent, IHandleEvent, IHandleAfterRender
{
+    protected Task DispatchExceptionAsync(Exception exception);
}

public readonly struct RenderHandle
{
+    public Task DispatchExceptionAsync(Exception exception);
}

Usage Examples

// Some code in a component
try
{
    await InvokeAsync(async () => { /* some code that may throw, possibly asynchronously */ });
}
catch (Exception e)
{
    await DispatchExceptionAsync(e);
}

Alternative Designs

An alternative would be to have a method Dispatch() that is used like InvokeAsync() but returns void instead of a task. This would require fewer lines of code to use it, but also would not allow the developer to get a result back from their work item or know when the async operation is completed.

Risks

None that I can think of. Does not cause a breaking change, nor should cause a performance regression.

@Nick-Stanton Nick-Stanton added area-blazor Includes: Blazor, Razor Components api-suggestion Early API idea and discussion, it is NOT ready for implementation api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Jan 12, 2023
@ghost
Copy link

ghost commented Jan 12, 2023

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@Nick-Stanton Nick-Stanton removed the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jan 12, 2023
@javiercn javiercn added this to the 8.0-preview1 milestone Jan 16, 2023
@SteveSandersonMS
Copy link
Member

Looks good to me!

@halter73
Copy link
Member

halter73 commented Jan 19, 2023

API Review Notes:

  • Why doesn't InvokeAsync call DispatchExceptionAsync internally? Will this ever be used for exceptions thrown from something other than InvokeAsync?
    • InvokeAsync is the primary scenario, but this makes this a non-breaking opt-in change.
  • How are people going to know to call try/catch? Documentation.
  • Is this behavior we even want on every call to InvokeAsync? No. Oftentimes, the error can be handled outside of the renderer.
  • Can RenderHandle.DispatchExceptionAsync be internal? Maybe. Edit: No.
  • Can this be worked around? Maybe using ExceptionDispatchInfo and SynchronizatinContext.Post? Maybe not that workaround, but there are ugly workarounds that we do not want to suggest to users.
  • What does the Task returned by DispatchExceptionAsync represent? When the exception has completed dispatching. So any handlers should have completed handling the error on the UI thread. This should never really throw except maybe an ObjectDisposeException or something.

API Approved!

namespace Microsoft.AspNetCore.Components;

public abstract class ComponentBase : IComponent, IHandleEvent, IHandleAfterRender
{
+    protected Task DispatchExceptionAsync(Exception exception);
}

public readonly struct RenderHandle
{
+    public Task DispatchExceptionAsync(Exception exception);
}

@halter73 halter73 added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Jan 19, 2023
@Nick-Stanton Nick-Stanton linked a pull request Jan 20, 2023 that will close this issue
@SteveSandersonMS
Copy link
Member

Why doesn't InvokeAsync call DispatchExceptionAsync internally? Will this ever be used for exceptions thrown from something other than InvokeAsync?

InvokeAsync returns a task. So if the invoked call throws, the returned task will be in a failed state with this exception. If we didn't do that, there would be no way for the caller to handle the exceptions.

How are people going to know to call try/catch? Documentation.

The question probably indicates that we were not clear about the use case. The point of DispatchExceptionAsync is when you have exceptions that come from some external system (not the Blazor lifecycle methods) and want to treat them the same as lifecycle method exceptions. So, the premise that you have an exception already implies you caught it. If you don't catch an external exception, then you have an unhandled exception which has nothing to do with Blazor.

Can RenderHandle.DispatchExceptionAsync be internal? Maybe.

No, it needs to be usable by components that don't inherit from ComponentBase.

Can this be worked around? Maybe using ExceptionDispatchInfo and SynchronizatinContext.Post? Maybe not that workaround, but there are ugly workarounds that we do not want to suggest to users.

The only existing workaround, which we use internally in a couple of places, is to write some code that causes a component to re-render itself and to rethrow the exception during rendering. That is a nasty hack, so adding DispatchExceptionAsync is intended to give a clean and obvious way to achieve this.

@halter73
Copy link
Member

InvokeAsync returns a task. So if the invoked call throws, the returned task will be in a failed state with this exception. If we didn't do that, there would be no way for the caller to handle the exceptions.

I was thinking it could rethrow, but I did not realize how fatal these dispatched exceptions are. After reading your explanation about this treating the exceptions as equivalent to exceptions in lifecycle methods and then reading https://learn.microsoft.com/en-us/aspnet/core/blazor/fundamentals/handle-errors?view=aspnetcore-7.0#lifecycle-methods, this API makes more sense to me. This is not something you'd want to do by default when the caller to InvokeAsync may be able to handle the exception gracefully.

No, it needs to be usable by components that don't inherit from ComponentBase.

Okay. This API is approved as proposed then. This is somethings we said would be easy to go and add back after API review. I've updated the approval comment, and I'll mention this in the email with the meeting notes.

@dotnet dotnet locked as resolved and limited conversation to collaborators Feb 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-blazor Includes: Blazor, Razor Components
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants