Make json source generation cancellable#69332
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis Issue Detailsnull
|
| public void Initialize(GeneratorInitializationContext context) | ||
| { | ||
| context.RegisterForSyntaxNotifications(() => new SyntaxContextReceiver()); | ||
| context.RegisterForSyntaxNotifications(() => new SyntaxContextReceiver(context.CancellationToken)); |
There was a problem hiding this comment.
@chsienki is ok/correct to use this cancellation token here? is the delegate guaranteed to have a shorter lifetime than the CT for GeneratorInitializationContext ?
There was a problem hiding this comment.
No, you can't do this. On the second run through, the initialization isn't called again but the token will be something else, so you'll still be referring to the previously captured token. The lack of a token on the GeneratorSyntaxContext was an oversight.
In both compilers before 4.0 and currently we check the cancellation token before we visit each tree (https://sourceroslyn.io/#Microsoft.CodeAnalysis/SourceGeneration/SyntaxStore.cs,812c07816abb8b9e)
It's obviously not as efficient as doing it per node visit but it will cancel out relatively quickly, at least.
There was a problem hiding this comment.
Good to know. I'll undo this
src/libraries/System.Text.Json/gen/JsonSourceGenerator.Parser.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/gen/JsonSourceGenerator.Roslyn3.11.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/gen/JsonSourceGenerator.Roslyn3.11.cs
Outdated
Show resolved
Hide resolved
|
@chsienki can you review? @eiriktsarpalis I'm not sure what your merge requirements are. If tests are passing and there's a signoff, is that sufficient? Thanks! |
Yep, that's it. I'll merge once the current PR builds complete. |
|
@eiriktsarpalis Are the failures expected? |
src/libraries/System.Text.Json/gen/JsonSourceGenerator.Roslyn3.11.cs
Outdated
Show resolved
Hide resolved
….11.cs Co-authored-by: Sam Harwell <[email protected]>
|
Thanks! |
ghost
locked as resolved and limited conversation to collaborators
5 participants
No description provided.