Filter cancellation exceptions in generator driver #58843

chsienki · 2022-01-13T19:21:47Z

Previously we were using the fatal error handler to do filtering which has been obsoleted via #58094.

Also fixes #58290 by adding a missing filter to the syntax receiver shim.

chsienki · 2022-01-13T19:22:15Z

@dotnet/roslyn-compiler for review please.

src/Compilers/Core/Portable/SourceGeneration/UserFunction.cs

jasonmalinowski · 2022-01-13T19:32:08Z

src/Compilers/Core/Portable/SourceGeneration/Nodes/SyntaxReceiverInputNode.cs

                        }
                    }
-                    catch (Exception e)
+                    catch (Exception e) when (ExceptionUtilities.IsNotCancelled(e, cancellationToken))


Is this missing one causing issues? Do we need this in 17.1?

src/Compilers/Core/Portable/SourceGeneration/UserFunction.cs

src/Compilers/Core/Portable/InternalUtilities/ExceptionUtilities.cs

Make all uses take a token Pass the token through from parent wrapper operations.

chsienki · 2022-01-18T23:10:31Z

Ping @dotnet/roslyn-compiler for reviews please :)

jasonmalinowski · 2022-01-18T23:14:21Z

src/Compilers/Core/Portable/SourceGeneration/GeneratorDriver.cs

-#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) when (!ExceptionUtilities.IsCurrentOperationBeingCancelled(e, cancellationToken))


Hmm, I'd imagine Initialize can't ever throw a cancellation exception, right? So should the filter just be removed here?

Agreed. Changed

Code changed and brings new concerns.

jaredpar · 2022-01-21T03:57:28Z

src/Compilers/CSharp/Test/Semantic/SourceGeneration/SyntaxAwareGeneratorTests.cs

+
+            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(); } })),


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?

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

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.

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.

src/Compilers/CSharp/Test/Semantic/SourceGeneration/GeneratorDriverTests.cs

src/Compilers/Core/Portable/InternalUtilities/ExceptionUtilities.cs

sharwell · 2022-01-21T16:49:50Z

src/Compilers/Core/Portable/SourceGeneration/UserFunction.cs

-#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))


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

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.

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

jaredpar · 2022-01-27T01:06:37Z

src/Compilers/Core/Portable/InternalUtilities/ExceptionUtilities.cs

+        /// <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 && cancellationToken.IsCancellationRequested;


This could be true when exception and cancellationToken are unrelated instances. Is there a reason you aren't checking && oce.CancellationToken == cancellationToken here?

Effectively I would expect it to be written as such (do the prop check before type check as it's cheaper)

Suggested change

=> exception is OperationCanceledException oce && cancellationToken.IsCancellationRequested;

=>

cancellationToken.IsCancellationRequested &&

exception is OperationCanceledException oce &&

oce.CancellationToken == cancellationToken;

➡️ The omission of cancellation identity check is intentional. See #58843 (comment).

Suggested change

=> exception is OperationCanceledException oce && cancellationToken.IsCancellationRequested;

=> exception is OperationCanceledException && cancellationToken.IsCancellationRequested;

I think I prefer this better as then there would be no allocation of a new object (as a copy of exception casted to that exception type) needlessly just for this check (increasing performance).

jaredpar · 2022-01-27T01:09:06Z

src/Compilers/CSharp/Test/Semantic/SourceGeneration/SyntaxAwareGeneratorTests.cs

+
+            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(); } })),


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.

Original cancellation detection behavior was restored

src/Compilers/CSharp/Test/Semantic/SourceGeneration/SyntaxAwareGeneratorTests.cs

…eGeneratorTests.cs Co-authored-by: Jared Parsons <[email protected]>

jasonmalinowski · 2022-01-28T01:42:37Z

src/Compilers/CSharp/Test/Semantic/SourceGeneration/SyntaxAwareGeneratorTests.cs

+            Assert.Single(results.Results);
+            Assert.IsType<OperationCanceledException>(results.Results[0].Exception);
+            Assert.Equal("Simulated cancellation from external source", results.Results[0].Exception!.Message);


Suggested change

Assert.Single(results.Results);

Assert.IsType<OperationCanceledException>(results.Results[0].Exception);

Assert.Equal("Simulated cancellation from external source", results.Results[0].Exception!.Message);

var result = Assert.Single(results.Results);

Assert.IsType<OperationCanceledException>(result.Exception);

Assert.Equal("Simulated cancellation from external source", result.Exception!.Message);

jasonmalinowski · 2022-01-28T01:45:04Z

src/Compilers/Core/Portable/SourceGeneration/UserFunction.cs

-#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))


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

chsienki · 2022-01-28T18:33:35Z

@jasonmalinowski Filed #59141 to track

