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 for dispatching to renderer sync context and capturing exceptions #44920

Closed
SteveSandersonMS opened this issue Nov 7, 2022 · 14 comments · Fixed by #46074
Closed

API for dispatching to renderer sync context and capturing exceptions #44920

SteveSandersonMS opened this issue Nov 7, 2022 · 14 comments · Fixed by #46074
Assignees
Labels
area-blazor Includes: Blazor, Razor Components enhancement This issue represents an ask for new feature or an enhancement to an existing one Priority:1 Work that is critical for the release, but we could probably ship without
Milestone

Comments

@SteveSandersonMS
Copy link
Member

Currently if you have an exception that occurs off the Blazor sync context, there isn't a good way to marshal it onto the sync context. In our internal code we've sometimes do a weird trick of performing a fake render that throws.

We should have some new API that's like InvokeAsync but instead of returning a Task, it returns void and handles any errors by dispatching them to the renderer. Maybe it could be called Dispatch or similar.

Note that it somehow needs to execute in the context of a particular component, so the error can hit the right error boundary. Ideally it would not be possible to pretend to be some other component - maybe it should rely on having access to a particular component's RenderHandle.

@SteveSandersonMS SteveSandersonMS added the area-blazor Includes: Blazor, Razor Components label Nov 7, 2022
@SteveSandersonMS
Copy link
Member Author

Previous incarnation of this request: #27716

@javiercn
Copy link
Member

javiercn commented Nov 8, 2022

@SteveSandersonMS Would it work if we had a DispatchError function on ComponentBase/RenderHandle?

I am not 100% sure how the Task -> void conversion works as I think they are separate concerns.

@SteveSandersonMS
Copy link
Member Author

I am not 100% sure how the Task -> void conversion works as I think they are separate concerns.

No conversion. I was just proposing some way to run code (which may be async, but not value-returning) within the context of a component. The reason for returning void is because the framework would be handling any exceptions.

Your more constrained proposal of DispatchError might be better. We'd need to think through if there are any legit scenarios where people would want to combine this with InvokeAsync, and in that case what sort of usage patterns would make sense and whether it's better to mix InvokeAsync and DispatchError manually, or whether a single Dispatch would be sufficient and easier to use.

Would it work if we had a DispatchError function on ComponentBase/RenderHandle?

Yeah, pretty sure it would end up being some function on RenderHandle that also gets wrapped by ComponentBase.

@mkArtakMSFT mkArtakMSFT added the enhancement This issue represents an ask for new feature or an enhancement to an existing one label Nov 8, 2022
@mkArtakMSFT mkArtakMSFT added this to the .NET 8 Planning milestone Nov 8, 2022
@ghost
Copy link

ghost commented Nov 8, 2022

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@mkArtakMSFT mkArtakMSFT added the Priority:1 Work that is critical for the release, but we could probably ship without label Nov 15, 2022
@Nick-Stanton
Copy link
Member

Nick-Stanton commented Jan 10, 2023

@javiercn @SteveSandersonMS while looking into this I think it makes sense to have this in RenderHandle even though there isn't any existing invocation logic there (just access to the Dispatcher). This way, _renderer.HandleExceptionViaErrorBoundary() could be leveraged and its call from ComponentBase follows the inline style with the asynchronous invocations. What are your thoughts on this?

EDIT: There isn't correct access for _renderer.HandleExceptionViaErrorBoundary() from the RenderHandle

@SteveSandersonMS
Copy link
Member Author

SteveSandersonMS commented Jan 11, 2023

while looking into this I think it makes sense to have this in RenderHandle

Sounds about right.

EDIT: There isn't correct access for _renderer.HandleExceptionViaErrorBoundary() from the RenderHandle

It looks like we could add an internal method on Renderer like:

internal void HandleComponentException(Exception exception, int componentId)
    => HandleExceptionViaErrorBoundary(exception, GetRequiredComponentState(componentId));

... and call that from the RenderHandle, though I haven't actually tried this to verify.

@javiercn
Copy link
Member

This sounds reasonable.

We might want to put the functionality in the RenderHandle (which internally delegates to the renderer) and expose a helper method on ComponentBase to let people easily raise them.

@Nick-Stanton
Copy link
Member

Using @SteveSandersonMS's internal method, I get this code block in RenderHandle:

    public void Dispatch (Action workItem)
    {
        if (_renderer == null)
        {
            ThrowNotInitialized();
        }

        try
        {
            Dispatcher.InvokeAsync(workItem);
        }
        catch (Exception e)
        {
            _renderer.HandleComponentException(e, _componentId);
        }
    }

Am I understanding the ask correctly?

@SteveSandersonMS
Copy link
Member Author

SteveSandersonMS commented Jan 12, 2023

Close! A few other details:

  • The code that people want to dispatch might itself be async, so the parameter type needs to be Func<Task>.
  • Renderer's HandleExceptionViaErrorBoundary requires the call to be made on the dispatcher's sync context and will throw a different error if not. So, the call into the renderer will need to be inside the dispatched block.
  • No need to assert about _renderer == null because that's checked as part of evaluating Dispatcher.

Maybe something like:

public void Dispatch(Func<Task> workItem)
{
    _ = Dispatcher.InvokeAsync(async () =>
    {
        try
        {
            await workItem();
        }
        catch (Exception e)
        {
            _renderer.HandleComponentException(e, _componentId);
        }
    });
}

Some drawbacks with this whole design:

  • In the case where a developer wants to get back a result from the work item, there isn't a way of doing that.
  • Similarly if they want to know when the async operation is completed, they can't do that either. We could change this to return the Task returned by InvokeAsync, but then the semantics become ambiguous - it would look as if you're still responsible for handline any exceptions since Task in general might be in a faulted state.

An alternative design that's clearer about what's going on, even at the cost of requiring more code to use it in some common cases, would be not having Dispatch at all and instead having a DispatchExceptionAsync like:

public Task DispatchExceptionAsync(Exception exception)
{
    return Dispatcher.InvokeAsync(() => _renderer.HandleComponentException(e, _componentId));
}

This is now purely responsible for getting the exception into the renderer, so it's now up to the developer to combine this with InvokeAsync in whatever way they want, e.g. they can make their own equivalent to Dispatch like this:

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

Now they can get back results from InvokeAsync, and they can choose whether or not to do anything with the Task returned by either InvokeAsync or DispatchExceptionAsync.

What do you think?

@Nick-Stanton
Copy link
Member

The code that people want to dispatch might itself be async, so the parameter type needs to be Func<Task>.

For sure, I think that similar to how InvokeAsync is used in ComponentBase, it would be good to have an Action and Func<Task> overload.

I'm torn but think the second approach may be better since the result still comes back. Is there a way you're leaning with this?

@SteveSandersonMS
Copy link
Member Author

Yes, I would lean towards the second (DispatchExceptionAsync) approach.

@Nick-Stanton
Copy link
Member

return Dispatcher.InvokeAsync(() => _renderer.HandleComponentException(e, _componentId));

Hmm, this lambda expression throws CS1673. Do you know a workaround to this?

@javiercn
Copy link
Member

@Nick-Stanton copy the _renderer into a local variable.

@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
area-blazor Includes: Blazor, Razor Components enhancement This issue represents an ask for new feature or an enhancement to an existing one Priority:1 Work that is critical for the release, but we could probably ship without
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants