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

Add DispatchExceptionAsync to ComponentBase #46074

Merged
merged 9 commits into from
Jan 21, 2023

Conversation

Nick-Stanton
Copy link
Member

Add DispatchExceptionAsync to ComponentBase

Adds way for developers to dispatch exceptions outside of the Blazor sync context to the renderer.

Description

DispatchExceptionAsync sends exceptions from the ComponentBase to the Renderer via the RenderHandle. See below proposal for more info.

Fixes #44920
API Proposal: #46068

@Nick-Stanton Nick-Stanton added area-blazor Includes: Blazor, Razor Components feature-rendering Features dealing with how blazor renders components labels Jan 13, 2023
@Nick-Stanton Nick-Stanton self-assigned this Jan 13, 2023
@Nick-Stanton
Copy link
Member Author

I've been playing with Selenium/E2E testing for a few days and this appears to be the cleanest way to test this scenario if I'm understanding it correctly

@Nick-Stanton Nick-Stanton marked this pull request as ready for review January 19, 2023 01:20
@Nick-Stanton Nick-Stanton requested a review from a team as a code owner January 19, 2023 01:20
Copy link
Member

@SteveSandersonMS SteveSandersonMS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice!

}
}

private Task ThrowExceptionAsync()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not throwing the exception asynchronously. The thread is not yielded at any point, so this is similar/equivalent to the SyncExceptionDispatch case.

For the exception to be thrown asynchronously you need to use something like await Task.Yield() before throwing the exception.

Also, I will point out that we normally also write unit tests for these types of behavior in the Renderer itself. There we normally use a TaskCompletionSource to very accurately control the process.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point.

In fact more generally I think these test cases are missing the key scenario: when an exception happens outside the normal lifecycle flow and you couldn't just do a normal throw anyway. With the way SyncExceptionDispatch and AsyncExceptionDispatch are structured, DispatchExceptionAsync is no different from simply using throw at that point, because we're still within the task chain of the lifecycle method.

It would be good to cover the primary use case where this new feature is what causes the exception to be associated with the renderer. For example, by using Task.Run to simulate having some code running completely separately from the renderer callstack/task chain:

    void ExternalExceptionDispatch()
    {
        Task.Run(() =>
        {
            // Inside Task.Run, we're outside the call stack or task chain of the lifecycle method, so
            // DispatchExceptionAsync is needed to get an exception back into the component
            _ = DispatchExceptionAsync(new InvalidTimeZoneException());
        });
    }

If we are going to have the SyncExceptionDispatch/AsyncExceptionDispatch cases, I imagine they would be there to cover the case where people are calling DispatchExceptionAsync even though they didn't need to (i.e., calls that are already on the lifecycle method call stack), so they could be simplified to just dispatching a newed-up exception like:

    async Task SyncExceptionDispatch()
    {
        await DispatchExceptionAsync(new InvalidTimeZoneException("Sync"));
    }

    async Task AsyncExceptionDispatch()
    {
        await Task.Yield();
        await DispatchExceptionAsync(new InvalidTimeZoneException("Async"));
    }

@Nick-Stanton Nick-Stanton linked an issue Jan 20, 2023 that may be closed by this pull request
@Nick-Stanton
Copy link
Member Author

@SteveSandersonMS @javiercn
During API review I could not think of a reason to justify RenderHandle.DipatchExceptionAsync to be public, so it's been changed to internal.

I should have test(s) for the new, correct testing scenario that you guys described completed soon.

@SteveSandersonMS
Copy link
Member

SteveSandersonMS commented Jan 20, 2023

During API review I could not think of a reason to justify RenderHandle.DipatchExceptionAsync to be public, so it's been changed to internal.

I totally understand that it's nonobvious, but RenderHandle.DispatchExceptionAsync should be public so it's usable by components that implement IComponent directly without inheriting from ComponentBase.

For more general context, ComponentBase is a set of opinionated patterns for components (e.g., having lifecycle methods) but isn't required - it's just the default used by the Razor compiler. Developers who want different patterns are free to implement IComponent and interact with the renderer using more low-level methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components feature-rendering Features dealing with how blazor renders components
Projects
None yet
4 participants