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 3 commits
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
16 changes: 14 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,18 @@

namespace Microsoft.CodeAnalysis.ErrorReporting
{
/// <summary>
/// Thrown when async code must cancel the current execution but does not have access to the <see cref="CancellationTokenSource"/> of the <see cref="CancellationToken"/> passed to the code.
/// Should be used in very rare cases where the <see cref="CancellationTokenSource"/> is out of our control (e.g. owned but not exposed by JSON RPC in certain call-back scenarios).
/// </summary>
internal sealed class OperationCanceledNotMatchingCancellationTokenException : OperationCanceledException
{
public OperationCanceledNotMatchingCancellationTokenException(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 +136,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 OperationCanceledNotMatchingCancellationTokenException)
{
return false;
}
Expand Down Expand Up @@ -200,7 +212,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 OperationCanceledNotMatchingCancellationTokenException)
{
return false;
}
Expand Down
13 changes: 4 additions & 9 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 @@ -100,12 +98,9 @@ private static bool ReportUnexpectedException(Exception exception, CancellationT
return FatalError.ReportAndCatch(exception);
}

// 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 +116,7 @@ private static Exception OnUnexpectedException(Exception exception, Cancellation

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

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