Skip to content

Filter cancellation exceptions in generator driver #58843

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
chsienki merged 9 commits into from
Jan 28, 2022
Merged

Filter cancellation exceptions in generator driver #58843

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0918664
Filter cancellation exceptions in generator driver
chsienki Jan 13, 2022
d7a7c30
Simplify exception filter.
chsienki Jan 18, 2022
79e0a5f
Remove cancellation filter for init
chsienki Jan 18, 2022
f1613ec
Formatting and windows test fix
chsienki Jan 20, 2022
7b053c2
Formatting
chsienki Jan 20, 2022
4b4cfe6
Restore cancellation check behavior
chsienki Jan 26, 2022
ab09881
Feedback
chsienki Jan 27, 2022
7ba4bbf
Update src/Compilers/CSharp/Test/Semantic/SourceGeneration/SyntaxAwar…
chsienki Jan 27, 2022
1c33b7e
Fix null warning
chsienki Jan 27, 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: 8 additions & 4 deletions src/Compilers/CSharp/Test/Semantic/SourceGeneration/GeneratorDriverTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1527,13 +1527,17 @@ public void User_WrappedFunc_Throw_Exceptions()
Assert.IsType<InvalidOperationException>(e.InnerException);
}

CancellationTokenSource cts = new CancellationTokenSource();
cts.Cancel();

// cancellation is not wrapped, and is bubbled up
Assert.Throws<OperationCanceledException>(() => timeoutFunc(30, new CancellationToken(true)));
Assert.Throws<OperationCanceledException>(() => userTimeoutFunc(30, new CancellationToken(true)));
Assert.Throws<OperationCanceledException>(() => timeoutFunc(30, cts.Token));
Assert.Throws<OperationCanceledException>(() => userTimeoutFunc(30, cts.Token));

// unless it wasn't *our* cancellation token, in which case it still gets wrapped
Assert.Throws<OperationCanceledException>(() => otherTimeoutFunc(30, CancellationToken.None));
Assert.Throws<UserFunctionException>(() => userOtherTimeoutFunc(30, CancellationToken.None));
Assert.True(cts.TryReset());
Assert.Throws<OperationCanceledException>(() => otherTimeoutFunc(30, cts.Token));
Assert.Throws<UserFunctionException>(() => userOtherTimeoutFunc(30, cts.Token));
}

[Fact]
Expand Down
31 changes: 31 additions & 0 deletions src/Compilers/CSharp/Test/Semantic/SourceGeneration/SyntaxAwareGeneratorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1960,6 +1960,37 @@ class C { }
Assert.True(generatorCancelled);
}

[Fact]
public void Syntax_Receiver_Cancellation_During_Visit()
{
var source = @"
class C
{
int Property { get; set; }

void Function()
{
var x = 5;
x += 4;
}
}
";
var parseOptions = TestOptions.Regular;
Compilation compilation = CreateCompilation(source, options: TestOptions.DebugDll, parseOptions: parseOptions);
compilation.VerifyDiagnostics();

Assert.Single(compilation.SyntaxTrees);

var cts = new CancellationTokenSource();
var testGenerator = new CallbackGenerator(
onInit: (i) => i.RegisterForSyntaxNotifications(() => new TestSyntaxReceiver(tag: 0, callback: (a) => { if (a is AssignmentExpressionSyntax){ cts.Cancel(); cts.Token.ThrowIfCancellationRequested(); } })),

@jaredpar jaredpar Jan 21, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This cancellation step is capturing the CancellationTokenSource from the outer context and using it to both cancel and detect cancellation. Is there no way for a receiver to grab the CancellationToken passed into RunGeneratorsAndUpdateCompilation? If so then I would expect the ThrowIfCancellationRequested call to be on that token (make sure we thread it through).

If not how are syntax receivers expected to cancel?

@sharwell sharwell Jan 21, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

SyntaxReceivers do not have access to any cancellation tokens. All cancellation exceptions thrown by a syntax receiver are errors.

@jaredpar jaredpar Jan 27, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm unsure what we are testing then here. If user code doesn't have access to the token then they can't exercise this code path. If our internal walk of the syntax throws based on the cancellation token passed to the generator then we shouldn't need a throw here.

@chsienki chsienki Jan 27, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'll adjust it to just throw a new cancellation exception to indicate its not expected to be coming from the driver. Honestly I think the set up is mostly just a copy paste from the execute test.

onExecute: (e) => { e.AddSource("test", SourceText.From("public class D{}", Encoding.UTF8)); }
);

GeneratorDriver driver = CSharpGeneratorDriver.Create(new[] { testGenerator }, parseOptions: parseOptions);
Assert.Throws<OperationCanceledException>(() => driver.RunGeneratorsAndUpdateCompilation(compilation, out var outputCompilation, out var outputDiagnostics, cts.Token));
}

