Improve Components error handling #7165

pranavkm · 2019-01-31T20:15:20Z

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

Fixes #4964

src/Components/Blazor/Blazor/src/Rendering/WebAssemblyRenderer.cs

src/Components/Components/src/ComponentBase.cs

src/Components/Components/src/Rendering/ComponentState.cs

src/Components/Components/src/Rendering/Renderer.cs

src/Components/Components/src/ComponentBase.cs

src/Components/Components/test/RendererTest.cs

src/Components/Server/src/DependencyInjection/RazorComponentsServiceCollectionExtensions.cs

src/Components/Server/src/Microsoft.AspNetCore.Components.Server.csproj

src/Components/Components/src/Rendering/Renderer.cs

src/Components/Components/src/ComponentBase.cs

src/Components/Components/src/Rendering/Renderer.cs

pranavkm · 2019-02-04T19:52:39Z

I addressed changes to the sources, but I'm going to hold off on test changes (which currently don't compile) until we resolve https://github.com/aspnet/AspNetCore/pull/7240/files. There's a @javiercn's PR sets up the tests correctly, and I would need to rebase my changes on top of it to make the correct assertions.

SteveSandersonMS · 2019-02-05T15:20:27Z

@pranavkm OK, sounds good. Please let us know when this is ready for a final review.

pranavkm · 2019-02-09T01:48:56Z

🆙 📅

src/Components/Blazor/Blazor/src/Rendering/WebAssemblyRenderer.cs

src/Components/Components/src/Rendering/Renderer.cs

javiercn · 2019-02-11T10:25:50Z

src/Components/Components/src/Rendering/Renderer.cs

                    // waiting for it to throw later. This can happen if the task is produced by
                    // an 'async' state machine (the ones generated using async/await) where even
                    // the synchronous exceptions will get captured and converted into a faulted
                    // task.
-                    ExceptionDispatchInfo.Capture(task.Exception.InnerException).Throw();
+                    HandleException(task.Exception.GetBaseException());


Can you explain to me the difference between these two? I thought you had to do ExceptionDispatchInfo.Capture to preserve the original callstack

You're right ExceptionDispatchInfo.Capture(...).Throw() is useful when you want to rethrow preserving the stack trace. In the regular case, HandleException logs the error so we don't need to wrap it up here. HtmlRenderer continues re-throwing and consequently uses ExceptionDispatchInfo. AggregateException.GetBaseException unwraps nested instances of AggregateExceptions - https://github.com/dotnet/corefx/blob/master/src/Common/src/CoreLib/System/AggregateException.cs#L290-L303. In the most common case where there's exactly one InnerException this should return that value. If the AggregateException was produced by many failing tasks, we should still be able to observe all of them rather than the first (viz task.Exception.InnerException)

src/Components/Components/src/Rendering/Renderer.cs

src/Components/Components/src/Rendering/HtmlRenderer.cs

src/Components/Components/src/ComponentBase.cs

src/Components/Components/src/Rendering/Renderer.cs

SteveSandersonMS

This is looking solid to me.

@javiercn Could you give sign-off before @pranavkm merges? I know you've had some pretty detailed involvement in the final changes here.

pranavkm · 2019-02-13T14:33:56Z

src/Components/Components/src/Rendering/Renderer.cs

@@ -329,8 +272,7 @@ public virtual Task InvokeAsync(Func<Task> workItem)
            // This is for example when we run on a system with a single thread, like WebAssembly.
            if (_dispatcher == null)
            {
-                workItem();
-                return Task.CompletedTask;
+                return workItem();


@javiercn noticed a blazor test fail and this turned out to be the culprit. workItem() returns a Task and we weren't awaiting it as part of InvokeAsync.

pranavkm

🆙 📅

pranavkm · 2019-02-13T14:35:08Z

src/Components/Components/src/Rendering/Renderer.cs

@@ -482,15 +428,45 @@ private void ProcessRenderQueue()
            }
        }

