FCS: fix races that made the background builder repeat work

docs/release-notes/.FSharp.Compiler.Service/11.0.100.md

@@ -147,6 +147,7 @@
 * Import: Don't walk non-F# assemblies when labelling trait constraint sources (PR [#20090](https://github.com/dotnet/fsharp/pull/20090))
 * Avoid per-instance lock object in InterruptibleLazy and DelayInitArrayMap (PR [#20088](https://github.com/dotnet/fsharp/pull/20088))
 * IL: fix leaking binary view ([PR #20250](https://github.com/dotnet/fsharp/pull/20250))
+* FCS: fix races that made the background builder repeat work ([PR #20481](https://github.com/dotnet/fsharp/pull/20481))
 
 ### Added
 

src/Compiler/Service/BackgroundCompiler.fs

@@ -416,12 +416,21 @@ type internal BackgroundCompiler
 
     // STATIC ROOT: FSharpLanguageServiceTestable.FSharpChecker.parseFileInProjectCache. Most recently used cache for parsing files.
     let parseFileCache =
-        MruCache<ParseCacheLockToken, _ * SourceTextHash * _, _>(
+        MruCache<ParseCacheLockToken, _ * SourceTextHash * _, FSharpParseFileResults>(
             parseFileCacheSize,
             areSimilar = AreSimilarForParsing,
             areSame = AreSameForParsing
         )
 
+    /// Parses that have not finished yet. They are kept apart from parseFileCache because it holds its older entries weakly,
+    /// and a node is not kept alive by the result its caller retains.
+    let parseFileInFlight =
+        MruCache<ParseCacheLockToken, _ * SourceTextHash * _, GraphNode<FSharpParseFileResults>>(
+            parseFileCacheSize,
+            areSimilar = AreSameForParsing,
+            areSame = AreSameForParsing
+        )
+
     // STATIC ROOT: FSharpLanguageServiceTestable.FSharpChecker.checkFileInProjectCache
     //
     /// Cache which holds recently seen type-checks.
@@ -455,9 +464,6 @@ type internal BackgroundCompiler
     let tryGetBuilderNode options =
         incrementalBuildersCache.TryGet(AnyCallerThread, options)
 
-    let tryGetBuilder options : Async<IncrementalBuilder option * FSharpDiagnostic[]> option =
-        tryGetBuilderNode options |> Option.map (fun x -> x.GetOrComputeValue())
-
     let tryGetSimilarBuilder options : Async<IncrementalBuilder option * FSharpDiagnostic[]> option =
         incrementalBuildersCache.TryGetSimilar(AnyCallerThread, options)
         |> Option.map (fun x -> x.GetOrComputeValue())
@@ -475,20 +481,28 @@ type internal BackgroundCompiler
                 incrementalBuildersCache.Set(AnyCallerThread, options, getBuilderNode)
                 getBuilderNode)
 
-    let createAndGetBuilder (options, userOpName) =
+    /// Replaces the builder node the caller has observed (if any), unless a concurrent request already did so,
+    /// in which case that node is reused instead of creating a second builder for the same project.
+    let createAndGetBuilder (options, userOpName, observedNode: GraphNode<_> option) =
         async {
             let! ct = Async.CancellationToken
-            let getBuilderNode = createBuilderNode (options, userOpName, ct)
+
+            let getBuilderNode =
+                lock gate (fun () ->
+                    match tryGetBuilderNode options with
+                    | Some node when not (observedNode |> Option.contains node) -> node
+                    | _ -> createBuilderNode (options, userOpName, ct))
+
             return! getBuilderNode.GetOrComputeValue()
         }
 
     let getOrCreateBuilder (options, userOpName) : Async<IncrementalBuilder option * FSharpDiagnostic[]> =
         async {
             use! _holder = Cancellable.UseToken()
 
-            match tryGetBuilder options with
-            | Some getBuilder ->
-                match! getBuilder with
+            match tryGetBuilderNode options with
+            | Some node ->
+                match! node.GetOrComputeValue() with
                 | builderOpt, creationDiags when builderOpt.IsNone || not builderOpt.Value.IsReferencesInvalidated ->
                     return builderOpt, creationDiags
                 | _ ->
@@ -502,8 +516,8 @@ type internal BackgroundCompiler
                             let key = (sourceFile, 0L, options)
                             checkFileInProjectCache.RemoveAnySimilar(ltok, key)))
 
-                    return! createAndGetBuilder (options, userOpName)
-            | _ -> return! createAndGetBuilder (options, userOpName)
+                    return! createAndGetBuilder (options, userOpName, Some node)
+            | None -> return! createAndGetBuilder (options, userOpName, None)
         }
 
     let getSimilarOrCreateBuilder (options, userOpName) =
@@ -560,6 +574,32 @@ type internal BackgroundCompiler
                 checkFileInProjectCache.Set(ltok, key, res)
                 res)
 
+    /// Ensures there is one parse per file, source and options while it runs; a finished parse lives in parseFileCache.
+    let getParseFileNode (key, parse: Async<FSharpParseFileResults>) =
+        parseCacheLock.AcquireLock(fun ltok ->
+            match parseFileCache.TryGet(ltok, key) with
+            | Some res -> GraphNode.FromResult res
+            | None ->
+                match parseFileInFlight.TryGet(ltok, key) with
+                | Some node -> node
+                | None ->
+                    Interlocked.Increment(&actualParseFileCount) |> ignore
+
+                    let node =
+                        GraphNode(
+                            async {
+                                try
+                                    let! res = parse
+                                    parseCacheLock.AcquireLock(fun ltok -> parseFileCache.Set(ltok, key, res))
+                                    return res
+                                finally
+                                    parseCacheLock.AcquireLock(fun ltok -> parseFileInFlight.RemoveAnySimilar(ltok, key))
+                            }
+                        )
+
+                    parseFileInFlight.Set(ltok, key, node)
+                    node)
+
     member _.ParseFile
         (fileName: string, sourceText: ISourceText, options: FSharpParsingOptions, cache: bool, flatErrors: bool, userOpName: string)
         =
@@ -573,13 +613,8 @@ type internal BackgroundCompiler
                         Activity.Tags.cache, cache.ToString()
                     |]
 
-            if cache then
-                let hash = sourceText.GetHashCode() |> int64
-
-                match parseCacheLock.AcquireLock(fun ltok -> parseFileCache.TryGet(ltok, (fileName, hash, options))) with
-                | Some res -> return res
-                | None ->
-                    Interlocked.Increment(&actualParseFileCount) |> ignore
+            let parse suggestNamesForErrors =
+                async {
                     let! ct = Async.CancellationToken
 
                     let parseDiagnostics, parseTree, anyErrors =
@@ -594,27 +629,15 @@ type internal BackgroundCompiler
                             ct
                         )
 
-                    let res =
-                        FSharpParseFileResults(parseDiagnostics, parseTree, anyErrors, options.SourceFiles)
+                    return FSharpParseFileResults(parseDiagnostics, parseTree, anyErrors, options.SourceFiles)
+                }
 
-                    parseCacheLock.AcquireLock(fun ltok -> parseFileCache.Set(ltok, (fileName, hash, options), res))
-                    return res
+            if cache then
+                let key = (fileName, sourceText.GetHashCode() |> int64, options)
+                let node = getParseFileNode (key, parse suggestNamesForErrors)
+                return! node.GetOrComputeValue()
             else
-                let! ct = Async.CancellationToken
-
-                let parseDiagnostics, parseTree, anyErrors =
-                    ParseAndCheckFile.parseFile (
-                        sourceText,
-                        fileName,
-                        options,
-                        userOpName,
-                        false,
-                        flatErrors,
-                        captureIdentifiersWhenParsing,
-                        ct
-                    )
-
-                return FSharpParseFileResults(parseDiagnostics, parseTree, anyErrors, options.SourceFiles)
+                return! parse false
         }
 
     /// Fetch the parse information from the background compiler (which checks w.r.t. the FileSystem API)