private class TestReceiverBase<T>
{
private readonly Action<T>? _callback;
Expand Down
10 changes: 10 additions & 0 deletions src/Compilers/Core/Portable/InternalUtilities/ExceptionUtilities.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

using System;
using System.Diagnostics;
using System.Threading;

namespace Roslyn.Utilities
{
Expand All @@ -27,5 +28,14 @@ internal static Exception Unreachable
{
get { return new InvalidOperationException("This program location is thought to be unreachable."); }
}

/// <summary>
/// Determine if an exception was an <see cref="OperationCanceledException"/>, and that the provided token caused the cancellation.
/// </summary>
/// <param name="exception">The exception to test.</param>
/// <param name="cancellationToken">Checked to see if the provided token was cancelled.</param>
/// <returns><see langword="true"/> if the exception was an <see cref="OperationCanceledException" /> and the token was canceled.</returns>
internal static bool IsCurrentOperationBeingCancelled(Exception exception, CancellationToken cancellationToken)
=> exception is OperationCanceledException oce && oce.CancellationToken == cancellationToken;
Comment thread
chsienki marked this conversation as resolved.
Outdated
}
}
10 changes: 4 additions & 6 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 @@ -7,6 +7,7 @@
using System.Diagnostics.CodeAnalysis;
using System.Reflection;
using System.Threading;
using Roslyn.Utilities;

#if NET20
// Some APIs referenced by documentation comments are not available on .NET Framework 2.0.
Expand Down Expand Up @@ -91,9 +92,6 @@ public static void CopyHandlerTo(Assembly assembly)

#endif

private static bool IsCurrentOperationBeingCancelled(Exception exception, CancellationToken cancellationToken)
=> exception is OperationCanceledException && cancellationToken.IsCancellationRequested;

