dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
34.9k stars 9.86k forks source link

Blazor server InputFile not properly handling exceptions #38824

Open KrazyTako opened 2 years ago

KrazyTako commented 2 years ago

Describe the bug

When using the <InputFile> and binding the OnChange="@LoadFiles" method, if the method throws an exception it will not break the Blazor server circuit or log the error to the server's console. The error will be logged to the browser's console and the app will continue to function as if nothing happened. This behavior seems to differ from everything else I have seen when handling exceptions in Blazor Server.

For example, when I use a normal button and bind the @onclick="() => LoadFiles(null) method, an exceptions will be logged to the server's console and the Blazor Server circuit will be broken. Any reason this isn't the same behavior when using <InputFile>? This causes things to seemingly fail silently especially in async methods .

To Reproduce

<InputFile OnChange="@LoadFiles" />
<button @onclick="() => LoadFiles(null)">Blow up</button>

@code {
    private void LoadFiles(InputFileChangeEventArgs e) {
        throw new Exception();
    }
}

Further technical details

dotnet --info Output ``` .NET SDK (reflecting any global.json): Version: 6.0.100 Commit: 9e8b04bbff Runtime Environment: OS Name: Windows OS Version: 10.0.19042 OS Platform: Windows RID: win10-x64 Base Path: C:\Program Files\dotnet\sdk\6.0.100\ Host (useful for support): Version: 6.0.0 Commit: 4822e3c3aa .NET SDKs installed: 3.0.100 [C:\Program Files\dotnet\sdk] 5.0.200 [C:\Program Files\dotnet\sdk] 6.0.100-preview.6.21355.2 [C:\Program Files\dotnet\sdk] 6.0.100 [C:\Program Files\dotnet\sdk] ```
TanayParikh commented 2 years ago

Thanks for contacting us @KrazyTako, looks like this may be a duplicate of https://github.com/dotnet/aspnetcore/issues/38502.

SteveSandersonMS commented 2 years ago

We think this is because InputFile triggers the OnChange call via JS interop, which in general means that the JS-side code is responsible for handling any errors. In this case since there's no application JS code that calls this, we probably should treat it as a fatal exception. It should be relatively straightforwards to change this, though is arguably a breaking change.

TanayParikh commented 2 years ago

We'll want to await the InvokeAsync and wrap with a try/catch. In the case of an exception, we'll need to treat it as a fatal exception & crush the circuit.

https://github.com/dotnet/aspnetcore/blob/bf9225b8d7fd5f8ee855dd50407a90587b812662/src/Components/Web/src/Forms/InputFile.cs#L110

TanayParikh commented 2 years ago

Looked into this some more, it may not be as straightforward a solution as I previously thought. The main issue is the InputFile.cs is defined in Microsoft.AspNetCore.Components.Forms and is platform agnostic (server vs. wasm). So there isn't a clear way to force-crush the circuit as we do elsewhere (ex. via UnhandledException in CircuitHost).

https://github.com/dotnet/aspnetcore/blob/bf9225b8d7fd5f8ee855dd50407a90587b812662/src/Components/Server/src/Circuits/CircuitHost.cs#L64-L68

This may require some design on how we wish to handle this. One proposal is to implement an OnError event handler in addition to the existing OnChange handler. Then if OnChange fails here:

https://github.com/dotnet/aspnetcore/blob/bf9225b8d7fd5f8ee855dd50407a90587b812662/src/Components/Web/src/Forms/InputFile.cs#L110

Then we could redirect to the OnError handler.

ghost commented 2 years ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

Schaeri commented 1 year ago

The problem also exists in Blazor WASM. Here, in case of an exception, the ErrorBoundry is not notified about the error. In our case we handle all unexpected errors via the ErrorBoundry. This works for all events (Click, OnInitialize etc.). However, not for the OnChanged of the InputFile. If this aspect is implemented, the behavior of Blazor WASM should also be revised.

ghost commented 7 months ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.