Skip to content

ComponentBase needs proper async error reporting #4964

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

Closed
SteveSandersonMS opened this issue Dec 14, 2018 · 4 comments
Closed

ComponentBase needs proper async error reporting #4964

SteveSandersonMS opened this issue Dec 14, 2018 · 4 comments
Assignees
Labels
area-blazor Includes: Blazor, Razor Components Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one

Comments

@SteveSandersonMS
Copy link
Member

SteveSandersonMS commented Dec 14, 2018

Currently, ComponentBase is hard-coded to deal with async lifecycle exceptions just by logging them to the console. This is understandable (albeit limited) in the WebAssembly case, since the developer console is the only place people are going to look for errors anyway, but it's no good for the server-side execution case.

We might want to have an event on ComponentBase that reports async lifecycle task failures. Then the WebAssembly host can set up a listener that just logs to console, whereas the server-side executor can wire it into the ASP.NET logging system.

@SteveSandersonMS SteveSandersonMS added enhancement This issue represents an ask for new feature or an enhancement to an existing one area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates razor-components labels Dec 14, 2018
@SteveSandersonMS SteveSandersonMS added this to the 3.0.0 milestone Dec 14, 2018
@SteveSandersonMS
Copy link
Member Author

Note: when implementing this, be sure to update the unit tests in ComponentBaseTest so that they explicitly check whether any unexpected exceptions are reported and fail if they are. For example, in the tests about cancellation, we need to observe that cancellation of OnInitAsync/OnSetParametersAsync tasks does not get reported as an exception, whereas actual failure of those tasks does get reported as an exception.

@javiercn
Copy link
Member

Now that SetParametersAsync we should not do any specific error handling within there for OnSetParametersAsync and OnInitAsync. We should let those errors flow all the way up to the renderer and let the renderer deal with them as appropriate.

Regarding errors happening from IHandleEvent we could change the interface to:

        Task IHandleEvent.HandleEventAsync(EventHandlerInvoker binding, UIEventArgs args)
        {
            var task = binding.Invoke(args);
            // If there is real async work going on, it adds a call to StateHasChanged() after 
            // the async work is done
            task = ContinueAfterLifecycleTask(task);

            // After each event, we synchronously re-render (unless !ShouldRender())
            // This just saves the developer the trouble of putting "StateHasChanged();"
            // at the end of every event callback.
            StateHasChanged();

            return task;
        }

This way errors can flow all the way to the renderer or other component where they can be handled appropriately in a single place.

@SteveSandersonMS Thoughts?

@SteveSandersonMS
Copy link
Member Author

@javiercn

Now that SetParametersAsync we should not do any specific error handling within there for OnSetParametersAsync and OnInitAsync. We should let those errors flow all the way up to the renderer and let the renderer deal with them as appropriate.

That sounds completely reasonable.

Regarding errors happening from IHandleEvent we could change the interface to...

It just so happens that @rynowak was contemplating this last week and started circling around a refinement of the IHandleEvent design. I'm not certain his design reached a conclusion, but when he gets back to looking at it, hopefully it can be made to line up with the requirements for error reporting.

@javiercn
Copy link
Member

@SteveSandersonMS For what is worth I'm in support of my proposal 😄. I think it's better if we handle exceptions in one place and the renderer seems to be the best place for it (as it allows for reporting errors to the appropriate environment).

@pranavkm pranavkm modified the milestones: 3.0.0, 3.0.0-preview3 Jan 30, 2019
pranavkm added a commit that referenced this issue Jan 31, 2019
* Change event handlers IHandleEvent, IHandleAfterEvent to be async.
* Return faulted tasks to Renderer instead of handling exceptions in ComponentBase
* Use ILogger in RemoteRenderer, and log to console in WebAssemblyRenderer
* Cleaning up touched files

Fixes #4964
pranavkm added a commit that referenced this issue Feb 4, 2019
* Change event handlers IHandleEvent, IHandleAfterEvent to be async.
* Return faulted tasks to Renderer instead of handling exceptions in ComponentBase
* Use ILogger in RemoteRenderer, and log to console in WebAssemblyRenderer
* Cleaning up touched files

Fixes #4964
@SteveSandersonMS SteveSandersonMS added the area-blazor Includes: Blazor, Razor Components label Feb 6, 2019
pranavkm added a commit that referenced this issue Feb 9, 2019
* Change event handlers IHandleEvent, IHandleAfterEvent to be async.
* Return faulted tasks to Renderer instead of handling exceptions in ComponentBase
* Use ILogger in RemoteRenderer, and log to console in WebAssemblyRenderer
* Cleaning up touched files

Fixes #4964
pranavkm added a commit that referenced this issue Feb 13, 2019
* Change event handlers IHandleEvent, IHandleAfterEvent to be async.
* Return faulted tasks to Renderer instead of handling exceptions in ComponentBase
* Use ILogger in RemoteRenderer, and log to console in WebAssemblyRenderer
* Cleaning up touched files

Fixes #4964
pranavkm added a commit that referenced this issue Feb 13, 2019
* Change event handlers IHandleEvent, IHandleAfterEvent to be async.
* Return faulted tasks to Renderer instead of handling exceptions in ComponentBase
* Use ILogger in RemoteRenderer, and log to console in WebAssemblyRenderer
* Cleaning up touched files

Fixes #4964
pranavkm added a commit that referenced this issue Feb 13, 2019
* Improve Components error handling

* Change event handlers IHandleEvent, IHandleAfterEvent to be async.
* Return faulted tasks to Renderer instead of handling exceptions in ComponentBase
* Use ILogger in RemoteRenderer, and log to console in WebAssemblyRenderer
* Cleaning up touched files

Fixes #4964
@pranavkm pranavkm added Done This issue has been fixed and removed 2 - Working labels Feb 13, 2019
@mkArtakMSFT mkArtakMSFT removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels May 9, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Dec 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Includes: Blazor, Razor Components Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one
Projects
None yet
Development

No branches or pull requests

4 participants