-
Notifications
You must be signed in to change notification settings - Fork 53
Add DURABLE0011: ContinueAsNew warning for unbounded orchestration loops #660
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.
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
+345
−0
Merged
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
80a17c2
Add DURABLE0011: ContinueAsNew warning for unbounded orchestration loops
torosent 6892207
Fix DURABLE0011 analyzer: use semantic analysis, add sealed/Initializ…
torosent 5715b2b
Fix XML doc: clarify ContinueAsNew check is presence-based, not reach…
torosent 8bbc069
Address second Copilot review: message clarity, perf, test consistency
torosent 7859d44
Add analyzer verification screenshot
torosent e9ce5f6
Detect all unbounded while(true) loops calling context methods, not j…
torosent a651d34
Update verification screenshot with activity-only loop detection
torosent 2672ecc
Isolate CallSubOrchestratorAsync test to not depend on WaitForExterna…
torosent 92e8c16
Add helpLinkUri to DURABLE0011 DiagnosticDescriptor for consistency
torosent e70e4dc
Use aka.ms/durabletask-analyzers for helpLinkUri
torosent 5635fde
Address review: rename test, document span containment limitation
torosent 8f73a47
Use IWhileLoopOperation instead of span containment for loop scoping
torosent 480cd5e
Add FuncOrchestrator test coverage for DURABLE0011
torosent File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
109 changes: 109 additions & 0 deletions
109
src/Analyzers/Orchestration/ContinueAsNewOrchestrationAnalyzer.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,109 @@ | ||
| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT License. | ||
|
|
||
| using System.Collections.Immutable; | ||
| using Microsoft.CodeAnalysis; | ||
| using Microsoft.CodeAnalysis.CSharp; | ||
| using Microsoft.CodeAnalysis.CSharp.Syntax; | ||
| using Microsoft.CodeAnalysis.Diagnostics; | ||
| using Microsoft.CodeAnalysis.Operations; | ||
| using static Microsoft.DurableTask.Analyzers.Orchestration.ContinueAsNewOrchestrationAnalyzer; | ||
|
|
||
| namespace Microsoft.DurableTask.Analyzers.Orchestration; | ||
|
|
||
| /// <summary> | ||
| /// Analyzer that reports a warning when an orchestration contains an unconditional while loop | ||
| /// that calls any TaskOrchestrationContext method (e.g. CallActivityAsync, WaitForExternalEvent, | ||
| /// CallSubOrchestratorAsync, CreateTimer) but no ContinueAsNew call within that loop. | ||
| /// Every orchestration API call adds to the replay history, so unbounded loops without | ||
| /// ContinueAsNew lead to unbounded history growth. | ||
| /// </summary> | ||
| [DiagnosticAnalyzer(LanguageNames.CSharp)] | ||
| public sealed class ContinueAsNewOrchestrationAnalyzer : OrchestrationAnalyzer<ContinueAsNewOrchestrationVisitor> | ||
| { | ||
| /// <summary> | ||
| /// Diagnostic ID supported for the analyzer. | ||
| /// </summary> | ||
| public const string DiagnosticId = "DURABLE0011"; | ||
|
|
||
| static readonly LocalizableString Title = new LocalizableResourceString(nameof(Resources.ContinueAsNewOrchestrationAnalyzerTitle), Resources.ResourceManager, typeof(Resources)); | ||
| static readonly LocalizableString MessageFormat = new LocalizableResourceString(nameof(Resources.ContinueAsNewOrchestrationAnalyzerMessageFormat), Resources.ResourceManager, typeof(Resources)); | ||
|
|
||
| static readonly DiagnosticDescriptor Rule = new( | ||
| DiagnosticId, | ||
| Title, | ||
| MessageFormat, | ||
| AnalyzersCategories.Orchestration, | ||
| DiagnosticSeverity.Warning, | ||
| isEnabledByDefault: true, | ||
| helpLinkUri: "https://aka.ms/durabletask-analyzers"); | ||
|
|
||
| /// <inheritdoc/> | ||
| public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => [Rule]; | ||
|
|
||
| /// <summary> | ||
| /// Visitor that inspects orchestration methods for unbounded loops without ContinueAsNew. | ||
| /// Only direct invocations within the loop body are considered; calls made through helper | ||
| /// methods invoked from the loop are not tracked back to the loop context. | ||
| /// </summary> | ||
| public sealed class ContinueAsNewOrchestrationVisitor : MethodProbeOrchestrationVisitor | ||
| { | ||
| /// <inheritdoc/> | ||
| public override bool Initialize() | ||
| { | ||
| return this.KnownTypeSymbols.TaskOrchestrationContext is not null; | ||
| } | ||
|
|
||
| /// <inheritdoc/> | ||
| protected override void VisitMethod(SemanticModel semanticModel, SyntaxNode methodSyntax, IMethodSymbol methodSymbol, string orchestrationName, Action<Diagnostic> reportDiagnostic) | ||
| { | ||
| foreach (WhileStatementSyntax whileStatement in methodSyntax.DescendantNodes().OfType<WhileStatementSyntax>()) | ||
| { | ||
| if (!IsAlwaysTrueCondition(whileStatement.Condition)) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| IOperation? whileOperation = semanticModel.GetOperation(whileStatement); | ||
| if (whileOperation is not IWhileLoopOperation whileLoop || whileLoop.Body is null) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| bool hasHistoryGrowingCall = false; | ||
| bool hasContinueAsNew = false; | ||
|
|
||
| foreach (IInvocationOperation invocation in whileLoop.Body.Descendants().OfType<IInvocationOperation>()) | ||
| { | ||
| IMethodSymbol targetMethod = invocation.TargetMethod; | ||
|
|
||
| if (targetMethod.IsEqualTo(this.KnownTypeSymbols.TaskOrchestrationContext, "ContinueAsNew")) | ||
| { | ||
| hasContinueAsNew = true; | ||
| } | ||
| else if (SymbolEqualityComparer.Default.Equals(targetMethod.ContainingType, this.KnownTypeSymbols.TaskOrchestrationContext) || | ||
| SymbolEqualityComparer.Default.Equals(targetMethod.ContainingType?.OriginalDefinition, this.KnownTypeSymbols.TaskOrchestrationContext)) | ||
| { | ||
| hasHistoryGrowingCall = true; | ||
| } | ||
|
|
||
| if (hasHistoryGrowingCall && hasContinueAsNew) | ||
| { | ||
| break; | ||
| } | ||
| } | ||
torosent marked this conversation as resolved.
Dismissed
Show dismissed
Hide dismissed
|
||
|
|
||
torosent marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if (hasHistoryGrowingCall && !hasContinueAsNew) | ||
| { | ||
| reportDiagnostic(Diagnostic.Create(Rule, whileStatement.WhileKeyword.GetLocation(), orchestrationName)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| static bool IsAlwaysTrueCondition(ExpressionSyntax condition) | ||
| { | ||
| return condition is LiteralExpressionSyntax literal | ||
| && literal.IsKind(SyntaxKind.TrueLiteralExpression); | ||
| } | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
229 changes: 229 additions & 0 deletions
229
test/Analyzers.Tests/Orchestration/ContinueAsNewOrchestrationAnalyzerTests.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,229 @@ | ||
| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT License. | ||
|
|
||
| using Microsoft.CodeAnalysis.Testing; | ||
| using Microsoft.DurableTask.Analyzers.Orchestration; | ||
|
|
||
| using VerifyCS = Microsoft.DurableTask.Analyzers.Tests.Verifiers.CSharpAnalyzerVerifier<Microsoft.DurableTask.Analyzers.Orchestration.ContinueAsNewOrchestrationAnalyzer>; | ||
|
|
||
| namespace Microsoft.DurableTask.Analyzers.Tests.Orchestration; | ||
|
|
||
| public class ContinueAsNewOrchestrationAnalyzerTests | ||
| { | ||
| [Fact] | ||
| public async Task EmptyCodeHasNoDiag() | ||
| { | ||
| string code = @""; | ||
|
|
||
| await VerifyCS.VerifyDurableTaskAnalyzerAsync(code); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task TaskOrchestratorWhileTrueWithExternalEventNoContinueAsNew_ReportsDiagnostic() | ||
| { | ||
| string code = Wrapper.WrapTaskOrchestrator(@" | ||
| public class MyOrchestrator : TaskOrchestrator<object, object> | ||
| { | ||
| public override async Task<object> RunAsync(TaskOrchestrationContext context, object input) | ||
| { | ||
| {|#0:while|} (true) | ||
| { | ||
| var item = await context.WaitForExternalEvent<string>(""new-work""); | ||
| await context.CallActivityAsync<string>(""ProcessItem"", item); | ||
| } | ||
| } | ||
| } | ||
| "); | ||
| DiagnosticResult expected = BuildDiagnostic().WithLocation(0).WithArguments("MyOrchestrator"); | ||
|
|
||
| await VerifyCS.VerifyDurableTaskAnalyzerAsync(code, expected); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task TaskOrchestratorWhileTrueWithSubOrchestratorNoContinueAsNew_ReportsDiagnostic() | ||
| { | ||
| string code = Wrapper.WrapTaskOrchestrator(@" | ||
| public class MyOrchestrator : TaskOrchestrator<object, object> | ||
| { | ||
| public override async Task<object> RunAsync(TaskOrchestrationContext context, object input) | ||
| { | ||
| {|#0:while|} (true) | ||
| { | ||
| await context.CallSubOrchestratorAsync<string>(""ProcessItem"", null); | ||
| } | ||
| } | ||
| } | ||
| "); | ||
| DiagnosticResult expected = BuildDiagnostic().WithLocation(0).WithArguments("MyOrchestrator"); | ||
|
|
||
| await VerifyCS.VerifyDurableTaskAnalyzerAsync(code, expected); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task TaskOrchestratorWhileTrueWithContextCallsNoContinueAsNew_ReportsDiagnostic() | ||
| { | ||
| string code = Wrapper.WrapTaskOrchestrator(@" | ||
| public class MyOrchestrator : TaskOrchestrator<object, object> | ||
| { | ||
| public override async Task<object> RunAsync(TaskOrchestrationContext context, object input) | ||
| { | ||
| {|#0:while|} (true) | ||
| { | ||
| await context.CallActivityAsync<string>(""DoWork"", ""input""); | ||
| await context.CreateTimer(TimeSpan.FromSeconds(30), CancellationToken.None); | ||
| } | ||
| } | ||
torosent marked this conversation as resolved.
Show resolved
Hide resolved
torosent marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| "); | ||
| DiagnosticResult expected = BuildDiagnostic().WithLocation(0).WithArguments("MyOrchestrator"); | ||
|
|
||
| await VerifyCS.VerifyDurableTaskAnalyzerAsync(code, expected); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task TaskOrchestratorWhileTrueWithContinueAsNew_NoDiagnostic() | ||
| { | ||
| string code = Wrapper.WrapTaskOrchestrator(@" | ||
| public class MyOrchestrator : TaskOrchestrator<object, object> | ||
| { | ||
| public override async Task<object> RunAsync(TaskOrchestrationContext context, object input) | ||
| { | ||
| int count = 0; | ||
| while (true) | ||
| { | ||
| var item = await context.WaitForExternalEvent<string>(""new-work""); | ||
| await context.CallSubOrchestratorAsync<string>(""ProcessItem"", item); | ||
| count++; | ||
| if (count >= 100) | ||
| { | ||
| context.ContinueAsNew(count); | ||
| return null; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| "); | ||
|
|
||
| await VerifyCS.VerifyDurableTaskAnalyzerAsync(code); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task TaskOrchestratorWhileTrueWithOnlyActivitiesAndContinueAsNew_NoDiagnostic() | ||
| { | ||
| string code = Wrapper.WrapTaskOrchestrator(@" | ||
| public class MyOrchestrator : TaskOrchestrator<object, object> | ||
| { | ||
| public override async Task<object> RunAsync(TaskOrchestrationContext context, object input) | ||
| { | ||
| while (true) | ||
| { | ||
| await context.CallActivityAsync<string>(""DoWork"", ""input""); | ||
| await context.CreateTimer(TimeSpan.FromSeconds(30), CancellationToken.None); | ||
| context.ContinueAsNew(null); | ||
| return null; | ||
| } | ||
| } | ||
| } | ||
| "); | ||
|
|
||
| await VerifyCS.VerifyDurableTaskAnalyzerAsync(code); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task TaskOrchestratorNoWhileTrue_NoDiagnostic() | ||
| { | ||
| string code = Wrapper.WrapTaskOrchestrator(@" | ||
| public class MyOrchestrator : TaskOrchestrator<string, string> | ||
| { | ||
| public override async Task<string> RunAsync(TaskOrchestrationContext context, string input) | ||
| { | ||
| var result = await context.CallActivityAsync<string>(""DoWork"", input); | ||
| return result; | ||
| } | ||
| } | ||
| "); | ||
|
|
||
| await VerifyCS.VerifyDurableTaskAnalyzerAsync(code); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task TaskOrchestratorWhileTrueNoContextCalls_NoDiagnostic() | ||
| { | ||
| string code = Wrapper.WrapTaskOrchestrator(@" | ||
| public class MyOrchestrator : TaskOrchestrator<string, string> | ||
| { | ||
| public override async Task<string> RunAsync(TaskOrchestrationContext context, string input) | ||
| { | ||
| int i = 0; | ||
| while (true) | ||
| { | ||
| i++; | ||
| if (i > 10) return ""done""; | ||
| } | ||
| } | ||
| } | ||
| "); | ||
|
|
||
| await VerifyCS.VerifyDurableTaskAnalyzerAsync(code); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task DurableFunctionWhileTrueWithExternalEventNoContinueAsNew_ReportsDiagnostic() | ||
| { | ||
| string code = Wrapper.WrapDurableFunctionOrchestration(@" | ||
| [Function(""Run"")] | ||
| async Task<object> Method([OrchestrationTrigger] TaskOrchestrationContext context) | ||
| { | ||
| {|#0:while|} (true) | ||
| { | ||
| var item = await context.WaitForExternalEvent<string>(""new-work""); | ||
| await context.CallActivityAsync<string>(""ProcessItem"", item); | ||
| } | ||
| } | ||
| "); | ||
| DiagnosticResult expected = BuildDiagnostic().WithLocation(0).WithArguments("Run"); | ||
|
|
||
| await VerifyCS.VerifyDurableTaskAnalyzerAsync(code, expected); | ||
| } | ||
|
|
||
torosent marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| [Fact] | ||
| public async Task FuncOrchestratorWhileTrueWithContextCallsNoContinueAsNew_ReportsDiagnostic() | ||
| { | ||
| string code = Wrapper.WrapFuncOrchestrator(@" | ||
| tasks.AddOrchestratorFunc<string>(""Run"", async context => | ||
| { | ||
| {|#0:while|} (true) | ||
| { | ||
| var item = await context.WaitForExternalEvent<string>(""new-work""); | ||
| await context.CallActivityAsync<string>(""ProcessItem"", item); | ||
| } | ||
| }); | ||
| "); | ||
| DiagnosticResult expected = BuildDiagnostic().WithLocation(0).WithArguments("Run"); | ||
|
|
||
| await VerifyCS.VerifyDurableTaskAnalyzerAsync(code, expected); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task FuncOrchestratorWhileTrueNoContextCalls_NoDiagnostic() | ||
| { | ||
| string code = Wrapper.WrapFuncOrchestrator(@" | ||
| tasks.AddOrchestratorFunc<string>(""Run"", async context => | ||
| { | ||
| int i = 0; | ||
| while (true) | ||
| { | ||
| i++; | ||
| if (i > 10) return ""done""; | ||
| } | ||
| }); | ||
| "); | ||
|
|
||
| await VerifyCS.VerifyDurableTaskAnalyzerAsync(code); | ||
| } | ||
|
|
||
| static DiagnosticResult BuildDiagnostic() | ||
| { | ||
| return VerifyCS.Diagnostic(ContinueAsNewOrchestrationAnalyzer.DiagnosticId); | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.