…ess-instance-members * upstream/main: (669 commits) Fix 'hasStaticConstructor' check in MethodCompiler (dotnet#59116) Update dependencies from https://github.com/dotnet/arcade build 20220127.8 (dotnet#59134) Update StreamJsonRpc (dotnet#59073) Resources Filter cancellation exceptions in generator driver (dotnet#58843) Revert "Remove dependency on EditorFeatures from Remote.ServiceHub project (dotnet#59059)" Strings Inline Convert to switch expression Explicitly test empty string case. Fix comment Simplify test code Run all Add tests Delete test generator Add support for specifying server in tests Remove options review feedback Format document after each provider (dotnet#59091) [main] Update dependencies from dotnet/arcade (dotnet#59015) ...

chsienki added Area-Compilers Feature - Source Generators Source Generators labels Jan 13, 2022

chsienki added this to the 17.2 milestone Jan 13, 2022

chsienki requested a review from a team as a code owner January 13, 2022 19:21

Filter cancellation exceptions in generator driver

0918664

chsienki force-pushed the source-generators/cancellation-filters branch from 886503d to 0918664 Compare January 13, 2022 19:24

sharwell reviewed Jan 13, 2022

View reviewed changes

src/Compilers/Core/Portable/SourceGeneration/UserFunction.cs Outdated Show resolved Hide resolved

jasonmalinowski previously approved these changes Jan 13, 2022

View reviewed changes

jasonmalinowski mentioned this pull request Jan 14, 2022

Remove places where the compiler is swallowing exceptions #58870

Merged

AraHaan suggested changes Jan 15, 2022

View reviewed changes

src/Compilers/Core/Portable/InternalUtilities/ExceptionUtilities.cs Outdated Show resolved Hide resolved

Simplify exception filter.

d7a7c30

Make all uses take a token Pass the token through from parent wrapper operations.

jasonmalinowski reviewed Jan 18, 2022

View reviewed changes

cston approved these changes Jan 18, 2022

View reviewed changes

Remove cancellation filter for init

79e0a5f

jasonmalinowski approved these changes Jan 19, 2022

View reviewed changes

chsienki added 2 commits January 20, 2022 11:57

Formatting and windows test fix

f1613ec

Formatting

7b053c2

jaredpar reviewed Jan 21, 2022

View reviewed changes

sharwell reviewed Jan 21, 2022

View reviewed changes

src/Compilers/CSharp/Test/Semantic/SourceGeneration/GeneratorDriverTests.cs Outdated Show resolved Hide resolved

sharwell previously requested changes Jan 21, 2022

View reviewed changes

src/Compilers/Core/Portable/InternalUtilities/ExceptionUtilities.cs Outdated Show resolved Hide resolved

sharwell reviewed Jan 21, 2022

View reviewed changes

Restore cancellation check behavior

4b4cfe6

jaredpar reviewed Jan 27, 2022

View reviewed changes

runfoapp bot mentioned this pull request Jan 27, 2022

InitializeToolset flakiness in correctness builds #58199

Closed

Feedback

ab09881

jaredpar reviewed Jan 27, 2022

View reviewed changes

src/Compilers/CSharp/Test/Semantic/SourceGeneration/SyntaxAwareGeneratorTests.cs Outdated Show resolved Hide resolved

jaredpar approved these changes Jan 27, 2022

View reviewed changes

Update src/Compilers/CSharp/Test/Semantic/SourceGeneration/SyntaxAwar…

7ba4bbf

…eGeneratorTests.cs Co-authored-by: Jared Parsons <[email protected]>

chsienki enabled auto-merge (squash) January 27, 2022 20:51

auto-merge was automatically disabled January 27, 2022 21:18
Pull Request is not mergeable

Fix null warning

1c33b7e

chsienki enabled auto-merge (squash) January 27, 2022 21:42

jasonmalinowski approved these changes Jan 28, 2022

View reviewed changes

chsienki merged commit 82ea47b into dotnet:main Jan 28, 2022

ghost modified the milestones: 17.2, Next Jan 28, 2022

chsienki mentioned this pull request Jan 28, 2022

Add an API to the GeneratorDriver to let us hook and report errors #59141

Open

RikkiGibson modified the milestones: Next, 17.2.P1 Feb 4, 2022

chsienki mentioned this pull request Jul 19, 2022

Don't report generator init failures as watsons #57315

Closed

	=> exception is OperationCanceledException oce && cancellationToken.IsCancellationRequested;
	=> exception is OperationCanceledException && cancellationToken.IsCancellationRequested;

Filter cancellation exceptions in generator driver #58843

Filter cancellation exceptions in generator driver #58843

Uh oh!

Conversation

chsienki commented Jan 13, 2022

Uh oh!

chsienki commented Jan 13, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

chsienki commented Jan 18, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chsienki commented Jan 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants