From b114757a67b38889d4fd5c559a5e48d369c08e55 Mon Sep 17 00:00:00 2001 From: Sergey Teplyakov Date: Tue, 1 Jul 2025 15:33:54 -0700 Subject: [PATCH 1/2] Add analyzer for async delegates with LongRunning tasks Introduces EPC36, which warns when async delegates are used with Task.Factory.StartNew and TaskCreationOptions.LongRunning. Adds analyzer implementation, tests, documentation, and diagnostic descriptor. This helps prevent inefficient use of threads and clarifies intent when working with async code. --- docs/Rules/EPC36.md | 164 +++++++++++ ...legatesForLongRunningTasksAnalyzerTests.cs | 263 ++++++++++++++++++ .../AnalyzerReleases.Unshipped.md | 3 +- ...yncDelegatesForLongRunningTasksAnalyzer.cs | 159 +++++++++++ .../DiagnosticDescriptors.cs | 11 + 5 files changed, 599 insertions(+), 1 deletion(-) create mode 100644 docs/Rules/EPC36.md create mode 100644 src/ErrorProne.NET.CoreAnalyzers.Tests/AsyncAnalyzers/DoNotUseAsyncDelegatesForLongRunningTasksAnalyzerTests.cs create mode 100644 src/ErrorProne.NET.CoreAnalyzers/AsyncAnalyzers/DoNotUseAsyncDelegatesForLongRunningTasksAnalyzer.cs diff --git a/docs/Rules/EPC36.md b/docs/Rules/EPC36.md new file mode 100644 index 0000000..7a3257b --- /dev/null +++ b/docs/Rules/EPC36.md @@ -0,0 +1,164 @@ +# EPC36 - Do not use async delegates with Task.Factory.StartNew and TaskCreationOptions.LongRunning + +This analyzer warns when an async delegate is used with `Task.Factory.StartNew` and `TaskCreationOptions.LongRunning`. + +## Description + +`TaskCreationOptions.LongRunning` is intended for long-running synchronous work that would otherwise block a thread pool thread for an extended period. When used with async delegates, it defeats the purpose of async programming by: + +1. **Wasting thread pool threads**: The LongRunning option creates a dedicated thread that will be used only before the first `await` in the async delegate (or async method). +3. **Creating confusion**: The intent of the code becomes unclear - is it meant to be long-running synchronous work or efficient async work? Is it really the case that the synchronous block of the async method is taking a long time, or it's just a mistake? + +## Code that triggers the analyzer + +❌ **Bad** - Using async lambda with LongRunning: +```csharp +using System; +using System.Threading.Tasks; + +class Example +{ + void BadExamples() + { + // ❌ EPC36: Async lambda with LongRunning + Task.Factory.StartNew(async () => + { + await SomeAsyncOperation(); + }, TaskCreationOptions.LongRunning); + + // ❌ EPC36: Async delegate with LongRunning + Task.Factory.StartNew(async delegate() + { + await SomeAsyncOperation(); + }, TaskCreationOptions.LongRunning); + + // ❌ EPC36: Async method reference with LongRunning + Task.Factory.StartNew(SomeAsyncMethod, TaskCreationOptions.LongRunning); + + // ❌ EPC36: Combined options including LongRunning + Task.Factory.StartNew(async () => + { + await SomeAsyncOperation(); + }, TaskCreationOptions.LongRunning | TaskCreationOptions.AttachedToParent); + } + + async Task SomeAsyncMethod() => await Task.Delay(100); + async Task SomeAsyncOperation() => await Task.Delay(1000); +} +``` + +## How to fix + +Choose the appropriate approach based on your intent: + +✅ **Good** - Use `Task.Run` for async delegates: +```csharp +using System; +using System.Threading.Tasks; + +class Example +{ + void GoodExamples() + { + // ✅ Correct: Use Task.Run for async work + Task.Run(async () => + { + await SomeAsyncOperation(); + }); + + // ✅ Correct: Task.Run with async method reference + Task.Run(SomeAsyncMethod); + + // ✅ Correct: If you need specific task creation options (except LongRunning) + Task.Factory.StartNew(async () => + { + await SomeAsyncOperation(); + }, TaskCreationOptions.AttachedToParent); + } + + async Task SomeAsyncMethod() => await Task.Delay(100); + async Task SomeAsyncOperation() => await Task.Delay(1000); +} +``` + +✅ **Good** - Use LongRunning only for truly long-running synchronous work: +```csharp +using System; +using System.Threading.Tasks; + +class Example +{ + void LongRunningSyncWork() + { + // ✅ Correct: LongRunning with synchronous work + Task.Factory.StartNew(() => + { + // Long-running CPU-intensive or blocking synchronous operation + for (int i = 0; i < 1_000_000_000; i++) + { + // Some heavy computation + ProcessData(i); + } + }, TaskCreationOptions.LongRunning); + + // ✅ Correct: LongRunning with blocking I/O that can't be made async + Task.Factory.StartNew(() => + { + // Legacy blocking operation that can't be easily made async + LegacyBlockingOperation(); + }, TaskCreationOptions.LongRunning); + } + + void ProcessData(int value) { /* CPU work */ } + void LegacyBlockingOperation() { /* Blocking I/O */ } +} +``` + +## When to use each approach + +- **`Task.Run`**: For async operations or when you need to run async code on the thread pool +- **`Task.Factory.StartNew` without LongRunning**: When you need specific task creation options but still want efficient async execution +- **`Task.Factory.StartNew` with LongRunning**: Only for long-running synchronous operations that would otherwise tie up thread pool threads + +## Performance impact + +Using async delegates with LongRunning can lead to: +- **Thread pool starvation**: Each LongRunning task consumes a dedicated thread +- **Increased memory usage**: Each thread consumes approximately 1MB of virtual memory for its stack +- **Reduced scalability**: The application cannot efficiently handle many concurrent operations + +## Examples in context + +❌ **Bad** - Common anti-pattern: +```csharp +// This creates a dedicated thread that just waits for async operations +var tasks = new List(); +for (int i = 0; i < 100; i++) +{ + tasks.Add(Task.Factory.StartNew(async () => + { + await DownloadFileAsync($"file{i}.txt"); // ❌ EPC36 + }, TaskCreationOptions.LongRunning)); +} +await Task.WhenAll(tasks); +``` + +✅ **Good** - Efficient async pattern: +```csharp +// This uses the thread pool efficiently for async operations +var tasks = new List(); +for (int i = 0; i < 100; i++) +{ + tasks.Add(Task.Run(async () => + { + await DownloadFileAsync($"file{i}.txt"); // ✅ Correct + })); +} +await Task.WhenAll(tasks); +``` + +## See also + +- [Task.Run vs Task.Factory.StartNew](https://docs.microsoft.com/en-us/dotnet/standard/parallel-programming/task-run-vs-task-factory-startnew) +- [TaskCreationOptions.LongRunning documentation](https://docs.microsoft.com/en-us/dotnet/api/system.threading.tasks.taskcreationoptions) +- [Async/await best practices](https://docs.microsoft.com/en-us/archive/msdn-magazine/2013/march/async-await-best-practices-in-asynchronous-programming) diff --git a/src/ErrorProne.NET.CoreAnalyzers.Tests/AsyncAnalyzers/DoNotUseAsyncDelegatesForLongRunningTasksAnalyzerTests.cs b/src/ErrorProne.NET.CoreAnalyzers.Tests/AsyncAnalyzers/DoNotUseAsyncDelegatesForLongRunningTasksAnalyzerTests.cs new file mode 100644 index 0000000..89f30d0 --- /dev/null +++ b/src/ErrorProne.NET.CoreAnalyzers.Tests/AsyncAnalyzers/DoNotUseAsyncDelegatesForLongRunningTasksAnalyzerTests.cs @@ -0,0 +1,263 @@ +using System; +using NUnit.Framework; +using System.Threading.Tasks; +using Verify = ErrorProne.NET.TestHelpers.CSharpCodeFixVerifier< + ErrorProne.NET.AsyncAnalyzers.DoNotUseAsyncDelegatesForLongRunningTasksAnalyzer, + Microsoft.CodeAnalysis.Testing.EmptyCodeFixProvider>; + +namespace ErrorProne.NET.CoreAnalyzers.Tests.AsyncAnalyzers +{ + [TestFixture] + public class DoNotUseAsyncDelegatesForLongRunningTasksAnalyzerTests + { + [Test] + public async Task WarnOnAsyncLambdaWithLongRunning() + { + var test = @" +using System; +using System.Threading.Tasks; + +class Test +{ + void TestMethod() + { + [|Task.Factory.StartNew(async () => { await Task.Delay(100); }, TaskCreationOptions.LongRunning)|]; + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task WarnOnAsyncDelegateWithLongRunning() + { + var test = @" +using System; +using System.Threading.Tasks; + +class Test +{ + void TestMethod() + { + [|Task.Factory.StartNew(async delegate() { await Task.Delay(100); }, TaskCreationOptions.LongRunning)|]; + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task WarnOnAsyncMethodReferenceWithLongRunning() + { + var test = @" +using System; +using System.Threading.Tasks; + +class Test +{ + async Task AsyncMethod() => await Task.Delay(100); + + void TestMethod() + { + [|Task.Factory.StartNew(AsyncMethod, TaskCreationOptions.LongRunning)|]; + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task WarnOnAsyncLambdaWithCombinedOptions() + { + var test = @" +using System; +using System.Threading.Tasks; + +class Test +{ + void TestMethod() + { + [|Task.Factory.StartNew(async () => { await Task.Delay(100); }, TaskCreationOptions.LongRunning | TaskCreationOptions.AttachedToParent)|]; + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task NoWarningOnSyncLambdaWithLongRunning() + { + var test = @" +using System; +using System.Threading.Tasks; + +class Test +{ + void TestMethod() + { + Task.Factory.StartNew(() => { System.Threading.Thread.Sleep(100); }, TaskCreationOptions.LongRunning); + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task NoWarningOnSyncDelegateWithLongRunning() + { + var test = @" +using System; +using System.Threading.Tasks; + +class Test +{ + void TestMethod() + { + Task.Factory.StartNew(delegate() { System.Threading.Thread.Sleep(100); }, TaskCreationOptions.LongRunning); + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task NoWarningOnAsyncLambdaWithoutLongRunning() + { + var test = @" +using System; +using System.Threading.Tasks; + +class Test +{ + void TestMethod() + { + Task.Factory.StartNew(async () => { await Task.Delay(100); }); + Task.Factory.StartNew(async () => { await Task.Delay(100); }, TaskCreationOptions.None); + Task.Factory.StartNew(async () => { await Task.Delay(100); }, TaskCreationOptions.AttachedToParent); + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task NoWarningOnTaskRun() + { + var test = @" +using System; +using System.Threading.Tasks; + +class Test +{ + void TestMethod() + { + Task.Run(async () => { await Task.Delay(100); }); + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task WarnOnTaskFactoryStartNewWithDirectOptions() + { + var test = @" +using System; +using System.Threading.Tasks; + +class Test +{ + void TestMethod() + { + var factory = Task.Factory; + [|factory.StartNew(async () => { await Task.Delay(100); }, TaskCreationOptions.LongRunning)|]; + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task WarnOnNewTaskFactoryStartNew() + { + var test = @" +using System; +using System.Threading.Tasks; + +class Test +{ + void TestMethod() + { + var factory = new TaskFactory(); + [|factory.StartNew(async () => { await Task.Delay(100); }, TaskCreationOptions.LongRunning)|]; + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task NoWarningOnSyncMethodReferenceWithLongRunning() + { + var test = @" +using System; +using System.Threading.Tasks; + +class Test +{ + void SyncMethod() => System.Threading.Thread.Sleep(100); + + void TestMethod() + { + Task.Factory.StartNew(SyncMethod, TaskCreationOptions.LongRunning); + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task WarnOnAsyncLambdaWithParametersAndLongRunning() + { + var test = @" +using System; +using System.Threading.Tasks; + +class Test +{ + void TestMethod() + { + var state = ""test""; + [|Task.Factory.StartNew(async (obj) => { await Task.Delay(100); }, state, TaskCreationOptions.LongRunning)|]; + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task NoWarningOnGenericTaskFactorySyncMethodReferenceWithLongRunning() + { + var test = @" +using System; +using System.Threading.Tasks; + +class Test +{ + int SyncMethod() + { + System.Threading.Thread.Sleep(100); + return 42; + } + + void TestMethod() + { + var factory = new TaskFactory(); + factory.StartNew(SyncMethod, TaskCreationOptions.LongRunning); + } +} +"; + await Verify.VerifyAsync(test); + } + } +} diff --git a/src/ErrorProne.NET.CoreAnalyzers/AnalyzerReleases.Unshipped.md b/src/ErrorProne.NET.CoreAnalyzers/AnalyzerReleases.Unshipped.md index ca6cd2f..1fb7d80 100644 --- a/src/ErrorProne.NET.CoreAnalyzers/AnalyzerReleases.Unshipped.md +++ b/src/ErrorProne.NET.CoreAnalyzers/AnalyzerReleases.Unshipped.md @@ -13,7 +13,7 @@ EPC16 | Async | Warning | NullConditionalOperatorAnalyzer EPC17 | Async | Warning | AsyncVoidDelegateAnalyzer EPC18 | Async | Warning | TaskInstanceToStringConversionAnalyzer EPC19 | CodeSmell | Warning | CancellationTokenRegistrationAnalyzer -EPC20 | Async | Warning | DefaultToStringImplementationUsageAnalyzer +EPC20 | CodeSmell | Warning | DefaultToStringImplementationUsageAnalyzer EPC23 | Performance | Warning | HashSetContainsAnalyzer EPC24 | Performance | Warning | HashTableIncompatibilityAnalyzer EPC25 | Performance | Warning | DefaultEqualsOrHashCodeUsageAnalyzer @@ -27,6 +27,7 @@ EPC32 | Async | Warning | TaskCompletionSourceRunContinuationsAnalyzer EPC33 | Async | Warning | DoNotUseThreadSleepAnalyzer EPC34 | ErrorHandling | Warning | MustUseResultAnalyzer EPC35 | Async | Warning | DoNotBlockUnnecessarilyInAsyncMethodsAnalyzer +EPC36 | Async | Warning | DoNotUseAsyncDelegatesForLongRunningTasksAnalyzer ERP021 | ErrorHandling | Warning | ThrowExAnalyzer ERP022 | ErrorHandling | Warning | SwallowAllExceptionsAnalyzer ERP031 | Concurrency | Warning | ConcurrentCollectionAnalyzer diff --git a/src/ErrorProne.NET.CoreAnalyzers/AsyncAnalyzers/DoNotUseAsyncDelegatesForLongRunningTasksAnalyzer.cs b/src/ErrorProne.NET.CoreAnalyzers/AsyncAnalyzers/DoNotUseAsyncDelegatesForLongRunningTasksAnalyzer.cs new file mode 100644 index 0000000..3871678 --- /dev/null +++ b/src/ErrorProne.NET.CoreAnalyzers/AsyncAnalyzers/DoNotUseAsyncDelegatesForLongRunningTasksAnalyzer.cs @@ -0,0 +1,159 @@ +using System.Threading.Tasks; +using ErrorProne.NET.Core; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Operations; +using ErrorProne.NET.CoreAnalyzers; + +namespace ErrorProne.NET.AsyncAnalyzers +{ + [DiagnosticAnalyzer(LanguageNames.CSharp)] + public sealed class DoNotUseAsyncDelegatesForLongRunningTasksAnalyzer : DiagnosticAnalyzerBase + { + public static readonly DiagnosticDescriptor Rule = DiagnosticDescriptors.EPC36; + + /// + public DoNotUseAsyncDelegatesForLongRunningTasksAnalyzer() + : base(Rule) + { + } + + /// + protected override void InitializeCore(AnalysisContext context) + { + context.EnableConcurrentExecution(); + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + + context.RegisterOperationAction(AnalyzeInvocation, OperationKind.Invocation); + } + + private void AnalyzeInvocation(OperationAnalysisContext context) + { + var invocation = (IInvocationOperation)context.Operation; + + // Check if this is Task.Factory.StartNew + if (!IsTaskFactoryStartNew(invocation, context.Compilation)) + { + return; + } + + // Check if TaskCreationOptions.LongRunning is used + if (!HasLongRunningOption(invocation)) + { + return; + } + + // Check if the first argument is an async delegate + if (invocation.Arguments.Length > 0 && IsAsyncDelegate(invocation.Arguments[0].Value)) + { + context.ReportDiagnostic(Diagnostic.Create(Rule, invocation.Syntax.GetLocation())); + } + } + + private static bool IsTaskFactoryStartNew(IInvocationOperation invocation, Compilation compilation) + { + var targetMethod = invocation.TargetMethod; + + if (targetMethod.Name != "StartNew") + { + return false; + } + + // Check if it's Task.Factory.StartNew + if (isTaskFactoryType(targetMethod.ReceiverType, compilation)) + { + return true; + } + + return false; + + static bool isTaskFactoryType(ITypeSymbol? type, Compilation compilation) + { + // Ignoring generic TaskFactory, because the type of the delegate + // that is accepted by it is 'Func', not 'Func>'. + // SO the async delegates can't be used there! + return type.IsClrType(compilation, typeof(TaskFactory)); + } + } + + private static bool HasLongRunningOption(IInvocationOperation invocation) + { + // Look for TaskCreationOptions.LongRunning in any of the arguments + foreach (var argument in invocation.Arguments) + { + if (ContainsLongRunningOption(argument.Value)) + { + return true; + } + } + + return false; + } + + private static bool ContainsLongRunningOption(IOperation operation) + { + switch (operation) + { + case IFieldReferenceOperation fieldRef: + return fieldRef.Field.Name == "LongRunning" && + fieldRef.Field.ContainingType?.Name == "TaskCreationOptions"; + + case IBinaryOperation binaryOp when binaryOp.OperatorKind == BinaryOperatorKind.Or: + return ContainsLongRunningOption(binaryOp.LeftOperand) || + ContainsLongRunningOption(binaryOp.RightOperand); + + case IConversionOperation conversion: + return ContainsLongRunningOption(conversion.Operand); + + default: + return false; + } + } + + private static bool IsAsyncDelegate(IOperation operation) + { + switch (operation) + { + case IAnonymousFunctionOperation anonymousFunction: + return anonymousFunction.Symbol.IsAsync; + + case IDelegateCreationOperation delegateCreation: + return IsAsyncDelegate(delegateCreation.Target); + + case IMethodReferenceOperation methodRef: + return methodRef.Method.IsAsync; + + default: + return false; + } + } + + private static bool IsAsyncLambdaOrDelegate(IAnonymousFunctionOperation anonymousFunction) + { + // Check if the lambda/delegate is declared as async + if (anonymousFunction.Syntax is AnonymousMethodExpressionSyntax anonymousMethod) + { + return anonymousMethod.AsyncKeyword.IsKind(SyntaxKind.AsyncKeyword); + } + + if (anonymousFunction.Syntax is LambdaExpressionSyntax lambda) + { + return lambda.AsyncKeyword.IsKind(SyntaxKind.AsyncKeyword); + } + + return false; + } + + private static bool IsAsyncMethodReference(IDelegateCreationOperation delegateCreation, OperationAnalysisContext context) + { + if (delegateCreation.Target is IMethodReferenceOperation methodRef) + { + return methodRef.Method.IsAsync; + } + + return false; + } + } +} diff --git a/src/ErrorProne.NET.CoreAnalyzers/DiagnosticDescriptors.cs b/src/ErrorProne.NET.CoreAnalyzers/DiagnosticDescriptors.cs index a48bd20..34396fe 100644 --- a/src/ErrorProne.NET.CoreAnalyzers/DiagnosticDescriptors.cs +++ b/src/ErrorProne.NET.CoreAnalyzers/DiagnosticDescriptors.cs @@ -289,6 +289,17 @@ internal static class DiagnosticDescriptors isEnabledByDefault: true, description: "Synchronously blocking on Tasks inside an async method can lead to deadlocks. Use 'await' instead."); + /// + public static readonly DiagnosticDescriptor EPC36 = new DiagnosticDescriptor( + nameof(EPC36), + title: "Do not use async delegates with Task.Factory.StartNew and TaskCreationOptions.LongRunning", + messageFormat: "Async delegate should not be used with TaskCreationOptions.LongRunning. Use Task.Run for async delegates or remove LongRunning for synchronous work.", + category: AsyncCategory, + defaultSeverity: DiagnosticSeverity.Warning, + isEnabledByDefault: true, + description: "TaskCreationOptions.LongRunning is intended for long-running synchronous work. Using it with async delegates wastes thread pool threads and defeats the purpose of using async methods in this case.", + helpLinkUri: GetHelpUri(nameof(EPC36))); + public static string GetHelpUri(string ruleId) { return $"https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/{ruleId}.md"; From 12b0aa7f540c3c29a424105ddc40994ef87a336b Mon Sep 17 00:00:00 2001 From: Sergey Teplyakov Date: Tue, 1 Jul 2025 15:37:04 -0700 Subject: [PATCH 2/2] Remove unused helper methods from analyzer --- ...yncDelegatesForLongRunningTasksAnalyzer.cs | 28 ------------------- 1 file changed, 28 deletions(-) diff --git a/src/ErrorProne.NET.CoreAnalyzers/AsyncAnalyzers/DoNotUseAsyncDelegatesForLongRunningTasksAnalyzer.cs b/src/ErrorProne.NET.CoreAnalyzers/AsyncAnalyzers/DoNotUseAsyncDelegatesForLongRunningTasksAnalyzer.cs index 3871678..569d52a 100644 --- a/src/ErrorProne.NET.CoreAnalyzers/AsyncAnalyzers/DoNotUseAsyncDelegatesForLongRunningTasksAnalyzer.cs +++ b/src/ErrorProne.NET.CoreAnalyzers/AsyncAnalyzers/DoNotUseAsyncDelegatesForLongRunningTasksAnalyzer.cs @@ -1,8 +1,6 @@ using System.Threading.Tasks; using ErrorProne.NET.Core; using Microsoft.CodeAnalysis; -using Microsoft.CodeAnalysis.CSharp; -using Microsoft.CodeAnalysis.CSharp.Syntax; using Microsoft.CodeAnalysis.Diagnostics; using Microsoft.CodeAnalysis.Operations; using ErrorProne.NET.CoreAnalyzers; @@ -129,31 +127,5 @@ private static bool IsAsyncDelegate(IOperation operation) return false; } } - - private static bool IsAsyncLambdaOrDelegate(IAnonymousFunctionOperation anonymousFunction) - { - // Check if the lambda/delegate is declared as async - if (anonymousFunction.Syntax is AnonymousMethodExpressionSyntax anonymousMethod) - { - return anonymousMethod.AsyncKeyword.IsKind(SyntaxKind.AsyncKeyword); - } - - if (anonymousFunction.Syntax is LambdaExpressionSyntax lambda) - { - return lambda.AsyncKeyword.IsKind(SyntaxKind.AsyncKeyword); - } - - return false; - } - - private static bool IsAsyncMethodReference(IDelegateCreationOperation delegateCreation, OperationAnalysisContext context) - { - if (delegateCreation.Target is IMethodReferenceOperation methodRef) - { - return methodRef.Method.IsAsync; - } - - return false; - } } }