Skip to content

Convert connection lost exceptions to cancellations to avoid overreporting NFE #61792

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.

Sign up for GitHub

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

Merged
tmat merged 4 commits into from
Jun 16, 2022
Merged

Convert connection lost exceptions to cancellations to avoid overreporting NFE #61792

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
45555c5
Convert connection lost exceptions to cancellations to avoid overrepo…
tmat Jun 9, 2022
484df39
Typo and comment
tmat Jun 9, 2022
dd4e0d3
Feedback
tmat Jun 9, 2022
1107d1d
Refactoring
tmat Jun 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions src/Compilers/Core/Portable/InternalUtilities/FatalError.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,14 @@

namespace Microsoft.CodeAnalysis.ErrorReporting
{
internal sealed class OperationCanceledNotMachingCancellationTokenException : OperationCanceledException
{
public OperationCanceledNotMachingCancellationTokenException(Exception innerException)
: base(innerException.Message, innerException)
{
}
}

internal static class FatalError
{
public delegate void ErrorReporterHandler(Exception exception, ErrorSeverity severity, bool forceDump);
Expand Down Expand Up @@ -124,7 +132,7 @@ public static bool ReportAndPropagateUnlessCanceled(Exception exception, ErrorSe
[DebuggerHidden]
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)
Copy link
Member

@davidwengier davidwengier Jun 9, 2022

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@sharwell sharwell Jun 9, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@tmat tmat Jun 9, 2022

Choose a reason for hiding this comment

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

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.

{
return false;
}
Expand Down Expand Up @@ -200,7 +208,7 @@ public static bool ReportAndCatchUnlessCanceled(Exception exception, ErrorSeveri
[DebuggerHidden]
public static bool ReportAndCatchUnlessCanceled(Exception exception, CancellationToken contextCancellationToken, ErrorSeverity severity = ErrorSeverity.Uncategorized)
{
if (ExceptionUtilities.IsCurrentOperationBeingCancelled(exception, contextCancellationToken))
if (ExceptionUtilities.IsCurrentOperationBeingCancelled(exception, contextCancellationToken) || exception is OperationCanceledNotMachingCancellationTokenException)
{
return false;
}
Expand Down
16 changes: 5 additions & 11 deletions src/Workspaces/Remote/Core/RemoteCallback.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ namespace Microsoft.CodeAnalysis.Remote
/// Wraps calls from a remote brokered service back to the client or to an in-proc brokered service.
/// The purpose of this type is to handle exceptions thrown by the underlying remoting infrastructure
/// in manner that's compatible with our exception handling policies.
///
/// TODO: This wrapper might not be needed once https://github.com/microsoft/vs-streamjsonrpc/issues/246 is fixed.
/// </summary>
internal readonly struct RemoteCallback<T>
where T : class
Expand Down Expand Up @@ -96,16 +94,12 @@ private static bool ReportUnexpectedException(Exception exception, CancellationT
return false;
}

// Log unexpected state where a cancellation exception occurs without being requested.
return FatalError.ReportAndCatch(exception);
return true;
}

// When a connection is dropped and CancelLocallyInvokedMethodsWhenConnectionIsClosed is
// set ConnectionLostException should not be thrown. Instead the cancellation token should be
// signaled and OperationCancelledException should be thrown.
// Seems to not work in all cases currently, so we need to cancel ourselves (bug https://github.com/microsoft/vs-streamjsonrpc/issues/551).
// Once this issue is fixed we can remov this if statement and fall back to reporting NFW
// as any observation of ConnectionLostException indicates a bug (e.g. https://github.com/microsoft/vs-streamjsonrpc/issues/549).
// When a connection is dropped we can see ConnectionLostException 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.
if (exception is ConnectionLostException)
{
return true;
Expand All @@ -121,7 +115,7 @@ private static Exception OnUnexpectedException(Exception exception, Cancellation

if (exception is ConnectionLostException)
{
throw new OperationCanceledException(exception.Message, exception);
throw new OperationCanceledNotMachingCancellationTokenException(exception);
}

// If this is hit the cancellation token passed to the service implementation did not use the correct token,
Expand Down
Loading