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

Blazor startup errors for duplicate routes is hard to discover and debug #3695

Closed
Eilon opened this issue Dec 8, 2021 · 11 comments
Closed
Assignees
Labels
area-blazor Blazor Hybrid / Desktop, BlazorWebView t/bug Something isn't working
Projects

Comments

@Eilon
Copy link
Member

Eilon commented Dec 8, 2021

Create an Blazor Hybrid app with a duplicate route. It will silently fail (you can find first-chance exceptions, but it's not obvious).

@Eilon Eilon added t/bug Something isn't working area-blazor Blazor Hybrid / Desktop, BlazorWebView labels Dec 8, 2021
@jsuarezruiz jsuarezruiz added this to New in Triage via automation Dec 9, 2021
@jsuarezruiz jsuarezruiz moved this from New to Ready For Work in Triage Dec 9, 2021
@javiercn
Copy link
Member

Gave this a try, it seems that the call stack is incomplete

   at Microsoft.AspNetCore.Components.Routing.Router.Refresh(Boolean isNavigationIntercepted)

@javiercn
Copy link
Member

The issue is in routing, PR here

@javiercn
Copy link
Member

@Eilon even if we fix this in Blazor, it won't make a difference unless we decide to patch it, so I leave it up to you decide whether you want to advocate for that.

@javiercn
Copy link
Member

@mkArtakMSFT the only remaining work here is to understand (if anything) why the call stack is incomplete, but I think that is not related to Blazor.

I'm closing this issue as there's no more work for us to do here.

Triage automation moved this from Ready For Work to Done Jan 28, 2022
@Eilon Eilon reopened this Jan 29, 2022
@Eilon
Copy link
Member Author

Eilon commented Jan 29, 2022

This issue wasn't about routing errors per se, but rather that if you have a problem with routing and it causes an exception, you have no idea that it happened. The "fix" here would be to make such problems visible to the developer.

@javiercn
Copy link
Member

javiercn commented Jan 29, 2022

@Eilon I'm a bit confused, can you provide a concrete and clear example of what you expect to happen here? From what I could tell the exception was accessible and clear and the part that was confusing was that we were producing a null reference exception because we changed the internal state of the router before throwing the exception.

After that change, the error is the same you get in other platforms like Blazor Server or Blazor Webassembly.

This issue wasn't about routing errors per se, but rather that if you have a problem with routing and it causes an exception, you have no idea that it happened. The "fix" here would be to make such problems visible to the developer.

Can you make a list of specific errors with concrete expected results? Otherwise its not clear what we need to account for and what the expectations are. As far as I can tell, after the fix I did in Blazor, you should be able to get a clear exception every time you have a set of incorrectly configured routes.

@javiercn
Copy link
Member

Also, note that to be very clear, the null reference exception causing this, is not a problem in Blazor desktop, it also happened in Blazor Webassembly and Blazor server.

@javiercn javiercn self-assigned this Jan 31, 2022
@Eilon
Copy link
Member Author

Eilon commented Feb 1, 2022

tell the exception was accessible and clear

It wasn't - you had to tell the debugger to catch all first chance exceptions (which isn't the default). The exception was not otherwise visible to the app developer - it looks like the app is running fine, except that it doesn't work.

I think to repro this, just have duplicate routes, run the app, and you see no exceptions (because I think the dup route bug was fixed only in .NET 7, right?).

The more general concern I have is that there is a whole class of this type of startup error that can be invisible to the developer, so even on .NET 7 where the dup route issue is fixed, I'm worried that there could be other types of exceptions that happen that would be just as invisible (but I don't know what exactly they might be).

@javiercn
Copy link
Member

javiercn commented Feb 1, 2022

It wasn't - you had to tell the debugger to catch all first chance exceptions (which isn't the default). The exception was not otherwise visible to the app developer - it looks like the app is running fine, except that it doesn't work.

This was the issue that I fixed, it wasn't Blazor Desktop specific, it was a Blazor routing issue.

I think to repro this, just have duplicate routes, run the app, and you see no exceptions (because I think the dup route bug was fixed only in .NET 7, right?).

You see a null reference exception, which was the exception that hid the original InvalidOperationException with the ambiguous routes.

The more general concern I have is that there is a whole class of this type of startup error that can be invisible to the developer, so even on .NET 7 where the dup route issue is fixed, I'm worried that there could be other types of exceptions that happen that would be just as invisible (but I don't know what exactly they might be).

I agree with you that this is within the realm of possibility, however, I would point out 2 things:

  • These would very likely be also "invisible" within other Blazor flavors.
  • What actions do you reasonably expect us to take here?

@javiercn
Copy link
Member

javiercn commented Feb 1, 2022

@Eilon I've captured the general expectations around exceptions in a separate issue, so I'm going to close this one in favor of that one, but before that, I want to make some clarifications around this issue.

The problem here is that one of our components throws an exception that is caught by the component itself before Blazor has any chance to even see the exception. As a result, a null reference exception is thrown from a separate call-site within the component instead of the original exception.

This is not Blazor Webview specific, as the webview code does not even get a chance to see the original exception, in the same way server and webassembly don't get a chance either.

It's not our intention that any component that we ship within Blazor (Routing, Forms, etc.) has these types of exceptions, when we find a situation like this, they get marked as a bug and fixed according to the assigned priority. I've provided a write-up of the general exception handling expectations in here. Since @TanayParikh has several bugs assigned in this area, I'm going to let him take the lead here.

Any similar bug in this category will have the same behavior on Blazor Server and Blazor Webassembly and if by chance any of our components exhibits this same behavior (where it throws and catches within the component, causing a separate issue to be raised instead) we will file a separate issue that will be specific to that component.

Based on this, I'm going to close up this issue as there's nothing more that we can reasonably do here. We already fixed the original issue in the component on .NET 7.0 and there is nothing we can do that is Blazor Webview specific to improve the situation until 7.0 (unless we decide to patch this issue in 6.0, which I leave up to you to advocate for).

I hope this helps clarify things.

@javiercn javiercn closed this as completed Feb 1, 2022
@Eilon
Copy link
Member Author

Eilon commented Feb 1, 2022

@javiercn - OK as long as you feel that the actual issue is fixed, that's fine with me 😄

@dotnet dotnet locked as resolved and limited conversation to collaborators Mar 3, 2022
@mkArtakMSFT mkArtakMSFT added this to the 6.0.300-preview.14 milestone Mar 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Blazor Hybrid / Desktop, BlazorWebView t/bug Something isn't working
Projects
No open projects
Development

No branches or pull requests

3 participants