-        private void InvokeRenderCompletedCalls(ArrayRange<RenderTreeDiff> updatedComponents)
+        private Task InvokeRenderCompletedCalls(ArrayRange<RenderTreeDiff> updatedComponents)


Changed this from async void to Task returning which we ignore. This follows the same pattern we would tell users to do, plus avoids any state machine if everything is synchronous.

* 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 · 2019-02-13T20:01:41Z

🆙 📅

javiercn

Great improvement!

rynowak · 2019-02-13T20:06:12Z

src/Components/Components/src/IHandleAfterRender.cs

@@ -11,6 +13,7 @@ public interface IHandleAfterRender
        /// <summary>
        /// Notifies the component that it has been rendered.
        /// </summary>
-        void OnAfterRender();
+        /// <returns>A <see cref="Task"/> that represents the asynchronous event handling operation.</returns>


This looks like a bad copy-paste

I couldn't think of anything else to say here so went with the boiler plate. Suggestions?

asynchronous event handling operation -> asynchronous rendering operation

pranavkm · 2019-02-13T22:23:13Z

Thanks for looking through the PR!

chucker · 2019-02-19T21:37:34Z

Hi,

I saw someone struggle with a dependency injection error in an RC project, where adding an @inject statement to their component caused a blank page to render. No error was logged to the browser console or the VS output window.

Will this PR improve this behavior, or should I file a separate issue?

pranavkm · 2019-02-19T21:41:15Z

A new issue with steps to reproduce the error would be great. That said, this change just went in, so I'd wait for preview3 before filing one.

pranavkm requested review from SteveSandersonMS and javiercn January 31, 2019 20:15

pranavkm commented Jan 31, 2019

View reviewed changes

Eilon added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Jan 31, 2019

SteveSandersonMS reviewed Feb 1, 2019

View reviewed changes

src/Components/Components/src/Rendering/Renderer.cs Outdated Show resolved Hide resolved

SteveSandersonMS reviewed Feb 1, 2019

View reviewed changes

src/Components/Components/src/Rendering/Renderer.cs Outdated Show resolved Hide resolved

SteveSandersonMS reviewed Feb 1, 2019

View reviewed changes

src/Components/Components/src/ComponentBase.cs Outdated Show resolved Hide resolved

SteveSandersonMS reviewed Feb 1, 2019

View reviewed changes

src/Components/Components/src/ComponentBase.cs Outdated Show resolved Hide resolved

SteveSandersonMS reviewed Feb 1, 2019

View reviewed changes

src/Components/Components/src/ComponentBase.cs Show resolved Hide resolved

SteveSandersonMS reviewed Feb 1, 2019

View reviewed changes

javiercn reviewed Feb 1, 2019

View reviewed changes

src/Components/Components/src/ComponentBase.cs Show resolved Hide resolved

javiercn reviewed Feb 1, 2019

View reviewed changes

src/Components/Components/src/ComponentBase.cs Outdated Show resolved Hide resolved

javiercn reviewed Feb 4, 2019

View reviewed changes

src/Components/Components/src/ComponentBase.cs Show resolved Hide resolved

javiercn reviewed Feb 4, 2019

View reviewed changes

src/Components/Components/src/Rendering/Renderer.cs Show resolved Hide resolved

pranavkm force-pushed the prkrishn/error-reporting branch from 73a5c87 to 2910504 Compare February 4, 2019 19:49

pranavkm force-pushed the prkrishn/error-reporting branch from 2910504 to 3768419 Compare February 9, 2019 01:43

pranavkm requested review from jkotalik and Tratcher as code owners February 9, 2019 21:29

javiercn reviewed Feb 11, 2019

View reviewed changes

src/Components/Blazor/Blazor/src/Rendering/WebAssemblyRenderer.cs Outdated Show resolved Hide resolved

javiercn self-assigned this Feb 11, 2019

javiercn reviewed Feb 11, 2019

View reviewed changes

