-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Cleanup LSP error reporting #74530
Cleanup LSP error reporting #74530
Conversation
{ | ||
if (rethrowExceptions) | ||
{ | ||
return nonMutatingRequestTask; | ||
try |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixes a bug here where we would only report NFW for non-mutating requests. If a mutating request failed we would never get fault telemetry for it.
await nonMutatingRequestTask.ConfigureAwait(false); | ||
} | ||
// If we had an exception, we want to record a NFW for it AND propogate it out to the queue so it can be handled appropriately. | ||
catch (Exception ex) when (FatalError.ReportAndPropagateUnlessCanceled(ex, ErrorSeverity.Critical)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i believe this is the correct pattern to report and throw, but let me know if not.
}); | ||
|
||
var didReport = false; | ||
FatalError.OverwriteHandler((exception, severity, dumps) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tests to verify NFW handler is called for mutating and non-mutating requests
046213b
to
f2bc949
Compare
f2bc949
to
bb279fd
Compare
Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1922370
We were reporting connection lost exceptions when the server shutdown due to refresh notifications being sent during shutdown (they are triggered by events that are not a part of the shutdown handling in the queue).
This modifies the code to catch connection lost exceptions from refresh notifications. Other parts of the system already handle connection loss issues and report failures.
Additionally fixes an issue where we would not report NFW for mutating LSP requests.