/// <summary>
/// Use in an exception filter to report an error without catching the exception.
/// The error is reported by calling <see cref="Handler"/>.
Expand Down Expand Up @@ -143,7 +141,7 @@ public static bool ReportAndPropagateUnlessCanceled(Exception exception, ErrorSe
[DebuggerHidden]
public static bool ReportAndPropagateUnlessCanceled(Exception exception, CancellationToken contextCancellationToken, ErrorSeverity severity = ErrorSeverity.Uncategorized)
{
if (IsCurrentOperationBeingCancelled(exception, contextCancellationToken))
if (ExceptionUtilities.IsCurrentOperationBeingCancelled(exception, contextCancellationToken))
{
return false;
}
Expand Down Expand Up @@ -258,7 +256,7 @@ public static bool ReportIfNonFatalAndCatchUnlessCanceled(Exception exception, E
#endif
static bool ReportAndCatchUnlessCanceled(Exception exception, CancellationToken contextCancellationToken, ErrorSeverity severity = ErrorSeverity.Uncategorized)
{
if (IsCurrentOperationBeingCancelled(exception, contextCancellationToken))
if (ExceptionUtilities.IsCurrentOperationBeingCancelled(exception, contextCancellationToken))
{
return false;
}
Expand Down Expand Up @@ -290,7 +288,7 @@ static bool ReportAndCatchUnlessCanceled(Exception exception, CancellationToken
[Obsolete("This is only to support places the compiler is catching and swallowing exceptions on the command line; do not use in new code.")]
public static bool ReportIfNonFatalAndCatchUnlessCanceled(Exception exception, CancellationToken contextCancellationToken, ErrorSeverity severity = ErrorSeverity.Uncategorized)
{
if (IsCurrentOperationBeingCancelled(exception, contextCancellationToken))
if (ExceptionUtilities.IsCurrentOperationBeingCancelled(exception, contextCancellationToken))
{
return false;
}
Expand Down
5 changes: 1 addition & 4 deletions src/Compilers/Core/Portable/SourceGeneration/GeneratorDriver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
using System.Linq;
using System.Threading;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.ErrorReporting;
using Microsoft.CodeAnalysis.PooledObjects;
using Roslyn.Utilities;

Expand Down Expand Up @@ -193,9 +192,7 @@ internal GeneratorDriverState RunGeneratorsCore(Compilation compilation, Diagnos
{
generator.Initialize(pipelineContext);
}
#pragma warning disable CS0618 // ReportIfNonFatalAndCatchUnlessCanceled is obsolete; tracked by https://github.com/dotnet/roslyn/issues/58375
catch (Exception e) when (FatalError.ReportIfNonFatalAndCatchUnlessCanceled(e, cancellationToken))
#pragma warning restore CS0618 // ReportIfNonFatalAndCatchUnlessCanceled is obsolete
catch (Exception e)
{
ex = e;
}
Expand Down
6 changes: 3 additions & 3 deletions src/Compilers/Core/Portable/SourceGeneration/Nodes/PostInitOutputNode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@ namespace Microsoft.CodeAnalysis
{
internal sealed class PostInitOutputNode : IIncrementalGeneratorOutputNode
{
private readonly Action<IncrementalGeneratorPostInitializationContext> _callback;
private readonly Action<IncrementalGeneratorPostInitializationContext, CancellationToken> _callback;

public PostInitOutputNode(Action<IncrementalGeneratorPostInitializationContext> callback)
public PostInitOutputNode(Action<IncrementalGeneratorPostInitializationContext, CancellationToken> callback)
{
_callback = callback;
}
Expand All @@ -20,7 +20,7 @@ public PostInitOutputNode(Action<IncrementalGeneratorPostInitializationContext>

public void AppendOutputs(IncrementalExecutionContext context, CancellationToken cancellationToken)
{
_callback(new IncrementalGeneratorPostInitializationContext(context.Sources, cancellationToken));
_callback(new IncrementalGeneratorPostInitializationContext(context.Sources, cancellationToken), cancellationToken);
}
}
}
6 changes: 3 additions & 3 deletions src/Compilers/Core/Portable/SourceGeneration/Nodes/SourceOutputNode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,13 @@ internal sealed class SourceOutputNode<TInput> : IIncrementalGeneratorOutputNode
{
private readonly IIncrementalGeneratorNode<TInput> _source;

private readonly Action<SourceProductionContext, TInput> _action;
private readonly Action<SourceProductionContext, TInput, CancellationToken> _action;

private readonly IncrementalGeneratorOutputKind _outputKind;

private readonly string _sourceExtension;

public SourceOutputNode(IIncrementalGeneratorNode<TInput> source, Action<SourceProductionContext, TInput> action, IncrementalGeneratorOutputKind outputKind, string sourceExtension)
public SourceOutputNode(IIncrementalGeneratorNode<TInput> source, Action<SourceProductionContext, TInput, CancellationToken>action, IncrementalGeneratorOutputKind outputKind, string sourceExtension)
{
_source = source;
_action = action;
Expand Down Expand Up @@ -70,7 +70,7 @@ public NodeStateTable<TOutput> UpdateStateTable(DriverStateTable.Builder graphSt
try
{
var stopwatch = SharedStopwatch.StartNew();
_action(context, entry.Item);
_action(context, entry.Item, cancellationToken);
var sourcesAndDiagnostics = (sourcesBuilder.ToImmutable(), diagnostics.ToReadOnly());
nodeTable.AddEntry(sourcesAndDiagnostics, EntryState.Added, stopwatch.Elapsed, inputs, EntryState.Added);
}
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/Core/Portable/SourceGeneration/Nodes/SyntaxReceiverInputNode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ public void VisitTree(Lazy<SyntaxNode> root, EntryState state, SemanticModel? mo
lastElapsedTime = stopwatch.Elapsed;
}
}
catch (Exception e)
catch (Exception e) when (!ExceptionUtilities.IsCurrentOperationBeingCancelled(e, cancellationToken))
{
throw new UserFunctionException(e);
}
Expand Down
22 changes: 8 additions & 14 deletions src/Compilers/Core/Portable/SourceGeneration/UserFunction.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Threading;
using Microsoft.CodeAnalysis.ErrorReporting;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis
{
Expand All @@ -30,9 +30,7 @@ internal static Func<TInput, CancellationToken, TOutput> WrapUserFunction<TInput
{
return userFunction(input, token);
}
#pragma warning disable CS0618 // ReportIfNonFatalAndCatchUnlessCanceled is obsolete; tracked by https://github.com/dotnet/roslyn/issues/58375
catch (Exception e) when (FatalError.ReportIfNonFatalAndCatchUnlessCanceled(e, token))
#pragma warning restore CS0618 // ReportIfNonFatalAndCatchUnlessCanceled is obsolete
catch (Exception e) when (!ExceptionUtilities.IsCurrentOperationBeingCancelled(e, token))

@sharwell sharwell Jan 21, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

❔ Are we still going to get NFW reporting if/when these occur inside the IDE

@jasonmalinowski jasonmalinowski Jan 25, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As a separate change we're going to add an API to the GeneratorDriver to let us hook and report these, which is similar to how we do this for analyzers. Having this via an internal only API that's mixed in for fault reporting is a bit funky.

@jasonmalinowski jasonmalinowski Jan 28, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@chsienki Do we have a tracking bug to actually do that?

{
throw new UserFunctionException(e);
}
Expand All @@ -44,34 +42,30 @@ internal static Func<TInput, CancellationToken, ImmutableArray<TOutput>> WrapUse
return (input, token) => userFunction.WrapUserFunction()(input, token).ToImmutableArray();
}

internal static Action<TInput> WrapUserAction<TInput>(this Action<TInput> userAction)
internal static Action<TInput, CancellationToken> WrapUserAction<TInput>(this Action<TInput> userAction)
{
return input =>
return (input, token) =>
{
try
{
userAction(input);
}
#pragma warning disable CS0618 // ReportIfNonFatalAndCatchUnlessCanceled is obsolete; tracked by https://github.com/dotnet/roslyn/issues/58375
catch (Exception e) when (FatalError.ReportIfNonFatalAndCatchUnlessCanceled(e))
#pragma warning restore CS0618 // ReportIfNonFatalAndCatchUnlessCanceled is obsolete
catch (Exception e) when (!ExceptionUtilities.IsCurrentOperationBeingCancelled(e, token))
{
throw new UserFunctionException(e);
}
};
}

internal static Action<TInput1, TInput2> WrapUserAction<TInput1, TInput2>(this Action<TInput1, TInput2> userAction)
internal static Action<TInput1, TInput2, CancellationToken> WrapUserAction<TInput1, TInput2>(this Action<TInput1, TInput2> userAction)
{
return (input1, input2) =>
return (input1, input2, token) =>
{
try
{
userAction(input1, input2);
}
#pragma warning disable CS0618 // ReportIfNonFatalAndCatchUnlessCanceled is obsolete; tracked by https://github.com/dotnet/roslyn/issues/58375
catch (Exception e) when (FatalError.ReportIfNonFatalAndCatchUnlessCanceled(e))
#pragma warning restore CS0618 // ReportIfNonFatalAndCatchUnlessCanceled is obsolete
catch (Exception e) when (!ExceptionUtilities.IsCurrentOperationBeingCancelled(e, token))
{
throw new UserFunctionException(e);
}
Expand Down