mrpmorris / Fluxor

Fluxor is a zero boilerplate Flux/Redux library for Microsoft .NET and Blazor.
MIT License
1.24k stars 141 forks source link

TaskCancelledException's are swallowed #293

Closed uhfath closed 2 years ago

uhfath commented 2 years ago

A small repro:

  1. navigate to Counter page;
  2. open development console;
  3. click Test button several times;
  4. notice A task was canceled. message which comes from Fluxor.Blazor.Web.StoreInitializer.UnhandledException callback;
  5. uncheck Auto cancel checkbox;
  6. click Test button again;
  7. notice The method or operation is not implemented. message and then a standard Blazor exception handling (a yellow bar at the bottom of the page and a exception's description in console).

When a checkbox is checked the Counter page dispatches a CounterAction action which simply awaits with a cancellation token. Then Counter page immediately cancels that token. At this point a TaskCancelledException exception is thrown and re-routed to Fluxor.Blazor.Web.StoreInitializer.UnhandledException. However, since this callback didn't 'handle' this exception I assumed it would 'bubble up' and be thrown in the main context. But it didn't.

To verify this when the checkbox is unchecked another exception (NotImplementedException) is thrown from the Counter action which is properly 'bubbled' and handled by Blazor framework in a standard way.

Is this by design or I'm using UnhandledException incorrectly?

mrpmorris commented 2 years ago

It is as designed.

The reason is that Fluxor doesn't (and shouldn't) know if it is running in an isolated process on a user's computer, or in a shared process space on a server.

Any unhandled exception for one user could bring down the service for all users. For example, try running this Counter page code in a Blazor Server app and watch what happens to all the other users when one of them ticks the checkbox to throw an exception before clicking the button.

@page "/counter"

<PageTitle>Counter</PageTitle>

<h1>Counter</h1>

<p role="status">Current count: @currentCount</p>

<button class="btn btn-primary" @onclick="IncrementCount">Click me</button>
<p>
    <EditForm Model=@this>
        <InputCheckbox @bind-Value=ShouldThrow />
    </EditForm>
    @ShouldThrow
</p>

@code {
    private int currentCount = 0;
    private bool ShouldThrow;

    private void IncrementCount()
    {
        currentCount++;
        if (ShouldThrow)
            new Timer(_ => throw new Exception("Ha"), null, 1000, 0);
    }
}
uhfath commented 2 years ago

Sorry, but I'm not sure I understand. My question was about the issue where only TaskCancelledException is 'swallowed' whereas any other type of exception is re-thrown as expected. And in both cases exceptions are unhandled by user code so the difference is only in their type.

mrpmorris commented 2 years ago

Could we have a teams meeting so you can screen share and go through this with me?

uhfath commented 2 years ago

Sure. I'm free right now if that's ok with you.

mrpmorris commented 2 years ago

Yep, send me an invitation mrpmorris@gmail.com Or just send an email and I'll reply with an invitation

uhfath commented 2 years ago

Sent an invitation. Hopefully correctly since never used teams before.

uhfath commented 2 years ago

@mrpmorris, thank you for your time in investigating this case!

As a result we've found out that both TaskCancelledException and OperationCancelledException are swallowed on Blazor side. Can be easily reproduced without fluxor involved. Some workarounds using OnUnhandledException:

  1. a separate javascript wrapper to be called from the event which communicates to blazor.webassembly.js to call it's internal exception callback (not the best);
  2. manually catching these exceptions and simply logging them (better);
  3. manually catching these exceptions and wrapping them in another one, e.g. AggregateException or even MyTaskCancelledException or MyOperationCancelledException (I prefer this approach).

Thanks again for your help, @mrpmorris !

mrpmorris commented 2 years ago

Thanks for taking the time to write this up!

If you add a report to the Microsoft repos, please post a link in here, out of interest :)

uhfath commented 2 years ago

Here it is: https://github.com/dotnet/aspnetcore/issues/41674

Looks like it's by design but I'll wait until team confirms it.

UPDATE_01: So we have a confirmation. According to this code the framework simply checks for cancelled tasks and swallows their exceptions. Not that I agree with this but I also understand their point.

mrpmorris commented 2 years ago

Your other option is to create your own exception type, and then in the UnhandledException event you could throw that with the original exception in InnerException.