src/Compiler/Service/IncrementalBuild.fs

@@ -1206,21 +1206,20 @@ type IncrementalBuilder(initialState: IncrementalBuilderInitialState, state: Inc
 
     let mutable currentState = state
 
-    let setCurrentState state cache (ct: CancellationToken) =
+    let updateCurrentState (update: IncrementalBuilderState -> IncrementalBuilderState) cache =
         async {
+            let! ct = Async.CancellationToken
             do! semaphore.WaitAsync(ct) |> Async.AwaitTask
             try
                 ct.ThrowIfCancellationRequested()
-                currentState <- computeStampedFileNames initialState state cache
+                // Read the state only under the lock: concurrent updates starting from the same stale snapshot
+                // would each build a fresh chain of bound models and lose each other's notifications.
+                currentState <- computeStampedFileNames initialState (update currentState) cache
             finally
                 semaphore.Release() |> ignore
         }
 
-    let checkFileTimeStamps (cache: TimeStampCache) =
-        async {
-            let! ct = Async.CancellationToken
-            do! setCurrentState currentState cache ct
-        }
+    let checkFileTimeStamps (cache: TimeStampCache) = updateCurrentState id cache
 
     do IncrementalBuilderEventTesting.MRU.Add(IncrementalBuilderEventTesting.IBECreated)
 
@@ -1400,11 +1399,9 @@ type IncrementalBuilder(initialState: IncrementalBuilderInitialState, state: Inc
         async {
             let slotOfFile = builder.GetSlotOfFileName fileName
             let cache = TimeStampCache defaultTimeStamp
-            let! ct = Async.CancellationToken
-            do! setCurrentState
-                    { currentState with
-                        slots = currentState.slots |> List.updateAt slotOfFile (currentState.slots[slotOfFile].Notify timeStamp) }
-                    cache ct
+            do! updateCurrentState
+                    (fun state -> { state with slots = state.slots |> List.updateAt slotOfFile (state.slots[slotOfFile].Notify timeStamp) })
+                    cache
         }
 
     member _.SourceFiles = fileNames |> Seq.map (fun f -> f.Source.FilePath) |> List.ofSeq

tests/FSharp.Compiler.Service.Tests/FSharp.Compiler.Service.Tests.fsproj

@@ -46,6 +46,7 @@
     <Compile Include="FileSystemTests.fs" />
     <Compile Include="ProjectAnalysisTests.fs" />
     <Compile Include="MultiProjectAnalysisTests.fs" />
+    <Compile Include="IncrementalBuilderRaceTests.fs" />
     <Compile Include="PerfTests.fs" />
     <Compile Include="InteractiveCheckerTests.fs" />
     <Compile Include="ExprTests.fs" />

tests/FSharp.Compiler.Service.Tests/IncrementalBuilderRaceTests.fs

@@ -0,0 +1,74 @@
+// Copyright (c) Microsoft Corporation.  All Rights Reserved.  See License.txt in the project root for license information.
+module FSharp.Compiler.Service.Tests.IncrementalBuilderRaceTests
+
+open System
+open System.IO
+open System.Collections.Concurrent
+open FSharp.Compiler.CodeAnalysis
+open FSharp.Compiler.Text
+open FSharp.Compiler.Service.Tests.Common
+open FSharp.Test.Assert
+open Xunit
+
+[<Fact>]
+let ``Concurrent requests after a file change type check each file once`` () =
+    // A private checker because we subscribe to FileChecked. The incremental builder is what is under test.
+    let checker = FSharpChecker.Create(useTransparentCompiler = false)
+
+    let dir = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName())
+    Directory.CreateDirectory dir |> ignore
+
+    try
+        let fileNames = [| for i in 1 .. 5 -> Path.Combine(dir, $"File{i}.fs") |]
+        fileNames |> Array.iteri (fun i fileName -> File.WriteAllText(fileName, $"module File{i + 1}\nlet x = 1\n"))
+
+        let dllName = Path.Combine(dir, "Project.dll")
+        let projFileName = Path.Combine(dir, "Project.fsproj")
+        let args = mkProjectCommandLineArgs (dllName, fileNames)
+        let options = { checker.GetProjectOptionsFromCommandLineArgs(projFileName, args) with SourceFiles = fileNames }
+
+        let checkCounts = ConcurrentDictionary<string, int>()
+        checker.FileChecked.Add(fun (fileName, _) -> checkCounts.AddOrUpdate(fileName, 1, (fun _ n -> n + 1)) |> ignore)
+
+        let checkedFiles () =
+            checkCounts |> Seq.map (fun kv -> Path.GetFileName kv.Key, kv.Value) |> Seq.sortBy fst |> List.ofSeq
+
+        checker.ParseAndCheckProject options |> Async.RunSynchronouslyImmediate |> ignore
+        checkedFiles () |> shouldEqual [ for i in 1 .. 5 -> $"File{i}.fs", 1 ]
+
+        // Invalidate the whole chain by touching the first file, then hit the builder with many concurrent requests.
+        // Each of them used to re-stamp the files from the same stale snapshot and build its own chain of bound models.
+        checkCounts.Clear()
+        File.SetLastWriteTimeUtc(fileNames[0], DateTime.UtcNow.AddSeconds 2.0)
+
+        Seq.init 50 (fun _ -> checker.ParseAndCheckProject options |> Async.Ignore)
+        |> Async.Parallel
+        |> Async.RunSynchronouslyImmediate
+        |> ignore
+
+        checkedFiles () |> shouldEqual [ for i in 1 .. 5 -> $"File{i}.fs", 1 ]
+    finally
+        try Directory.Delete(dir, true) with _ -> ()
+
+[<Fact>]
+let ``Parse results the caller retains are served from the cache after a collection`` () =
+    let checker = FSharpChecker.Create(useTransparentCompiler = false)
+
+    // One file more than the cache holds strongly, so the oldest entry is only reachable through its result.
+    let fileNames =
+        [| for i in 1 .. EnvMisc.parseFileCacheSize + 1 -> Path.GetFullPath $"CacheProbe{i}.fs" |]
+
+    let options = { FSharpParsingOptions.Default with SourceFiles = fileNames }
+    let sourceText = SourceText.ofString "module CacheProbe\nlet value = 1\n"
+
+    let parse fileName =
+        checker.ParseFile(fileName, sourceText, options, cache = true) |> Async.RunSynchronouslyImmediate
+
+    let retained = fileNames |> Array.map parse
+
+    GC.Collect()
+    GC.WaitForPendingFinalizers()
+    GC.Collect()
+
+    Object.ReferenceEquals(retained[0], parse fileNames[0]) |> shouldBeTrue
+    GC.KeepAlive retained