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

Handling errors in ComponentBase.InvokeAsync #27716

Closed
poke opened this issue Nov 11, 2020 · 7 comments
Closed

Handling errors in ComponentBase.InvokeAsync #27716

poke opened this issue Nov 11, 2020 · 7 comments
Labels
area-blazor Includes: Blazor, Razor Components ✔️ Resolution: Answered Resolved because the question asked by the original author has been answered. question Status: Resolved

Comments

@poke
Copy link
Contributor

poke commented Nov 11, 2020

Up until now, I was under the impression that I could use ComponentBase.InvokeAsync with server-side Blazor to lift some asynchronous work back onto the renderer. I use this primarily in cases where I have a C# event, which is synchronous, and I want to then do something asynchronously to update the component. For example:

@implements IDisposable
@inject SomeService someService

<ul>
  @if (isLoading)
  {
    <li>Loading…</li>
  }
  else
  {
    @foreach (var item in items)
    {
      <li>@item.Title</li>
    }
  }
</ul>

@code {
    bool isLoading = true;
    IList<Item> items;

    protected override void OnInitialized()
    {
        someService.ItemsChanged += HandleItemsChanged;
        isLoading = true;
    }

    public void Dispose()
    {
        someService.ItemsChanged -= HandleItemsChanged;
    }

    protected override async Task OnAfterRenderAsync(bool firstRender)
    {
        if (firstRender)
        {
            items = await someService.LoadItems();
            isLoading = false;
            StateHasChanged();
        }
    }

    HandleItemsChanged(object sender, EventArgs e)
    {
        InvokeAsync(async () =>
        {
            isLoading = true;
            StateHasChanged();

            items = await someService.LoadItems();
            isLoading = false;
            StateHasChanged();
        });
    }
}

This pattern has been working really well for me so far. I can load the data initially when the component is rendered, and when the underlying data is updated, the event causes the component to refresh its view.

Now, the LoadItems() is pretty safe and will rarely fail. In the rare case there is an exception, I am fine with the server-side Blazor application crashing. I have designed the error dialog for this purpose so users know that something critical happened and that they need to refresh the page to retry. This may not be the most graceful way to handle problems but it works pretty well in this case and does not require handling exceptions all over the place.

However, I now realized that exceptions that are thrown within the InvokeAsync action are not actually bubbling up to the renderer.

While the lifecycle methods are run directly through the renderer by calling AddToPendingTasks which will then handle errors in GetErrorHandledTask, it appears that call InvokeAsync will just run the action through the dispatcher which will eventually just return the task without doing any error handling.

So I am basically in an async void scenario without noticing it where exceptions thrown within InvokeAsync disappear into the void. – Ouch.

Is there any good approach on how to do global error handling for situations like this in server-side Blazor? I am not really looking forward to adding custom error handling to all my components. So if there was a way to trigger the default unhandled error experience, I would be very happy if I could just slap that around all InvokeAsync calls and keep the error behavior as I intended. Unfortunately, TryNotifyClientErrorAsync is not accessible from the outside and the other methods like DispatchEvent do more than I would like to do here.

@javiercn
Copy link
Member

javiercn commented Nov 11, 2020

@poke thanks for the detailed explanation.

I don't remember the exact details, but here are a couple of thoughts:

  • I believe the exception should be raised in the synchronization context and is available on the return task from InvokeAsync.
  • One thing you can do is store the exception and always call state has changed and throw at that point (it's not pretty but it'll likely work).
  • I believe the RendererSynchronizationContext should handle this, but I don't remember the exact details.
    • In this case, declaring the method as async void might actually help, so I would try that.

@javiercn javiercn added the area-blazor Includes: Blazor, Razor Components label Nov 11, 2020
@poke
Copy link
Contributor Author

poke commented Nov 11, 2020

@javiercn First of all, thanks for the quick response. I have been working on a repro now and tried a few things.

I believe the exception should be raised in the synchronization context and is available on the return task from InvokeAsync.

