Convert connection lost exceptions to cancellations to avoid overreporting NFE #61792
Conversation
ghost
added
the
| { | ||
| // When a connection is dropped we can see ObjectDisposedException even though CancelLocallyInvokedMethodsWhenConnectionIsClosed is set. | ||
| // That's because there might be a delay between the JsonRpc detecting the disconnect and the call attempting to send a message. | ||
| // Catch the ConnectionLostException exception here and convert it to OperationCanceledException. |
There was a problem hiding this comment.
| // Catch the ConnectionLostException exception here and convert it to OperationCanceledException. | |
| // Catch the ObjectDisposedException exception here and convert it to OperationCanceledException. |
| public static bool ReportAndPropagateUnlessCanceled(Exception exception, CancellationToken contextCancellationToken, ErrorSeverity severity = ErrorSeverity.Uncategorized) | ||
| { | ||
| if (ExceptionUtilities.IsCurrentOperationBeingCancelled(exception, contextCancellationToken)) | ||
| if (ExceptionUtilities.IsCurrentOperationBeingCancelled(exception, contextCancellationToken) || exception is OperationCanceledNotMachingCancellationTokenException) |
There was a problem hiding this comment.
Why not just put this check in IsCurrentOperationBeingCancelled?
Also, instead of a custom exception type, would it be easier to have an overload of IsCurrentOperationBeingCancelled that doesn't take a cancellation token at all, so the caller can decide whether they care about the tokens matching, without having to alter other places to throw a custom exception?
There was a problem hiding this comment.
Also, instead of a custom exception type, would it be easier to have an overload of IsCurrentOperationBeingCancelled that doesn't take a cancellation token at all, so the caller can decide whether they care about the tokens matching
Roslyn doesn't check for matching cancellation tokens.
There was a problem hiding this comment.
By matching I mean OperationCanceledException is thrown even when the cancellation token that flows into method is not signaled. Clearly Roslyn is checking that, this method does. If the call ends up calling back from OOP to devenv we would report NFE for that exception.
sharwell
left a comment
sharwell
left a comment
There was a problem hiding this comment.
Roslyn doesn't check for matching cancellation tokens (by design)
Can you link to the bucket showing this?
I thought this one was already prevented. Is something showing an unhandled case? |
It was already prevented - we just used plain OperationCancelledException instead of a specialized one. The change there is only to use the subtype to avoid NFEs reported from
I can't. I saw the exception being thrown while debugging the scenarios locally. |
This is the part that definitely looks covered by #61787. I particularly like that approach because it resolves the over-bucketing issue without completely blocking access to data about unexpected errors. |
|
Won't we start reporting ConnectionLostExceptions (that are wrapped in OperationCanceledException) if #61787 is merged? These are expected exceptions that occur on shutdown. |
sharwell
left a comment
sharwell
left a comment
There was a problem hiding this comment.
Reviewed with @tmat and we believe this only suppresses reporting of OperationCanceledException from ServiceHub in the specific case where it's caused by the communication pipes shutting down. Would like to see a better name for the exception if at all possible.
4 participants
No description provided.