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

Catch exceptions within LazyToolTip #52947

Merged
merged 1 commit into from
Apr 30, 2021

Conversation

sharwell
Copy link
Member

@sharwell sharwell commented Apr 27, 2021

This change converts a case involving a rare application crash to use our standard error handling/reporting strategy without crashing.

Fixes AB#621364

@sharwell sharwell requested review from a team as code owners April 27, 2021 01:37
@sharwell sharwell enabled auto-merge April 27, 2021 15:24
@jinujoseph jinujoseph added this to the 16.11 milestone Apr 27, 2021
@jasonmalinowski
Copy link
Member

Although there's no reason to be crashing processes for this, I'm worried that it appears the internal bug isn't really root caused -- is the plan to continue to investigate that in a later release?

@sharwell
Copy link
Member Author

sharwell commented Apr 27, 2021

@jasonmalinowski it is unlikely that this will be investigated or fixed. The hit rate is not significant relative to other non-fatal errors and I haven't seen any reports of specific negative side effects (i.e. the standard prioritization process for non-fatal errors does not indicate time should be spent performing the investigation which would be a prerequisite for a fix).

@jasonmalinowski
Copy link
Member

@sharwell I guess if the exception being thrown is due to projection being out of whack, this would potentially be just one of many potential crash points.

@sharwell sharwell disabled auto-merge April 27, 2021 18:07
@sharwell sharwell changed the base branch from main to release/dev16.11 April 30, 2021 15:04
@sharwell
Copy link
Member Author

@jasonmalinowski It's possible a crash still occurs, but most common paths go through top-level exception handlers that either report telemetry or show a gold bar (or both). This path was different because we were called directly by WPF.

@sharwell sharwell enabled auto-merge April 30, 2021 17:46
@CyrusNajmabadi
Copy link
Member

I dont' mind this in that all it is doing is taking something bad, and making it less bad for the user. We're still in a broken state, but we'll still get a watson from this. Hopefully though the user can continue. If they can't, that's ok as that's no worse than the state we're in now.

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm signing off. However, we should only take this through if Jason is ok and doesn't want something else done here as well. From my perspective though, this is strictly ok as not makign things worse, while potentially in some cases making it better for users.

For us, we can still track thsi trhough watson as we try to determine the root issue and what would need to be done with it.

@sharwell sharwell merged commit 0df296c into dotnet:release/dev16.11 Apr 30, 2021
@sharwell sharwell deleted the handle-exception branch April 30, 2021 20:38
@jasonmalinowski
Copy link
Member

@sharwell @CyrusNajmabadi Agreed; there's no reason to crash here, but the internal bug is now resolved, and no investigation to the root cause will now happen. I get the investigation is hard, but that'll still need to happen.

@sharwell
Copy link
Member Author

sharwell commented May 3, 2021

@jasonmalinowski The Watson issue is closed, but we automatically open issues for NFEs if they are hit a significant number of times in telemetry. According to @ryzngard this analysis process runs weekly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants