From eab74f713d0ef48c53867bfa314dab5dda687296 Mon Sep 17 00:00:00 2001 From: Sergey Teplyakov Date: Tue, 1 Jul 2025 10:29:18 -0700 Subject: [PATCH 1/2] Add analyzer to warn on blocking in async methods Introduces DoNotBlockUnnecessarilyInAsyncMethodsAnalyzer (EPC35) to detect and warn when Task.Result, Task.Wait(), or GetAwaiter().GetResult() are used inside async methods, which can cause deadlocks. Adds corresponding diagnostic descriptor, updates analyzer release notes, and provides comprehensive unit tests. Refactors TaskTypeExtensions for improved clarity and consistency. --- ...nnecessarilyInAsyncMethodsAnalyzerTests.cs | 295 ++++++++++++++++++ .../AnalyzerReleases.Unshipped.md | 1 + ...lockUnnecessarilyInAsyncMethodsAnalyzer.cs | 75 +++++ .../DiagnosticDescriptors.cs | 10 + .../TaskTypeExtensions.cs | 22 +- 5 files changed, 392 insertions(+), 11 deletions(-) create mode 100644 src/ErrorProne.NET.CoreAnalyzers.Tests/AsyncAnalyzers/DoNotBlockUnnecessarilyInAsyncMethodsAnalyzerTests.cs create mode 100644 src/ErrorProne.NET.CoreAnalyzers/AsyncAnalyzers/DoNotBlockUnnecessarilyInAsyncMethodsAnalyzer.cs 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 8fd3f9e..559dd8c 100644 --- a/src/ErrorProne.NET.CoreAnalyzers/AnalyzerReleases.Unshipped.md +++ b/src/ErrorProne.NET.CoreAnalyzers/AnalyzerReleases.Unshipped.md @@ -26,6 +26,7 @@ EPC31 | CodeSmell | Warning | DoNotReturnNullForTaskLikeAnalyzer EPC32 | CodeSmell | Warning | TaskCompletionSourceRunContinuationsAnalyzer EPC33 | CodeSmell | Warning | DoNotUseThreadSleepAnalyzer EPC34 | CodeSmell | Warning | MustUseResultAnalyzer +EPC35 | CodeSmell | Warning | DoNotBlockUnnecessarilyInAsyncMethodsAnalyzer: Warns when an async method blocks on a Task. ERP021 | CodeSmell | Warning | ThrowExAnalyzer ERP022 | CodeSmell | 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 16b98cd..97f640c 100644 --- a/src/ErrorProne.NET.CoreAnalyzers/DiagnosticDescriptors.cs +++ b/src/ErrorProne.NET.CoreAnalyzers/DiagnosticDescriptors.cs @@ -247,5 +247,15 @@ internal static class DiagnosticDescriptors defaultSeverity: DiagnosticSeverity.Warning, isEnabledByDefault: true, description: "Methods marked with MustUseResultAttribute return values that should always be observed and used."); + + /// + 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: CodeSmellCategory, + defaultSeverity: DiagnosticSeverity.Warning, + isEnabledByDefault: true, + description: "Synchronously blocking on Tasks inside an async method can lead to deadlocks. Use 'await' instead."); } } \ No newline at end of file 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) { From 226ba0a923711e62fde880cb5d66a41019956e70 Mon Sep 17 00:00:00 2001 From: Sergey Teplyakov Date: Tue, 1 Jul 2025 13:15:18 -0700 Subject: [PATCH 2/2] Added docs and fix merge conflict in DiagnosticDescriptor --- ReadMe.md | 1 + docs/Rules/EPC35.md | 106 ++++++++++++++++++ .../DiagnosticDescriptors.cs | 36 +----- 3 files changed, 109 insertions(+), 34 deletions(-) create mode 100644 docs/Rules/EPC35.md 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/DiagnosticDescriptors.cs b/src/ErrorProne.NET.CoreAnalyzers/DiagnosticDescriptors.cs index 0bea927..0a29107 100644 --- a/src/ErrorProne.NET.CoreAnalyzers/DiagnosticDescriptors.cs +++ b/src/ErrorProne.NET.CoreAnalyzers/DiagnosticDescriptors.cs @@ -279,51 +279,19 @@ internal static class DiagnosticDescriptors description: "The event source implementation must follow special rules to avoid hitting runtime errors.", helpLinkUri: GetHelpUri(nameof(ERP042))); -<<<<<<< HEAD - /// - public static readonly DiagnosticDescriptor EPC32 = new DiagnosticDescriptor( - nameof(EPC32), - title: "TaskCompletionSource should use RunContinuationsAsynchronously", - messageFormat: "TaskCompletionSource instance should be created with TaskCreationOptions.RunContinuationsAsynchronously", - category: CodeSmellCategory, - defaultSeverity: DiagnosticSeverity.Warning, - isEnabledByDefault: true, - description: "Always use TaskCreationOptions.RunContinuationsAsynchronously when creating TaskCompletionSource to avoid potential deadlocks."); - - /// - public static readonly DiagnosticDescriptor EPC33 = new DiagnosticDescriptor( - nameof(EPC33), - title: "Do not use Thread.Sleep in async methods", - messageFormat: "Thread.Sleep should not be used in async methods. Use 'await Task.Delay()' instead.", - category: CodeSmellCategory, - defaultSeverity: DiagnosticSeverity.Warning, - isEnabledByDefault: true, - description: "Thread.Sleep blocks the thread and defeats the purpose of async programming. Use Task.Delay with await instead."); - - /// - public static readonly DiagnosticDescriptor EPC34 = new DiagnosticDescriptor( - nameof(EPC34), - title: "Method return value marked with MustUseResultAttribute must be used", - messageFormat: "The return value of method '{0}' marked with MustUseResultAttribute must be used", - category: CodeSmellCategory, - defaultSeverity: DiagnosticSeverity.Warning, - isEnabledByDefault: true, - description: "Methods marked with MustUseResultAttribute return values that should always be observed and used."); - /// 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: CodeSmellCategory, + 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"; } ->>>>>>> master } } \ No newline at end of file