src/Components/Components/src/Rendering/Renderer.cs Outdated Show resolved Hide resolved

javiercn reviewed Feb 11, 2019

View reviewed changes

src/Components/Components/src/Rendering/Renderer.cs Outdated Show resolved Hide resolved

javiercn reviewed Feb 11, 2019

View reviewed changes

src/Components/Components/src/Rendering/HtmlRenderer.cs Show resolved Hide resolved

javiercn reviewed Feb 11, 2019

View reviewed changes

src/Components/Components/src/ComponentBase.cs Outdated Show resolved Hide resolved

javiercn reviewed Feb 11, 2019

View reviewed changes

src/Components/Components/src/ComponentBase.cs Outdated Show resolved Hide resolved

jkotalik removed their request for review February 11, 2019 14:59

javiercn reviewed Feb 13, 2019

View reviewed changes

src/Components/Components/src/ComponentBase.cs Show resolved Hide resolved

SteveSandersonMS reviewed Feb 13, 2019

View reviewed changes

src/Components/Components/src/Rendering/Renderer.cs Outdated Show resolved Hide resolved

SteveSandersonMS approved these changes Feb 13, 2019

View reviewed changes

pranavkm force-pushed the prkrishn/error-reporting branch from bfb0f57 to f1b6f16 Compare February 13, 2019 14:31

pranavkm commented Feb 13, 2019

View reviewed changes

pranavkm added 13 commits February 13, 2019 11:56

Fix broken test

5b04e41

WIP

2851209

Changes per PR comments

b1cdc37

Add test for Task.FromException

e76e699

Undo unnecessary dispatcher change

62db887

FIxup comment

3497f3e

Skipping flaky tests

fe73df2

Changes per PR comments

d73c40f

Fixup formatting

95dc139

More tweaks

57dc18d

Fix test failure

f19a744

Add comment

bfb3386

pranavkm force-pushed the prkrishn/error-reporting branch from f1b6f16 to bfb3386 Compare February 13, 2019 20:01

javiercn approved these changes Feb 13, 2019

View reviewed changes

rynowak reviewed Feb 13, 2019

View reviewed changes

pranavkm merged commit cddbc2e into master Feb 13, 2019

pranavkm deleted the prkrishn/error-reporting branch February 13, 2019 22:22

chucker mentioned this pull request Feb 19, 2019

Developer Exception Page displaying white page #3301

Closed

chucker mentioned this pull request Feb 20, 2019

[Components, Blazor] Better error handling when setting up the DI container #7739

Closed

Improve Components error handling #7165

Improve Components error handling #7165

Uh oh!

Conversation

pranavkm commented Jan 31, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pranavkm commented Feb 4, 2019

Uh oh!

SteveSandersonMS commented Feb 5, 2019

Uh oh!

pranavkm commented Feb 9, 2019

Uh oh!

Uh oh!

Uh oh!

javiercn Feb 11, 2019

Choose a reason for hiding this comment

Uh oh!

pranavkm Feb 12, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SteveSandersonMS left a comment

Choose a reason for hiding this comment

Uh oh!

pranavkm Feb 13, 2019

Choose a reason for hiding this comment

Uh oh!

pranavkm left a comment

Choose a reason for hiding this comment

Uh oh!

pranavkm Feb 13, 2019

Choose a reason for hiding this comment

Uh oh!

pranavkm commented Feb 13, 2019

Uh oh!

javiercn left a comment

Choose a reason for hiding this comment

Uh oh!

rynowak Feb 13, 2019

Choose a reason for hiding this comment

Uh oh!

pranavkm Feb 13, 2019

Choose a reason for hiding this comment

Uh oh!

rynowak Feb 13, 2019

Choose a reason for hiding this comment

Uh oh!

pranavkm commented Feb 13, 2019

Uh oh!

chucker commented Feb 19, 2019

Uh oh!

pranavkm commented Feb 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

pranavkm commented Feb 19, 2019 •

edited

Loading