Add analyzer to prevent use of blocking synchronous APIs #96
There are no files selected for viewing
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
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,79 @@ | ||
| --- | ||
| title: SyncBlocking Analyzer | ||
| category: analyzers | ||
| categoryindex: 1 | ||
| index: | ||
| --- | ||
|
|
||
| # SyncBlocking Analyzer | ||
|
|
||
| ## Problem | ||
|
|
||
| Calls to blocking methods and properties like `Task.Wait` and `Task.Result`, or `Async.RunSynchronously`, consume a thread in the thread pool for as long as they are running. | ||
| The .NET runtime tries to cope with this by spinning up new operating system threads if it detects thread pool starvation, but [there is a maximum number of threads](https://learn.microsoft.com/en-us/dotnet/standard/threading/the-managed-thread-pool#maximum-number-of-thread-pool-threads), and attempting to exceed this number will cause the application to deadlock once all threads are blocked waiting for a synchronous call to complete. | ||
|
|
||
| ```fsharp | ||
| let doTheThing (input : Task<int>) : int = | ||
| // This line blocks | ||
| let input = input.Result | ||
| input + 1 | ||
| ``` | ||
|
|
||
| ## Fix | ||
|
|
||
| The correct fix depends on context. | ||
|
|
||
| ## In a library function | ||
|
|
||
| Usually the correct answer is to propagate the asynchronous nature of the workflow up into the function signature: | ||
|
|
||
| ```fsharp | ||
| let doTheThing (input : Task<int>) : Task<int> = | ||
| task { | ||
| // Correct: await the asynchronous workflow | ||
| let! input = input | ||
| return input + 1 | ||
| } | ||
| ``` | ||
|
|
||
| ## In the main method | ||
| F# [does not support async main methods](https://github.com/dotnet/fsharp/issues/11631#issuecomment-855052325), so a blocking call will always be necessary in the entry point. | ||
|
|
||
| ```fsharp | ||
| [<EntryPoint>] | ||
| let main argv = | ||
| doTheThing () | ||
| // ANALYZER: synchronous blocking call allowed in entry point | ||
| |> Async.RunSynchronously | ||
| ``` | ||
|
|
||
| ## In tests | ||
|
|
||
| Many standard test runners support asynchronous tests out of the box. | ||
|
|
||
| Avoid synchronous blocking in tests. | ||
| Test runners may give you a much more restrictive thread pool than you are used to; this can result in heavy slowdowns or deadlocks in tests even when you wouldn't expect to see them in prod. | ||
|
|
||
| ```fsharp | ||
| open NUnit.Framework | ||
|
|
||
| // NUnit supports Task and Async out of the box, for example | ||
| [<Test>] | ||
| let myAsyncTest () = task { | ||
| let! result = computeMyResult () | ||
| result |> shouldEqual 8 | ||
| } | ||
| ``` | ||
|
|
||
| ## If you already have a proof that the access is safe | ||
|
|
||
| You might have some proof that a `Task` has already completed at the time you want to access it. | ||
| In that case, just suppress the analyzer at the point of access. | ||
|
|
||
| ```fsharp | ||
| if myTask.IsCompletedSuccessfully then | ||
| // SAFETY: synchronous blocking call allowed because task has completed | ||
| myTask.Result | ||
| else | ||
| ... | ||
| ``` |
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
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
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,123 @@ | ||
| module GR.FSharp.Analyzers.SyncBlockingAnalyzer | ||
|
|
||
| open System | ||
| open FSharp.Analyzers.Comments | ||
| open FSharp.Analyzers.SDK | ||
| open FSharp.Analyzers.SDK.TASTCollecting | ||
| open FSharp.Compiler.CodeAnalysis | ||
| open FSharp.Compiler.Symbols | ||
| open FSharp.Compiler.Syntax | ||
| open FSharp.Compiler.SyntaxTrivia | ||
| open FSharp.Compiler.Text | ||
|
|
||
| [<Literal>] | ||
| let Code = "GRA-SYNCBLOCK-001" | ||
|
|
||
| [<Literal>] | ||
| let SwitchOffComment = "synchronous blocking call allowed" | ||
|
|
||
| let problematicMethods = | ||
| [ | ||
| "Microsoft.FSharp.Control.RunSynchronously" // This is how it appears in the TAST | ||
| "System.Threading.Tasks.Task.Wait" | ||
| "System.Threading.Tasks.Task.WaitAll" | ||
| "System.Threading.Tasks.Task.WaitAny" | ||
| "System.Runtime.CompilerServices.TaskAwaiter.GetResult" | ||
| "System.Runtime.CompilerServices.TaskAwaiter`1.GetResult" | ||
| "System.Runtime.CompilerServices.ValueTaskAwaiter.GetResult" | ||
| "System.Runtime.CompilerServices.ValueTaskAwaiter`1.GetResult" | ||
| ] | ||
| |> Set.ofList | ||
|
|
||
| let problematicProperties = | ||
| [ | ||
| "System.Threading.Tasks.Task.get_Result" // Note: F# doesn't include `1 in FullName for generic type property getters | ||
| "System.Threading.Tasks.ValueTask.get_Result" // Same here | ||
| ] | ||
| |> Set.ofList | ||
|
|
||
| let analyze (sourceText : ISourceText) (ast : ParsedInput) (checkFileResults : FSharpCheckFileResults) = | ||
| let comments = | ||
| match ast with | ||
| | ParsedInput.ImplFile parsedImplFileInput -> parsedImplFileInput.Trivia.CodeComments | ||
| | _ -> [] | ||
|
|
||
| let violations = ResizeArray<range * string * string> () | ||
|
|
||
| let walker = | ||
| { new TypedTreeCollectorBase() with | ||
| override _.WalkCall _ (mfv : FSharpMemberOrFunctionOrValue) _ _ _ (m : range) = | ||
|
|
||
| // Check for regular method calls | ||
| if problematicMethods.Contains mfv.FullName then | ||
| if not (isSwitchedOffPerComment SwitchOffComment comments sourceText m) then | ||
| let methodName = | ||
| if mfv.DisplayName.Contains '.' then | ||
| mfv.DisplayName | ||
| elif mfv.FullName = "Microsoft.FSharp.Control.RunSynchronously" then | ||
| // Special handling for Async.RunSynchronously, which has a different name in the TAST | ||
| "Async.RunSynchronously" | ||
| else | ||
| // Get more context for better error messages | ||
| let parts = mfv.FullName.Split '.' | ||
|
|
||
| if parts.Length >= 2 then | ||
| $"{parts.[parts.Length - 2]}.{parts.[parts.Length - 1]}" | ||
| else | ||
| mfv.DisplayName | ||
|
|
||
| violations.Add (m, "method", methodName) | ||
|
|
||
| // Check for property getters (Result property access) | ||
| elif problematicProperties.Contains mfv.FullName then | ||
| if not (isSwitchedOffPerComment SwitchOffComment comments sourceText m) then | ||
| // For property getters, use just the property name | ||
| violations.Add (m, "property", "Result") | ||
| } | ||
|
|
||
| match checkFileResults.ImplementationFile with | ||
| | Some typedTree -> walkTast walker typedTree | ||
| | None -> () | ||
|
|
||
| violations | ||
| |> Seq.map (fun (range, _kind, name) -> | ||
| { | ||
| Type = "SyncBlockingAnalyzer" | ||
| Message = | ||
| $"Synchronous blocking call '%s{name}' should be avoided. " | ||
| + "This can cause deadlocks and thread pool starvation. " | ||
| + "Consider using `let!` in a `task` or `async` computation expression. " | ||
| + "Suppress with comment including text 'synchronous blocking call allowed'." | ||
| Code = Code | ||
| Severity = Severity.Warning | ||
| Range = range | ||
| Fixes = [] | ||
| } | ||
| ) | ||
| |> Seq.toList | ||
|
|
||
| [<Literal>] | ||
| let Name = "SyncBlockingAnalyzer" | ||
|
|
||
| [<Literal>] | ||
| let ShortDescription = | ||
| "Bans synchronous blocking operations like Task.Result and Async.RunSynchronously" | ||
|
|
||
| [<Literal>] | ||
| let HelpUri = | ||
| "https://g-research.github.io/fsharp-analyzers/analyzers/SyncBlockingAnalyzer.html" | ||
|
|
||
| [<CliAnalyzer(Name, ShortDescription, HelpUri)>] | ||
| let syncBlockingCliAnalyzer : Analyzer<CliContext> = | ||
| fun (ctx : CliContext) -> | ||
| async { return analyze ctx.SourceText ctx.ParseFileResults.ParseTree ctx.CheckFileResults } | ||
|
|
||
| [<EditorAnalyzer(Name, ShortDescription, HelpUri)>] | ||
| let syncBlockingEditorAnalyzer : Analyzer<EditorContext> = | ||
| fun (ctx : EditorContext) -> | ||
| async { | ||
| return | ||
| ctx.CheckFileResults | ||
| |> Option.map (analyze ctx.SourceText ctx.ParseFileResults.ParseTree) | ||
| |> Option.defaultValue [] | ||
| } | ||
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 |
|---|---|---|
| @@ -1,41 +1,42 @@ | ||
| <?xml version="1.0" encoding="utf-8"?> | ||
| <Project Sdk="Microsoft.NET.Sdk"> | ||
|
Contributor
Author
There was a problem hiding this comment. The scaffolding script reformatted this file. |
||
| <PropertyGroup> | ||
| <TargetFramework>net8.0</TargetFramework> | ||
| <IsPackable>false</IsPackable> | ||
| <GenerateProgramFile>false</GenerateProgramFile> | ||
| <IsTestProject>true</IsTestProject> | ||
| <AssemblyName>G-Research.FSharp.Analyzers.Tests</AssemblyName> | ||
| </PropertyGroup> | ||
| <ItemGroup> | ||
| <Compile Include="Common.fs" /> | ||
| <Compile Include="StringAnalyzerTests.fs" /> | ||
| <Compile Include="JsonSerializerOptionsAnalyzerTests.fs" /> | ||
| <Compile Include="UnionCaseAnalyzerTests.fs" /> | ||
| <Compile Include="VirtualCallAnalyzerTests.fs" /> | ||
| <Compile Include="LoggingArgFuncNotFullyAppliedAnalyzerTests.fs" /> | ||
| <Compile Include="LoggingTemplateMissingValuesAnalyzerTests.fs" /> | ||
| <Compile Include="TypeAnnotateStringFunctionAnalyzerTests.fs" /> | ||
| <Compile Include="ImmutableCollectionEqualityAnalyzerTests.fs" /> | ||
| <Compile Include="TypedInterpolatedStringsAnalyzerTests.fs" /> | ||
| <Compile Include="DisposedBeforeAsyncRunAnalyzerTests.fs" /> | ||
| <Compile Include="SyncBlockingAnalyzerTests.fs" /> | ||
|
Contributor
Author
There was a problem hiding this comment. This is the only actually-changed line, and it's new. |
||
| </ItemGroup> | ||
| <ItemGroup> | ||
| <None Include="data\**\*.fs*" /> | ||
| </ItemGroup> | ||
| <ItemGroup> | ||
| <PackageReference Update="FSharp.Core" /> | ||
| <PackageReference Include="Microsoft.NET.Test.Sdk" /> | ||
| <PackageReference Include="NUnit" /> | ||
| <PackageReference Include="NUnit3TestAdapter" /> | ||
| </ItemGroup> | ||
| <ItemGroup Condition="'$(UseLocalAnalyzersSDK)' == 'false'"> | ||
| <PackageReference Include="FSharp.Analyzers.SDK.Testing" /> | ||
| </ItemGroup> | ||
| <ItemGroup> | ||
| <ProjectReference Include="..\..\src\FSharp.Analyzers\FSharp.Analyzers.fsproj" /> | ||
| </ItemGroup> | ||
| <ItemGroup Condition="'$(UseLocalAnalyzersSDK)' == 'true'"> | ||
| <ProjectReference Include="$(LocalAnalyzersSDKRepo)/src/FSharp.Analyzers.SDK.Testing/FSharp.Analyzers.SDK.Testing.fsproj" /> | ||
| </ItemGroup> | ||
| </Project> | ||
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,56 @@ | ||
| module GR.FSharp.Analyzers.Tests.SyncBlockingAnalyzerTests | ||
|
|
||
| open System.Collections | ||
| open System.IO | ||
| open NUnit.Framework | ||
| open FSharp.Compiler.CodeAnalysis | ||
| open FSharp.Analyzers.SDK.Testing | ||
| open GR.FSharp.Analyzers | ||
| open GR.FSharp.Analyzers.Tests.Common | ||
|
|
||
| let mutable projectOptions : FSharpProjectOptions = FSharpProjectOptions.zero | ||
|
|
||
| [<SetUp>] | ||
| let Setup () = | ||
| task { | ||
| let! options = mkOptionsFromProject framework [] | ||
| projectOptions <- options | ||
| } | ||
|
|
||
| type TestCases() = | ||
|
|
||
| interface IEnumerable with | ||
| member _.GetEnumerator () : IEnumerator = | ||
| constructTestCaseEnumerator [| "syncBlocking" |] | ||
|
|
||
| [<TestCaseSource(typeof<TestCases>)>] | ||
| let SyncBlockingAnalyzerTests (fileName : string) = | ||
| task { | ||
| let fileName = Path.Combine (dataFolder, fileName) | ||
|
|
||
| let! messages = | ||
| File.ReadAllText fileName | ||
| |> getContext projectOptions | ||
| |> SyncBlockingAnalyzer.syncBlockingCliAnalyzer | ||
|
|
||
| do! assertExpected fileName messages | ||
| } | ||
|
|
||
| type NegativeTestCases() = | ||
|
|
||
| interface IEnumerable with | ||
| member _.GetEnumerator () : IEnumerator = | ||
| constructTestCaseEnumerator [| "syncBlocking" ; "negative" |] | ||
|
|
||
| [<TestCaseSource(typeof<NegativeTestCases>)>] | ||
| let NegativeTests (fileName : string) = | ||
| task { | ||
| let fileName = Path.Combine (dataFolder, fileName) | ||
|
|
||
| let! messages = | ||
| File.ReadAllText fileName | ||
| |> getContext projectOptions | ||
| |> SyncBlockingAnalyzer.syncBlockingCliAnalyzer | ||
|
|
||
| Assert.That (messages, Is.Empty) | ||
| } |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the scope of this comment the next line or something broader (or narrower)?
For example, would I have to do?:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's currently specific to the line immediately before. We could totally do better.