diff --git a/ReadMe.md b/ReadMe.md index 0993cf6..fed3238 100644 --- a/ReadMe.md +++ b/ReadMe.md @@ -26,6 +26,7 @@ Add the following nuget package to you project: https://www.nuget.org/packages/E | [EPC31](https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/EPC31.md) | Do not return null for Task-like types | | [EPC32](https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/EPC32.md) | TaskCompletionSource should use RunContinuationsAsynchronously | | [EPC33](https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/EPC33.md) | Do not use Thread.Sleep in async methods | +| [EPC35](https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/EPC35.md) | Do not block unnecessarily in async methods | ### Generic Bugs and Code Smells diff --git a/docs/Rules/EPC35.md b/docs/Rules/EPC35.md new file mode 100644 index 0000000..7c578b3 --- /dev/null +++ b/docs/Rules/EPC35.md @@ -0,0 +1,106 @@ +# EPC35 - Do not block unnecessarily in async methods + +This analyzer detects synchronous blocking operations inside async methods that can lead to deadlocks and poor performance. + +## Description + +The analyzer warns when you use blocking operations like `.Wait()`, `.Result`, or `.GetAwaiter().GetResult()` on Task-like types inside async methods. These blocking operations defeat the purpose of async programming and can cause deadlocks, especially in UI applications or when there's a limited synchronization context. + +## Code that triggers the analyzer + +```csharp +public async Task ProcessAsync() +{ + // Using .Wait() blocks the thread + SomeAsyncMethod().Wait(); // ❌ EPC35 + + // Using .Result property blocks the thread + var result = SomeAsyncMethod().Result; // ❌ EPC35 + + // Using GetAwaiter().GetResult() blocks the thread + var data = GetDataAsync().GetAwaiter().GetResult(); // ❌ EPC35 + + // More async work + await AnotherAsyncMethod(); +} +``` + +## How to fix + +Use `await` instead of blocking operations: + +```csharp +public async Task ProcessAsync() +{ + // Use await instead of .Wait() + await SomeAsyncMethod(); // ✅ Correct + + // Use await instead of .Result + var result = await SomeAsyncMethod(); // ✅ Correct + + // Use await instead of GetAwaiter().GetResult() + var data = await GetDataAsync(); // ✅ Correct + + // More async work + await AnotherAsyncMethod(); +} +``` + +## Why this is important + +1. **Deadlock Prevention**: Blocking on async operations can cause deadlocks, especially when there's a synchronization context (like in UI applications) +2. **Performance**: Blocking ties up thread pool threads unnecessarily +3. **Scalability**: Proper async/await allows better resource utilization +4. **Consistency**: Mixing blocking and async patterns makes code harder to reason about + +## Examples of problematic patterns + +```csharp +// UI event handler - high risk of deadlock +private void Button_Click(object sender, EventArgs e) +{ + ProcessDataAsync().Wait(); // ❌ Very dangerous in UI context +} + +// ASP.NET Controller - can cause thread pool starvation +public ActionResult GetData() +{ + var result = FetchDataAsync().Result; // ❌ Blocks request thread + return Json(result); +} + +// Library method - blocks caller unnecessarily +public string GetConfiguration() +{ + return LoadConfigAsync().GetAwaiter().GetResult(); // ❌ Forces synchronous behavior +} +``` + +## Correct async patterns + +```csharp +// UI event handler - make it async +private async void Button_Click(object sender, EventArgs e) +{ + await ProcessDataAsync(); // ✅ Proper async UI pattern +} + +// ASP.NET Controller - return Task +public async Task GetData() +{ + var result = await FetchDataAsync(); // ✅ Truly async request handling + return Json(result); +} + +// Library method - keep it async +public async Task GetConfigurationAsync() +{ + return await LoadConfigAsync(); // ✅ Maintains async all the way +} +``` + +## Related rules + +- **EPC27**: Avoid async void methods (except for event handlers) +- **EPC31**: Do not return null for Task-like types +- **EPC33**: Do not use Thread.Sleep in async methods diff --git a/src/ErrorProne.NET.CoreAnalyzers.Tests/AsyncAnalyzers/DoNotBlockUnnecessarilyInAsyncMethodsAnalyzerTests.cs b/src/ErrorProne.NET.CoreAnalyzers.Tests/AsyncAnalyzers/DoNotBlockUnnecessarilyInAsyncMethodsAnalyzerTests.cs new file mode 100644 index 0000000..30054e9 --- /dev/null +++ b/src/ErrorProne.NET.CoreAnalyzers.Tests/AsyncAnalyzers/DoNotBlockUnnecessarilyInAsyncMethodsAnalyzerTests.cs @@ -0,0 +1,295 @@ +using System.Threading.Tasks; +using NUnit.Framework; +using VerifyCS = ErrorProne.NET.TestHelpers.CSharpCodeFixVerifier< + ErrorProne.NET.AsyncAnalyzers.DoNotBlockUnnecessarilyInAsyncMethodsAnalyzer, + Microsoft.CodeAnalysis.Testing.EmptyCodeFixProvider>; + + +namespace ErrorProne.NET.CoreAnalyzers.Tests.AsyncAnalyzers +{ + [TestFixture] + public class DoNotBlockUnnecessarilyInAsyncMethodsAnalyzerTests + { + [Test] + public async Task WarnOnResultInAsyncMethod() + { + string code = @" +using System.Threading.Tasks; +public class MyClass +{ + public async Task MyMethod() + { + var t = Task.FromResult(42); + var result = [|t.Result|]; + } +}"; + await VerifyCS.VerifyAsync(code); + } + + [Test] + public async Task WarnOnWaitInAsyncMethod() + { + string code = @" +using System.Threading.Tasks; +public class MyClass +{ + public async Task MyMethod() + { + var t = Task.FromResult(42); + [|t.Wait()|]; + } +}"; + await VerifyCS.VerifyAsync(code); + } + + [Test] + public async Task WarnOnGetAwaiterGetResultInAsyncMethod() + { + string code = @" +using System.Threading.Tasks; +public class MyClass +{ + public async Task MyMethod() + { + var t = Task.FromResult(42); + var result = [|t.GetAwaiter().GetResult()|]; + } +}"; + await VerifyCS.VerifyAsync(code); + } + + [Test] + public async Task NoWarnInNonAsyncMethod() + { + string code = @" +using System.Threading.Tasks; +public class MyClass +{ + public Task MyMethod() + { + var t = Task.FromResult(42); + var result = t.Result; + return t; + } +}"; + await VerifyCS.VerifyAsync(code); + } + + [Test] + public async Task NoWarnOnNonTask() + { + string code = @" +public class MyClass +{ + public class Foo { public int Result => 42; } + public async System.Threading.Tasks.Task MyMethod() + { + var f = new Foo(); + var result = f.Result; + } +}"; + await VerifyCS.VerifyAsync(code); + } + + [Test] + public async Task WarnOnResultInAsyncLambda() + { + string code = @" +using System; +using System.Threading.Tasks; +public class MyClass +{ + public void MyMethod() + { + Func asyncLambda = async () => + { + var t = Task.FromResult(42); + var result = [|t.Result|]; + }; + } +}"; + await VerifyCS.VerifyAsync(code); + } + + [Test] + public async Task WarnOnWaitInAsyncLambda() + { + string code = @" +using System; +using System.Threading.Tasks; +public class MyClass +{ + public void MyMethod() + { + Func asyncLambda = async () => + { + var t = Task.FromResult(42); + [|t.Wait()|]; + }; + } +}"; + await VerifyCS.VerifyAsync(code); + } + + [Test] + public async Task WarnOnGetAwaiterGetResultInAsyncLambda() + { + string code = @" +using System; +using System.Threading.Tasks; +public class MyClass +{ + public void MyMethod() + { + Func> asyncLambda = async () => + { + var t = Task.FromResult(42); + return [|t.GetAwaiter().GetResult()|]; + }; + } +}"; + await VerifyCS.VerifyAsync(code); + } + + [Test] + public async Task WarnOnResultInAsyncLocalMethod() + { + string code = @" +using System.Threading.Tasks; +public class MyClass +{ + public void MyMethod() + { + async Task LocalAsyncMethod() + { + var t = Task.FromResult(42); + var result = [|t.Result|]; + } + } +}"; + await VerifyCS.VerifyAsync(code); + } + + [Test] + public async Task WarnOnWaitInAsyncLocalMethod() + { + string code = @" +using System.Threading.Tasks; +public class MyClass +{ + public void MyMethod() + { + async Task LocalAsyncMethod() + { + var t = Task.FromResult(42); + [|t.Wait()|]; + } + } +}"; + await VerifyCS.VerifyAsync(code); + } + + [Test] + public async Task WarnOnGetAwaiterGetResultInAsyncLocalMethod() + { + string code = @" +using System.Threading.Tasks; +public class MyClass +{ + public void MyMethod() + { + async Task LocalAsyncMethod() + { + var t = Task.FromResult(42); + return [|t.GetAwaiter().GetResult()|]; + } + } +}"; + await VerifyCS.VerifyAsync(code); + } + + [Test] + public async Task NoWarnInNonAsyncLambda() + { + string code = @" +using System; +using System.Threading.Tasks; +public class MyClass +{ + public void MyMethod() + { + Func lambda = () => + { + var t = Task.FromResult(42); + return t.Result; + }; + } +}"; + await VerifyCS.VerifyAsync(code); + } + + [Test] + public async Task NoWarnInNonAsyncLocalMethod() + { + string code = @" +using System.Threading.Tasks; +public class MyClass +{ + public void MyMethod() + { + int LocalMethod() + { + var t = Task.FromResult(42); + return t.Result; + } + } +}"; + await VerifyCS.VerifyAsync(code); + } + + [Test] + public async Task WarnOnResultInNestedAsyncLambda() + { + string code = @" +using System; +using System.Threading.Tasks; +public class MyClass +{ + public async Task MyMethod() + { + Func asyncLambda = async () => + { + Func nestedAsyncLambda = async () => + { + var t = Task.FromResult(42); + var result = [|t.Result|]; + }; + await nestedAsyncLambda(); + }; + await asyncLambda(); + } +}"; + await VerifyCS.VerifyAsync(code); + } + + [Test] + public async Task WarnOnResultInAsyncDelegate() + { + string code = @" +using System; +using System.Threading.Tasks; +public class MyClass +{ + public void MyMethod() + { + Func asyncDelegate = async delegate() + { + var t = Task.FromResult(42); + var result = [|t.Result|]; + }; + } +}"; + await VerifyCS.VerifyAsync(code); + } + } +} diff --git a/src/ErrorProne.NET.CoreAnalyzers/AnalyzerReleases.Unshipped.md b/src/ErrorProne.NET.CoreAnalyzers/AnalyzerReleases.Unshipped.md index 5f9fb14..ca6cd2f 100644 --- a/src/ErrorProne.NET.CoreAnalyzers/AnalyzerReleases.Unshipped.md +++ b/src/ErrorProne.NET.CoreAnalyzers/AnalyzerReleases.Unshipped.md @@ -26,6 +26,7 @@ EPC31 | Async | Warning | DoNotReturnNullForTaskLikeAnalyzer EPC32 | Async | Warning | TaskCompletionSourceRunContinuationsAnalyzer EPC33 | Async | Warning | DoNotUseThreadSleepAnalyzer EPC34 | ErrorHandling | Warning | MustUseResultAnalyzer +EPC35 | Async | Warning | DoNotBlockUnnecessarilyInAsyncMethodsAnalyzer ERP021 | ErrorHandling | Warning | ThrowExAnalyzer ERP022 | ErrorHandling | Warning | SwallowAllExceptionsAnalyzer ERP031 | Concurrency | Warning | ConcurrentCollectionAnalyzer diff --git a/src/ErrorProne.NET.CoreAnalyzers/AsyncAnalyzers/DoNotBlockUnnecessarilyInAsyncMethodsAnalyzer.cs b/src/ErrorProne.NET.CoreAnalyzers/AsyncAnalyzers/DoNotBlockUnnecessarilyInAsyncMethodsAnalyzer.cs new file mode 100644 index 0000000..08376b8 --- /dev/null +++ b/src/ErrorProne.NET.CoreAnalyzers/AsyncAnalyzers/DoNotBlockUnnecessarilyInAsyncMethodsAnalyzer.cs @@ -0,0 +1,75 @@ +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Operations; +using ErrorProne.NET.Core; +using ErrorProne.NET.CoreAnalyzers; + +namespace ErrorProne.NET.AsyncAnalyzers +{ + [DiagnosticAnalyzer(LanguageNames.CSharp)] + public sealed class DoNotBlockUnnecessarilyInAsyncMethodsAnalyzer : DiagnosticAnalyzerBase + { + public static readonly DiagnosticDescriptor Rule = DiagnosticDescriptors.EPC35; + + /// + public DoNotBlockUnnecessarilyInAsyncMethodsAnalyzer() + : base(Rule) + { + } + + /// + protected override void InitializeCore(AnalysisContext context) + { + context.EnableConcurrentExecution(); + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + + context.RegisterOperationAction(AnalyzeInvocation, OperationKind.Invocation); + context.RegisterOperationAction(AnalyzePropertyReference, OperationKind.PropertyReference); + } + + private void AnalyzePropertyReference(OperationAnalysisContext context) + { + var propertyReference = (IPropertyReferenceOperation)context.Operation; + + var methodSymbol = propertyReference.FindEnclosingMethodSymbol(context); + if (methodSymbol == null || !methodSymbol.IsAsync) + { + return; + } + + if (propertyReference.Property.Name == "Result" && + propertyReference.Instance?.Type?.IsTaskLike(context.Compilation) == true) + { + context.ReportDiagnostic(Diagnostic.Create(Rule, propertyReference.Syntax.GetLocation(), propertyReference.Syntax.ToString())); + } + } + + private void AnalyzeInvocation(OperationAnalysisContext context) + { + var invocation = (IInvocationOperation)context.Operation; + + var methodSymbol = invocation.FindEnclosingMethodSymbol(context); + if (methodSymbol == null || !methodSymbol.IsAsync) + { + return; + } + + var targetMethod = invocation.TargetMethod; + + if (targetMethod.Name == "Wait" && invocation.Instance?.Type?.IsTaskLike(context.Compilation) == true) + { + context.ReportDiagnostic(Diagnostic.Create(Rule, invocation.Syntax.GetLocation(), invocation.Syntax.ToString())); + } + else if (targetMethod.Name == "GetResult") + { + // Check for GetAwaiter().GetResult() + if (invocation.Instance is IInvocationOperation getAwaiterInvocation && + getAwaiterInvocation.TargetMethod.Name == "GetAwaiter" && + getAwaiterInvocation.Instance?.Type.IsTaskLike(context.Compilation) == true) + { + context.ReportDiagnostic(Diagnostic.Create(Rule, invocation.Syntax.GetLocation(), invocation.Syntax.ToString())); + } + } + } + } +} diff --git a/src/ErrorProne.NET.CoreAnalyzers/DiagnosticDescriptors.cs b/src/ErrorProne.NET.CoreAnalyzers/DiagnosticDescriptors.cs index b2ca473..0a29107 100644 --- a/src/ErrorProne.NET.CoreAnalyzers/DiagnosticDescriptors.cs +++ b/src/ErrorProne.NET.CoreAnalyzers/DiagnosticDescriptors.cs @@ -279,6 +279,16 @@ internal static class DiagnosticDescriptors description: "The event source implementation must follow special rules to avoid hitting runtime errors.", helpLinkUri: GetHelpUri(nameof(ERP042))); + /// + public static readonly DiagnosticDescriptor EPC35 = new DiagnosticDescriptor( + "EPC35", + title: "Do not block unnecessarily in async methods", + messageFormat: "'{0}' is blocking and should not be used in an async method. Use 'await' instead.", + category: AsyncCategory, + defaultSeverity: DiagnosticSeverity.Warning, + isEnabledByDefault: true, + description: "Synchronously blocking on Tasks inside an async method can lead to deadlocks. Use 'await' instead."); + public static string GetHelpUri(string ruleId) { return $"https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/{ruleId}.md"; diff --git a/src/ErrorProne.NET.CoreAnalyzers/TaskTypeExtensions.cs b/src/ErrorProne.NET.CoreAnalyzers/TaskTypeExtensions.cs index 7dc2c3c..c6a4b77 100644 --- a/src/ErrorProne.NET.CoreAnalyzers/TaskTypeExtensions.cs +++ b/src/ErrorProne.NET.CoreAnalyzers/TaskTypeExtensions.cs @@ -33,9 +33,9 @@ public static TaskLikeTypesHolder GetTaskTypes(Compilation compilation) return new(taskType, taskOfTType, valueTaskType, valueTaskOfTType); } - public static bool IsTaskLike(this ITypeSymbol? returnType, Compilation compilation, TaskLikeTypes typesToCheck) + public static bool IsTaskLike(this ITypeSymbol? type, Compilation compilation, TaskLikeTypes typesToCheck) { - if (returnType == null) + if (type == null) { return false; } @@ -46,37 +46,37 @@ public static bool IsTaskLike(this ITypeSymbol? returnType, Compilation compilat return false; // ? } - if ((typesToCheck & TaskLikeTypes.Task) != 0 && returnType.Equals(taskType, SymbolEqualityComparer.Default)) + if ((typesToCheck & TaskLikeTypes.Task) != 0 && type.Equals(taskType, SymbolEqualityComparer.Default)) { return true; } - if ((typesToCheck & TaskLikeTypes.TaskOfT) != 0 && returnType.OriginalDefinition.Equals(taskOfTType, SymbolEqualityComparer.Default)) + if ((typesToCheck & TaskLikeTypes.TaskOfT) != 0 && type.OriginalDefinition.Equals(taskOfTType, SymbolEqualityComparer.Default)) { return true; } - if ((typesToCheck & TaskLikeTypes.ValueTask) != 0 && returnType.Equals(valueTaskType, SymbolEqualityComparer.Default)) + if ((typesToCheck & TaskLikeTypes.ValueTask) != 0 && type.Equals(valueTaskType, SymbolEqualityComparer.Default)) { return true; } - if ((typesToCheck & TaskLikeTypes.ValueTaskOfT) != 0 && returnType.OriginalDefinition.Equals(valueTaskOfTType, SymbolEqualityComparer.Default)) + if ((typesToCheck & TaskLikeTypes.ValueTaskOfT) != 0 && type.OriginalDefinition.Equals(valueTaskOfTType, SymbolEqualityComparer.Default)) { return true; } - if (returnType.IsErrorType()) + if (type.IsErrorType()) { - return returnType.Name.Equals("Task") || - returnType.Name.Equals("ValueTask"); + return type.Name.Equals("Task") || + type.Name.Equals("ValueTask"); } return false; } - public static bool IsTaskLike(this ITypeSymbol? returnType, Compilation compilation) => - IsTaskLike(returnType, compilation, TaskLikeTypes.All); + public static bool IsTaskLike(this ITypeSymbol? type, Compilation compilation) => + IsTaskLike(type, compilation, TaskLikeTypes.All); public static bool IsTaskCompletionSource(this ITypeSymbol? type, Compilation compilation) {