Yes, that’s correct. I could for example do InvokeAsync(…).ContinueWith(t => { DoSomething(t.Exception) }). My issue is that the exception is not bubbling up to the renderer, causing that standard Blazor error message.

One thing you can do is store the exception and always call state has changed and throw at that point

I don’t think I understand this idea. My ideas so far would have been catching the exceptions in all InvokeAsync calls and having a custom handling for all of them; either per-component, or cascading it up to some root component which will then do the handling in some global way. The problem I have here is that I need to do this for every single InvokeAsync call of which I have quite a few.

In this case, declaring the method as async void might actually help, so I would try that.

So this one is really odd. First of all, I still don’t fully understand the synchronization contexts so this is a bit magic to me. I was under the assumption that since the invocation inside the synchronization context is done, the exception could not bubble up since the InvokeAsync call itself happens outside of that component’s context (which is exactly why one would need InvokeAsync in the first place).

So I have tried to replace my call above with an await InvokeAsync(…) inside an async void event handler method. And yes, this indeed changes something! If I raise the event from inside a Blazor context, e.g. after pressing a button, then this indeed gives me the correct behavior. The exception bubbles up to the renderer and the typical unhandled error message shows.

However, and this is where it gets really bad, when I raise the event from somewhere else, for example a different HTTP request, then this uncaught exception causes a stack trace like this and actually crashes the application. Both Kestrel and IIS Express are impacted:

