Do not crash on unexpected exception

[tmat]

Avoid crashing the host process when an unexpected exception is thrown.

When an unexpected exception is thrown in ServiceHub process it needs to be propagated to the client so that we report actionable diagnostics in the log and in the info-bar. If the process is crashed via fail-fast we can't report good error. See e.g. #47234 (comment)

Proposal

ReportAndCatch, ReportAndCatchUnlessCanceled

The caller declares it is able to recover from the exception at this call site (catches the exception - returns true).
This uses NonFatal handler - csc ignores it, VS/OOP reports non-fatal Watson
In the compiler this would only be used for exceptions from analyzers.

ReportAndPropagate, ReportAndPropagateUnlessCanceled

The caller declares it is not able to recover at this point and the exception needs to be propagated (returns false).
This uses Fatal handler - csc fail-fast, VS/OOP reports non-fatal Watson
We would never use fail fast in these handlers in VS/OOP. We could potentially have env variable/reg key that forces fatal watson for testing/manual investigations

Review strategy: commit by commit.

[tmat]

@dotnet/roslyn-compiler @dotnet/roslyn-ide PTAL

[jasonmalinowski]

@tmat I know this sounds funny what I'm about to propose, but rather than changing this which also impacts other hosts (and anything running in devenv.exe itself), should we have the code in service hub set the 'fatal' handler to be non-fatal as well?

[sharwell]

@tmat I marked this as draft so it doesn't accidentally get merged before a design review.

[sharwell]

This is related to #47891 as well

[sharwell]

💡 Consider adding a data property to the exception indicating that it has already been reported through telemetry. Otherwise we could see large numbers of duplicate reports as the stack unwinds.

[jaredpar]

Avoid crashing the host process when an unexpected exception is thrown.
When an unexpected exception is thrown in ServiceHub process ...

This change doesn't seem like it impacts only ServiceHub though. This seems to change the policy for all hosts. Correct?

[tmat]

This change doesn't seem like it impacts only ServiceHub though. This seems to change the policy for all hosts. Correct?

Correct. I think it's better not to fail-fast in any of our hosts.

[tmat]

@tmat I know this sounds funny what I'm about to propose, but rather than changing this which also impacts other hosts (and anything running in devenv.exe itself), should we have the code in service hub set the 'fatal' handler to be non-fatal as well?

I don't think so. I think we need to consider at each call-site what the intended semantics is. In some rare cases we might want to actually fail fast. In most cases we should either recover or report NFE and propagate the exception.

[jaredpar]

Correct. I think it's better not to fail-fast in any of our hosts.

The hosts include the command line compilers and we generally feel that fail-fast there is highly beneficial to us.

[tmat]

The hosts include the command line compilers and we generally feel that fail-fast there is highly beneficial to us.

I see - because we don't report NFW from command line compilers, right? Should command line compilers set the handler for NFW to fail fast then?

[jaredpar]

Should command line compilers set the handler for NFW to fail fast then?

Yes. I think we should probably essentially have a method on the vein of UseHostPolicyUnlessCanceled that we use here so it's explicit that it's a host decision. Then we can focus on the hosts picking the right defaults here.

[tmat]

The handlers always used host policy - that has been the default.

Looking again at call sites in the compiler:

• In most call sites exceptions are definitely not expected and we want to crash in csc but not in VS/OOP.
• When we see an exception from analyzer we want to log NFW if in VS/OOP and do nothing in csc (not fail-fast).

So maybe we need to do what @jasonmalinowski suggested and also rename the methods to be clear.

Proposal:

ReportAndCatch, ReportAndCatchUnlessCanceled

• The caller declares it is able to recover from the exception at this call site (catches the exception - returns true).
• This uses NonFatal handler - csc ignores it, VS/OOP reports non-fatal Watson
• In the compiler this would only be used for exceptions from analyzers.

ReportAndPropagate, ReportAndPropagateUnlessCanceled

• The caller declares it is not able to recover at this point and the exception needs to be propagated (returns false).
• This uses Fatal handler - csc fail-fast, VS/OOP reports non-fatal Watson

We would never use fail fast in these handlers in VS/OOP. We could potentially have env variable/reg key that forces fatal watson for testing/manual investigations

[jaredpar]

@tmat love that summary in its entirety (including the env var trick to help with investigations)

[tmat]

Cool. I'll update the PR.

[jasonmalinowski]

The proposal makes sense to me @tmat. I'd say let's communicate well to the compiler team that what might have been 'fatal' in the command line may not be fatal in the VS/OOP case. If the compiler gets a bunch of non-fatal Watsons from VS those might not be possible to hit in the command line case because we marched along after a bad failure and had downstream issues. In the VS/OOP case a fatal exception is data loss, and so marching along is the right thing in many cases but it can be surprising.

[tmat]

we marched along after a bad failure and had downstream issues.

The first filter that will encounter the unexpected exception will report NFW - at this point it's the same stack/state as Fatal Watson from command line.
The handler marks the exception so that we don't report any more Watsons for it. If our code recovers from the exception (ReportAndCatch is used) then there is no unexpected downstream behavior (unless we have a bug in the recovery). If the code does not recover (ReportAndPropagate) then the exception is not caught and the same exception is propagated to the caller (but since it's marked we no longer report Watsons for it). It may happen that we translate one exception to another (e.g. wrap the exception in outer exception). But then we will see what the inner exception was and it's clear Watsons for the outer exception are different from the inner one (the outer exception no longer has a stack that originates in the compiler).

[tmat]

@sharwell PTAL - I have changed the approach from the original PR.

[cston]

Compiler changes LGTM.

[ryzngard]

Design lgtm, we should consider capturing dumps for VS/OOP in ReportAndPropagate and ReportAndPropagateUnlessCanceled cases, and maybe using severity on the FaultError to indicate that these are more problematic. Right now all of our FaultEvent calls use the same severity, maybe we should set severity to something more drastic as well. This will help us prioritize accordingly with telemetry we get.

[tmat]

@ryzngard We do capture dumps in all Report* handlers when running in VS/OOP - both fatal and non-fatal handlers end up reporting NFE. See 1366d93#diff-92b0fbadf21baf9af53e946ec4a2b433R30-R31.

Right now all of our FaultEvent calls use the same severity, maybe we should set severity to something more drastic as well. This will help us prioritize accordingly with telemetry we get.

We can do that, but I'd consider that a separate work item - feel free to file an issue if you think it would help.
I think it might be sufficient to prioritize either based on hit count. In either case a feature won't work due to a bug.

[tmat]

@sharwell OK to merge?

[sharwell]

@tmat I'll take a look later today but I'm guessing it's in line with what I was already asking for

[ryzngard]

@ryzngard We do capture dumps in all Report* handlers when running in VS/OOP - both fatal and non-fatal handlers end up reporting NFE. See 1366d93#diff-92b0fbadf21baf9af53e946ec4a2b433R30-R31.

Right now all of our FaultEvent calls use the same severity, maybe we should set severity to something more drastic as well. This will help us prioritize accordingly with telemetry we get.

We can do that, but I'd consider that a separate work item - feel free to file an issue if you think it would help.
I think it might be sufficient to prioritize either based on hit count. In either case a feature won't work due to a bug.

NFE != dump, so something to consider. For crashes we always* get a dump, and we no longer will be.

[sharwell]

@ryzngard that's true, but even for fatal errors the default heap dump collected is often insufficient and we have to request a full dump. In this case, the two options are close to the same.

[tmat]

@sharwell Ping.

[sharwell]

@Cosifne It looks like this is missing a milestone

[sharwell]

@tmat It looks like this is missing a milestone