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..569d52a --- /dev/null +++ b/src/ErrorProne.NET.CoreAnalyzers/AsyncAnalyzers/DoNotUseAsyncDelegatesForLongRunningTasksAnalyzer.cs @@ -0,0 +1,131 @@ +using System.Threading.Tasks; +using ErrorProne.NET.Core; +using Microsoft.CodeAnalysis; +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; + } + } + } +} 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";