Unhandled exception. System.Exception: Load items failed
   at ExampleApp.SomeService.LoadItems() in C:\…\SomeService.cs:line 22
   at ExampleApp.Pages.ItemList.<HandleItemsChanged>b__6_0() in C:\…\Pages\ItemList.razor:line 51
   at Microsoft.AspNetCore.Components.Rendering.RendererSynchronizationContext.<>c.<<InvokeAsync>b__9_0>d.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at ExampleApp.Pages.ItemList.HandleItemsChanged(Object sender, EventArgs e) in C:\…\Pages\ItemList.razor:line 46
   at System.Threading.Tasks.Task.<>c.<ThrowAsync>b__139_1(Object state)
   at System.Threading.QueueUserWorkItemCallback.<>c.<.cctor>b__6_0(QueueUserWorkItemCallback quwi)
   at System.Threading.ExecutionContext.RunForThreadPoolUnsafe[TState](ExecutionContext executionContext, Action`1 callback, TState& state)
   at System.Threading.QueueUserWorkItemCallback.Execute()
   at System.Threading.ThreadPoolWorkQueue.Dispatch()

So this is actually worse than before where exception was just swallowed and the circuit resulted in an invalid (but still connected) state. This crashes the application completely which might even indicate a bug (or I’m doing something super dumb here).


The repro is over at https://github.com/poke/aspnet-blazor-issue27716. You can just clone and dotnet run it. Once you open the app, there are three options in the navigation:

  • Item list: To open the list component, which is implemented exactly like I showed above, just now with async void
  • Add item: To add an item to the underlying list, raising the change event.
  • Disable/Enable exception: To toggle whether or not to throw an exception while loading the list (in the LoadItems() method).

If you use it just like that, it works exactly how I would like to do: You can view the list and add items via the buttons. The list will automatically reload. If you enable the exception and then add an item, the list will also reload but fail and the circuit will disconnect. This is what wasn’t working before making it async void!

But now there’s also one more thing in this app, which is an endpoint at https://localhost:5001/add which adds an item via a custom route handler. It calls exactly the same method as the button but does so outside of the Blazor context. When you do so, without the enabled exception, then all is well. However, if you now enable the exception and then make the request to that endpoint, the list will start to reload and then the whole server app will crash.

I hope I am doing something really stupid here because being able to crash the ASP.NET Core app doesn’t sound like something you should be able to do from a Blazor server component 😨

@javiercn
Copy link
Member

@poke thanks for the additional details and the repro.

I took a look at it and the behavior is by design. The problem is that the exception is being raised from "outside" the Blazor synchronization context, and you can't "invokeasync" on to it when the context is not setup, so you end up with a background thread that throws an exception and that's why the app gets terminated.

In other words, the synchronization context lets you get back to "where you were" but it doesn't let you get back to where you setup the callback "from somewhere else".

My take is that you avoid this pattern all together since it is bound to give you grief and instead wrap the component into a separate component that handles the event notification and passes down a task to the components below as a parameter or as a cascading parameter.

A great example of one of this type of implementation is CascadingAuthenticationState and AuthenticationStateProvider within the codebase. I suggest you check those out on the code base and use them as a basis for your implementation.

@javiercn javiercn added ✔️ Resolution: Answered Resolved because the question asked by the original author has been answered. question labels Nov 17, 2020
@ghost ghost added the Status: Resolved label Nov 17, 2020
@ghost
Copy link

ghost commented Nov 18, 2020

This issue has been resolved and has not had any activity for 1 day. It will be closed for housekeeping purposes.

See our Issue Management Policies for more information.

@ghost ghost closed this as completed Nov 18, 2020
@poke
Copy link
Contributor Author

poke commented Nov 23, 2020

@javiercn Thank you for your answer (and sorry for the late response)!

you can't "invokeasync" on to it when the context is not setup

Hm, guess I misunderstood this then. I was hoping that InvokeAsync would be moving the execution into the Blazor context, similar to how dispatching a task onto the UI dispatcher e.g. in WPF would work.

My take is that you avoid this pattern all together since it is bound to give you grief

That’s a real shame though since I found events to be a very natural way to do this, and I was actually happy to get back some use case for them considering that they have mostly disappeared elsewhere. I guess, if I were to do error handling on each call site, I would probably be fine continuing this approach too.

I’ve took another close look at how the authentication state handles this and it appears that it mostly solves this by moving the actual awaiting of the task into the component rendering process (either within the lifecycle events or within the Razor code itself). That way, the error is thrown within the context and exceptions will bubble up with the default error handling.

But I think that the fact that it’s a cascading parameter does not directly have an impact here, and I actually think that it complicates the situation in my example for various reasons: You will have to wrap everything in another component hierarchy which will cause problems if you use page-based routing since you will either have to make this “state” globally available through the host/app component, or completely change what the target component for the routing is so that you can add this extra layer.
And with the task being passed as a parameter, you no longer have a simple way of detecting when the data has actually changed and when you need to reload and e.g. show a loading indicator. So you might show that loading indicator when some unrelated parameter changed.

I have an example of this in the cascading-parameter branch of my repo. I also implemented a different approach, moving the await into the lifecycle, in the lifecycle branch where I just set a marker in the event handler and trigger re-rendering which will then cause the data loading within the OnAfterRenderAsync.

The latter is likely what I am going to follow with for now since it is the “easiest” way for me to migrate to without having to change the component hierarchy everywhere.

Summing this up, there are still two things that I would wish to change here:

  1. Even with the explanation it still seems very odd to me that I can crash the ASP.NET Core application through an unhandled exception in a Blazor component. In every other place, ASP.NET Core will attempt to recover from unhandled exceptions and prevent a crash.

    This is even more weird to me considering that the dispatcher does have some handling for unhandled exceptions. It just appears that exceptions from within invoked actions are not caught properly. I feel like this should be done in some way.

  2. My main issue with doing error handling myself is that I would have to do custom error handling. If there was a way to relay it back to the default error handling (which will just terminate the circuit) from the outside, that would be perfect. Unfortunately, everything within the rendering stuff is marked as internal with no means to access it from the outside. Having some way to trigger RemoteRenderer.UnhandledException would help a lot.

Anyway, thank you for your response! And I hope you will actually see my reply given the very aggressive issue management (closing after one day of inactivity after I had to wait 6 days too seems a bit harsh 😁).

@javiercn
Copy link
Member

@poke thanks for the additional details.

I'll try to give some final answers just for completeness.

  1. Even with the explanation it still seems very odd to me that I can crash the ASP.NET Core application through an unhandled exception in a Blazor component. In every other place, ASP.NET Core will attempt to recover from unhandled exceptions and prevent a crash.

The exception is thrown from a background thread, so ASP.NET Core request pipeline doesn't get to see the exception and since it's an exception on a background thread, it will crash the host.

I think there is also a misunderstanding on how the sync context works and what is capable of doing. The problem is that InvokeAsync only works when you are calling it from within the synchronization context already. It's goal is to make sure that a callback/continuation executes within the same context it was originally present.

The issue in your case is that it is being called from an API with no context. One possible way to make this work (although it's very hacky and I'm speculating here) would be to capture the synchronization context manually when you are registering the callback and making sure that you set it up/restore afterwards, but I would not go that way, at this point using events is more problematic than the value it brings, IMO.

Nothing will save you now from having to write code for handling these errors, but you can potentially make it a oneliner if you write a couple of extension methods:

public static async Task<T> HandleErrors<T>(this Task<T> invocation)
{
    try
    {
        await invocation();
    }
    catch(Exception e)
   {
      ...
   }
}

And then use it with

    HandleItemsChanged(object sender, EventArgs e)
    {
        InvokeAsync(async () =>
        {
            isLoading = true;
            StateHasChanged();

            items = await someService.LoadItems();
            isLoading = false;
            StateHasChanged();
        })
        .HandleErrors();
    }

(Just an option/suggestion)

2. My main issue with doing error handling myself is that I would have to do custom error handling. If there was a way to relay it back to the default error handling (which will just terminate the circuit) from the outside, that would be perfect. Unfortunately, everything within the rendering stuff is marked as internal with no means to access it from the outside. Having some way to trigger RemoteRenderer.UnhandledException would help a lot.

We are looking into ways we can make this better, allowing for errors to be dispatched to the renderer in thesescenarios, but that won't happen until our next release.

Anyway, thank you for your response! And I hope you will actually see my reply given the very aggressive issue management (closing after one day of inactivity after I had to wait 6 days too seems a bit harsh 😁).

It's hard some time, but we need to balance giving people time to answer with our ability to efficiently triage the backlog, otherwise it rapidly becomes unmanageable. That said, when an issue is closed its not the end of it, if people come back in a few days we are happy to reopen the issue and continue the discussion if necessary 😃.

@poke
Copy link
Contributor Author

poke commented Nov 23, 2020

@javiercn Thanks again for your fast reply 😊

I think there is also a misunderstanding on how the sync context works and what is capable of doing.

Yes, that’s absolutely true and I’m working on fixing that ASAP 😄 Your clarification did help me though so thank you for the additional details! You are also absolutely righ that an exception from a background thread would crash the app. I guess my confusion comes from the fact that this background thread is also a thread that is currently handling a HTTP request and as such would be “protected” by the middleware pipeline. But with this all being async and over a synchronization context, I guess that this still makes it a normal background thread for the Blazor app and as such implies all the usual caveats.

One possible way to make this work […]

I’ve already started on reworking my app to load data only within the lifecycle methods, so that should probably make everything work better without taking too long to write everything 😅

RE that extension method: My main problem would be exactly that error handling within that catch for which I would have to provide some UI handling then. And doing that in a globally available way from tens of components would be a bit messy.

We are looking into ways we can make this better, allowing for errors to be dispatched to the renderer in these scenarios, but that won't happen until our next release.

If you are looking into this, then that’s already all I am asking for! Having a way to trigger this in the future would be great!

And don’t worry about the issue management. I totally understand why it’s necessary, and it probably doesn’t even prevent these processes from still being very painful for you as maintainers. – You did see my reply and even replied back, so all is well! ☺️

Again, thank you very much for your responses! It helped me a lot and I think I have a good idea now on how to prevent error from disappearing silently and/or crashing my app 😁

@ghost ghost locked as resolved and limited conversation to collaborators Dec 23, 2020
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Includes: Blazor, Razor Components ✔️ Resolution: Answered Resolved because the question asked by the original author has been answered. question Status: Resolved
Projects
None yet
Development

No branches or pull requests

2 participants