From c3a6b0fb47c10ae22846a24112c57c964fb5d6b6 Mon Sep 17 00:00:00 2001 From: Petr Pokorny Date: Tue, 16 Jan 2024 14:59:37 +0100 Subject: [PATCH 01/10] Improve AsyncMemoize tests --- .../CompilerService/AsyncMemoize.fs | 86 +++++++++---------- 1 file changed, 42 insertions(+), 44 deletions(-) diff --git a/tests/FSharp.Compiler.ComponentTests/CompilerService/AsyncMemoize.fs b/tests/FSharp.Compiler.ComponentTests/CompilerService/AsyncMemoize.fs index 817a3b8c70a..920a2a58cb7 100644 --- a/tests/FSharp.Compiler.ComponentTests/CompilerService/AsyncMemoize.fs +++ b/tests/FSharp.Compiler.ComponentTests/CompilerService/AsyncMemoize.fs @@ -11,28 +11,21 @@ open FSharp.Compiler.DiagnosticsLogger open FSharp.Compiler.Diagnostics open FSharp.Compiler.BuildGraph -[] -let ``Stack trace`` () = - let memoize = AsyncMemoize() +let timeout = TimeSpan.FromSeconds 10 - let computation key = node { - // do! Async.Sleep 1 |> NodeCode.AwaitAsync +let waitFor (mre: ManualResetEvent) = + if not <| mre.WaitOne timeout then + failwith "waitFor timed out" - let! result = memoize.Get'(key * 2, node { - //do! Async.Sleep 1 |> NodeCode.AwaitAsync - return key * 5 - }) - return result * 2 +let rec internal spinFor (duration: TimeSpan) = + node { + let sw = Stopwatch.StartNew() + do! Async.Sleep 10 |> NodeCode.AwaitAsync + return! spinFor (duration - sw.Elapsed) } - //let _r2 = computation 10 - - let result = memoize.Get'(1, computation 1) |> NodeCode.RunImmediateWithoutCancellation - - Assert.Equal(10, result) - [] let ``Basics``() = @@ -74,16 +67,21 @@ let ``We can cancel a job`` () = let jobStarted = new ManualResetEvent(false) - let computation key = node { - jobStarted.Set() |> ignore - do! Async.Sleep 1000 |> NodeCode.AwaitAsync + let jobCanceled = new ManualResetEvent(false) + + let computation action = node { + action() |> ignore + do! spinFor timeout failwith "Should be canceled before it gets here" - return key * 2 } let eventLog = ResizeArray() - let memoize = AsyncMemoize() - memoize.OnEvent(fun (e, (_label, k, _version)) -> eventLog.Add (e, k)) + let memoize = AsyncMemoize() + memoize.OnEvent(fun (e, (_label, k, _version)) -> + eventLog.Add (e, k) + if e = Canceled then + jobCanceled.Set() |> ignore + ) use cts1 = new CancellationTokenSource() use cts2 = new CancellationTokenSource() @@ -91,26 +89,22 @@ let ``We can cancel a job`` () = let key = 1 - let _task1 = NodeCode.StartAsTask_ForTesting( memoize.Get'(key, computation key), ct = cts1.Token) + let _task1 = NodeCode.StartAsTask_ForTesting( memoize.Get'(key, computation jobStarted.Set), ct = cts1.Token) - jobStarted.WaitOne() |> ignore + waitFor jobStarted jobStarted.Reset() |> ignore - let _task2 = NodeCode.StartAsTask_ForTesting( memoize.Get'(key, computation key), ct = cts2.Token) - let _task3 = NodeCode.StartAsTask_ForTesting( memoize.Get'(key, computation key), ct = cts3.Token) - - Assert.Equal<(JobEvent * int) array>([| Started, key |], eventLog |> Seq.toArray ) - - do! Task.Delay 100 + let _task2 = NodeCode.StartAsTask_ForTesting( memoize.Get'(key, computation ignore), ct = cts2.Token) + let _task3 = NodeCode.StartAsTask_ForTesting( memoize.Get'(key, computation ignore), ct = cts3.Token) cts1.Cancel() cts2.Cancel() - do! Task.Delay 100 + waitFor jobStarted cts3.Cancel() - do! Task.Delay 100 + waitFor jobCanceled Assert.Equal<(JobEvent * int) array>([| Started, key; Started, key; Canceled, key |], eventLog |> Seq.toArray ) } @@ -120,12 +114,11 @@ let ``Job is restarted if first requestor cancels`` () = task { let jobStarted = new ManualResetEvent(false) + let jobCanComplete = new ManualResetEvent(false) + let computation key = node { jobStarted.Set() |> ignore - - for _ in 1 .. 5 do - do! Async.Sleep 100 |> NodeCode.AwaitAsync - + waitFor jobCanComplete return key * 2 } @@ -140,18 +133,21 @@ let ``Job is restarted if first requestor cancels`` () = let key = 1 let _task1 = NodeCode.StartAsTask_ForTesting( memoize.Get'(key, computation key), ct = cts1.Token) - jobStarted.WaitOne() |> ignore + + waitFor jobStarted + jobStarted.Reset() |> ignore let _task2 = NodeCode.StartAsTask_ForTesting( memoize.Get'(key, computation key), ct = cts2.Token) let _task3 = NodeCode.StartAsTask_ForTesting( memoize.Get'(key, computation key), ct = cts3.Token) - do! Task.Delay 100 - cts1.Cancel() - do! Task.Delay 100 + waitFor jobStarted + cts3.Cancel() + jobCanComplete.Set() |> ignore + let! result = _task2 Assert.Equal(2, result) @@ -216,12 +212,11 @@ let ``Job is restarted if first requestor cancels but keeps running if second re task { let jobStarted = new ManualResetEvent(false) + let jobCanComplete = new ManualResetEvent(false) + let computation key = node { jobStarted.Set() |> ignore - - for _ in 1 .. 5 do - do! Async.Sleep 100 |> NodeCode.AwaitAsync - + waitFor jobCanComplete return key * 2 } @@ -239,6 +234,7 @@ let ``Job is restarted if first requestor cancels but keeps running if second re jobStarted.WaitOne() |> ignore jobStarted.Reset() |> ignore + let _task2 = NodeCode.StartAsTask_ForTesting( memoize.Get'(key, computation key), ct = cts2.Token) let _task3 = NodeCode.StartAsTask_ForTesting( memoize.Get'(key, computation key), ct = cts3.Token) @@ -248,6 +244,8 @@ let ``Job is restarted if first requestor cancels but keeps running if second re cts2.Cancel() + jobCanComplete.Set() |> ignore + let! result = _task3 Assert.Equal(2, result) From de170ae939f7fe6ccdd82708926237dd93562a13 Mon Sep 17 00:00:00 2001 From: Petr Pokorny Date: Tue, 16 Jan 2024 15:59:20 +0100 Subject: [PATCH 02/10] relax test condition --- .../CompilerService/AsyncMemoize.fs | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/FSharp.Compiler.ComponentTests/CompilerService/AsyncMemoize.fs b/tests/FSharp.Compiler.ComponentTests/CompilerService/AsyncMemoize.fs index 920a2a58cb7..404d4f3213e 100644 --- a/tests/FSharp.Compiler.ComponentTests/CompilerService/AsyncMemoize.fs +++ b/tests/FSharp.Compiler.ComponentTests/CompilerService/AsyncMemoize.fs @@ -249,8 +249,6 @@ let ``Job is restarted if first requestor cancels but keeps running if second re let! result = _task3 Assert.Equal(2, result) - Assert.Equal(TaskStatus.Canceled, _task1.Status) - let orderedLog = eventLog |> Seq.sortBy fst |> Seq.map snd |> Seq.toList let expected = [ Started, key; Started, key; Finished, key ] From af8fc6a7452cf59ca13e475ec66dac9ffb0777bc Mon Sep 17 00:00:00 2001 From: Petr Pokorny Date: Tue, 16 Jan 2024 16:19:51 +0100 Subject: [PATCH 03/10] Revert "Cancellable: set token from node/async in features code (#16348)" This reverts commit d4e3b26c7738444fa21092e76e77ff3e21c63006. --- src/Compiler/Facilities/BuildGraph.fs | 8 ----- src/Compiler/Interactive/fsi.fs | 3 -- src/Compiler/Service/BackgroundCompiler.fs | 28 --------------- src/Compiler/Service/FSharpCheckerResults.fs | 3 -- src/Compiler/Service/service.fs | 6 ---- src/Compiler/Utilities/Cancellable.fs | 38 ++++++++++++++------ src/Compiler/Utilities/Cancellable.fsi | 1 - 7 files changed, 27 insertions(+), 60 deletions(-) diff --git a/src/Compiler/Facilities/BuildGraph.fs b/src/Compiler/Facilities/BuildGraph.fs index 8927862c23c..1df58c1024b 100644 --- a/src/Compiler/Facilities/BuildGraph.fs +++ b/src/Compiler/Facilities/BuildGraph.fs @@ -17,14 +17,12 @@ let wrapThreadStaticInfo computation = async { let diagnosticsLogger = DiagnosticsThreadStatics.DiagnosticsLogger let phase = DiagnosticsThreadStatics.BuildPhase - let ct = Cancellable.Token try return! computation finally DiagnosticsThreadStatics.DiagnosticsLogger <- diagnosticsLogger DiagnosticsThreadStatics.BuildPhase <- phase - Cancellable.Token <- ct } type Async<'T> with @@ -127,7 +125,6 @@ type NodeCode private () = static member RunImmediate(computation: NodeCode<'T>, ct: CancellationToken) = let diagnosticsLogger = DiagnosticsThreadStatics.DiagnosticsLogger let phase = DiagnosticsThreadStatics.BuildPhase - let ct2 = Cancellable.Token try try @@ -135,7 +132,6 @@ type NodeCode private () = async { DiagnosticsThreadStatics.DiagnosticsLogger <- diagnosticsLogger DiagnosticsThreadStatics.BuildPhase <- phase - Cancellable.Token <- ct2 return! computation |> Async.AwaitNodeCode } @@ -143,7 +139,6 @@ type NodeCode private () = finally DiagnosticsThreadStatics.DiagnosticsLogger <- diagnosticsLogger DiagnosticsThreadStatics.BuildPhase <- phase - Cancellable.Token <- ct2 with :? AggregateException as ex when ex.InnerExceptions.Count = 1 -> raise (ex.InnerExceptions[0]) @@ -153,14 +148,12 @@ type NodeCode private () = static member StartAsTask_ForTesting(computation: NodeCode<'T>, ?ct: CancellationToken) = let diagnosticsLogger = DiagnosticsThreadStatics.DiagnosticsLogger let phase = DiagnosticsThreadStatics.BuildPhase - let ct2 = Cancellable.Token try let work = async { DiagnosticsThreadStatics.DiagnosticsLogger <- diagnosticsLogger DiagnosticsThreadStatics.BuildPhase <- phase - Cancellable.Token <- ct2 return! computation |> Async.AwaitNodeCode } @@ -168,7 +161,6 @@ type NodeCode private () = finally DiagnosticsThreadStatics.DiagnosticsLogger <- diagnosticsLogger DiagnosticsThreadStatics.BuildPhase <- phase - Cancellable.Token <- ct2 static member CancellationToken = cancellationToken diff --git a/src/Compiler/Interactive/fsi.fs b/src/Compiler/Interactive/fsi.fs index ca0e2335064..e5ff5b6c754 100644 --- a/src/Compiler/Interactive/fsi.fs +++ b/src/Compiler/Interactive/fsi.fs @@ -4089,7 +4089,6 @@ type FsiInteractionProcessor ?cancellationToken: CancellationToken ) = let cancellationToken = defaultArg cancellationToken CancellationToken.None - use _ = Cancellable.UsingToken(cancellationToken) if tokenizer.LexBuffer.IsPastEndOfStream then let stepStatus = @@ -4218,7 +4217,6 @@ type FsiInteractionProcessor member _.EvalInteraction(ctok, sourceText, scriptFileName, diagnosticsLogger, ?cancellationToken) = let cancellationToken = defaultArg cancellationToken CancellationToken.None - use _ = Cancellable.UsingToken(cancellationToken) use _ = UseBuildPhase BuildPhase.Interactive use _ = UseDiagnosticsLogger diagnosticsLogger use _scope = SetCurrentUICultureForThread fsiOptions.FsiLCID @@ -4895,7 +4893,6 @@ type FsiEvaluationSession SpawnInteractiveServer(fsi, fsiOptions, fsiConsoleOutput) use _ = UseBuildPhase BuildPhase.Interactive - use _ = Cancellable.UsingToken(CancellationToken.None) if fsiOptions.Interact then // page in the type check env diff --git a/src/Compiler/Service/BackgroundCompiler.fs b/src/Compiler/Service/BackgroundCompiler.fs index 514bb8e45c5..f9f952dde70 100644 --- a/src/Compiler/Service/BackgroundCompiler.fs +++ b/src/Compiler/Service/BackgroundCompiler.fs @@ -574,9 +574,6 @@ type internal BackgroundCompiler Activity.Tags.cache, cache.ToString() |] - let! ct = Async.CancellationToken - use _ = Cancellable.UsingToken(ct) - if cache then let hash = sourceText.GetHashCode() |> int64 @@ -629,9 +626,6 @@ type internal BackgroundCompiler "BackgroundCompiler.GetBackgroundParseResultsForFileInProject" [| Activity.Tags.fileName, fileName; Activity.Tags.userOpName, userOpName |] - let! ct = NodeCode.CancellationToken - use _ = Cancellable.UsingToken(ct) - let! builderOpt, creationDiags = getOrCreateBuilder (options, userOpName) match builderOpt with @@ -783,9 +777,6 @@ type internal BackgroundCompiler Activity.Tags.userOpName, userOpName |] - let! ct = NodeCode.CancellationToken - use _ = Cancellable.UsingToken(ct) - let! cachedResults = node { let! builderOpt, creationDiags = getAnyBuilder (options, userOpName) @@ -846,9 +837,6 @@ type internal BackgroundCompiler Activity.Tags.userOpName, userOpName |] - let! ct = NodeCode.CancellationToken - use _ = Cancellable.UsingToken(ct) - let! builderOpt, creationDiags = getOrCreateBuilder (options, userOpName) match builderOpt with @@ -897,9 +885,6 @@ type internal BackgroundCompiler Activity.Tags.userOpName, userOpName |] - let! ct = NodeCode.CancellationToken - use _ = Cancellable.UsingToken(ct) - let! builderOpt, creationDiags = getOrCreateBuilder (options, userOpName) match builderOpt with @@ -969,9 +954,6 @@ type internal BackgroundCompiler Activity.Tags.userOpName, userOpName |] - let! ct = NodeCode.CancellationToken - use _ = Cancellable.UsingToken(ct) - let! builderOpt, _ = getOrCreateBuilder (options, userOpName) match builderOpt with @@ -991,9 +973,6 @@ type internal BackgroundCompiler Activity.Tags.userOpName, userOpName |] - let! ct = NodeCode.CancellationToken - use _ = Cancellable.UsingToken(ct) - let! builderOpt, creationDiags = getOrCreateBuilder (options, userOpName) match builderOpt with @@ -1134,9 +1113,6 @@ type internal BackgroundCompiler Activity.Tags.userOpName, userOpName |] - let! ct = NodeCode.CancellationToken - use _ = Cancellable.UsingToken(ct) - let! builderOpt, _ = getOrCreateBuilder (options, userOpName) match builderOpt with @@ -1185,8 +1161,6 @@ type internal BackgroundCompiler /// Parse and typecheck the whole project (the implementation, called recursively as project graph is evaluated) member private _.ParseAndCheckProjectImpl(options, userOpName) = node { - let! ct = NodeCode.CancellationToken - use _ = Cancellable.UsingToken(ct) let! builderOpt, creationDiags = getOrCreateBuilder (options, userOpName) @@ -1452,8 +1426,6 @@ type internal BackgroundCompiler |] async { - let! ct = Async.CancellationToken - use _ = Cancellable.UsingToken(ct) let! ct = Async.CancellationToken // If there was a similar entry (as there normally will have been) then re-establish an empty builder . This diff --git a/src/Compiler/Service/FSharpCheckerResults.fs b/src/Compiler/Service/FSharpCheckerResults.fs index 48b9875c9f0..5f18a90968a 100644 --- a/src/Compiler/Service/FSharpCheckerResults.fs +++ b/src/Compiler/Service/FSharpCheckerResults.fs @@ -3585,9 +3585,6 @@ type FsiInteractiveChecker(legacyReferenceResolver, tcConfig: TcConfig, tcGlobal member _.ParseAndCheckInteraction(sourceText: ISourceText, ?userOpName: string) = cancellable { - let! ct = Cancellable.token () - use _ = Cancellable.UsingToken(ct) - let userOpName = defaultArg userOpName "Unknown" let fileName = Path.Combine(tcConfig.implicitIncludeDir, "stdin.fsx") let suggestNamesForErrors = true // Will always be true, this is just for readability diff --git a/src/Compiler/Service/service.fs b/src/Compiler/Service/service.fs index b79254d7935..492ff2da497 100644 --- a/src/Compiler/Service/service.fs +++ b/src/Compiler/Service/service.fs @@ -348,9 +348,6 @@ type FSharpChecker use _ = Activity.start "FSharpChecker.Compile" [| Activity.Tags.userOpName, _userOpName |] async { - let! ct = Async.CancellationToken - use _ = Cancellable.UsingToken(ct) - let ctok = CompilationThreadToken() return CompileHelpers.compileFromArgs (ctok, argv, legacyReferenceResolver, None, None) } @@ -485,9 +482,6 @@ type FSharpChecker let userOpName = defaultArg userOpName "Unknown" node { - let! ct = NodeCode.CancellationToken - use _ = Cancellable.UsingToken(ct) - if fastCheck <> Some true || not captureIdentifiersWhenParsing then return! backgroundCompiler.FindReferencesInFile(fileName, options, symbol, canInvalidateProject, userOpName) else diff --git a/src/Compiler/Utilities/Cancellable.fs b/src/Compiler/Utilities/Cancellable.fs index c702e3b7a0b..59e7def4c10 100644 --- a/src/Compiler/Utilities/Cancellable.fs +++ b/src/Compiler/Utilities/Cancellable.fs @@ -2,32 +2,47 @@ namespace FSharp.Compiler open System open System.Threading +open Internal.Utilities.Library [] type Cancellable = [] - static val mutable private token: CancellationToken - - static member UsingToken(ct) = - let oldCt = Cancellable.token - - Cancellable.token <- ct + static val mutable private tokens: CancellationToken list + static let disposable = { new IDisposable with - member this.Dispose() = Cancellable.token <- oldCt + member this.Dispose() = + Cancellable.Tokens <- Cancellable.Tokens |> List.tail } - static member Token - with get () = Cancellable.token - and internal set v = Cancellable.token <- v + static member Tokens + with private get () = + match box Cancellable.tokens with + | Null -> [] + | _ -> Cancellable.tokens + and private set v = Cancellable.tokens <- v + + static member UsingToken(ct) = + Cancellable.Tokens <- ct :: Cancellable.Tokens + disposable + + static member Token = + match Cancellable.Tokens with + | [] -> CancellationToken.None + | token :: _ -> token + /// There may be multiple tokens if `UsingToken` is called multiple times, producing scoped structure. + /// We're interested in the current, i.e. the most recent, one. static member CheckAndThrow() = - Cancellable.token.ThrowIfCancellationRequested() + match Cancellable.Tokens with + | [] -> () + | token :: _ -> token.ThrowIfCancellationRequested() namespace Internal.Utilities.Library open System open System.Threading +open FSharp.Compiler #if !FSHARPCORE_USE_PACKAGE open FSharp.Core.CompilerServices.StateMachineHelpers @@ -48,6 +63,7 @@ module Cancellable = ValueOrCancelled.Cancelled(OperationCanceledException ct) else try + use _ = Cancellable.UsingToken(ct) oper ct with :? OperationCanceledException as e -> ValueOrCancelled.Cancelled(OperationCanceledException e.CancellationToken) diff --git a/src/Compiler/Utilities/Cancellable.fsi b/src/Compiler/Utilities/Cancellable.fsi index 6e36d7ecb6d..23515432bdd 100644 --- a/src/Compiler/Utilities/Cancellable.fsi +++ b/src/Compiler/Utilities/Cancellable.fsi @@ -7,7 +7,6 @@ open System.Threading type Cancellable = static member internal UsingToken: CancellationToken -> IDisposable static member Token: CancellationToken - static member internal Token: CancellationToken with set static member CheckAndThrow: unit -> unit namespace Internal.Utilities.Library From 0a9f7287f598cba826d30ef2c6275e3ca68bcb1c Mon Sep 17 00:00:00 2001 From: 0101 <0101@innit.cz> Date: Tue, 16 Jan 2024 18:02:21 +0100 Subject: [PATCH 04/10] remove UsingToken --- src/Compiler/Facilities/AsyncMemoize.fs | 3 +-- src/Compiler/Service/TransparentCompiler.fs | 2 -- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/src/Compiler/Facilities/AsyncMemoize.fs b/src/Compiler/Facilities/AsyncMemoize.fs index ad798140cb9..8309eaa1c15 100644 --- a/src/Compiler/Facilities/AsyncMemoize.fs +++ b/src/Compiler/Facilities/AsyncMemoize.fs @@ -346,7 +346,6 @@ type internal AsyncMemoize<'TKey, 'TVersion, 'TValue when 'TKey: equality and 'T try // TODO: Should unify starting and restarting - use _ = Cancellable.UsingToken(cts.Token) log (Started, key) Interlocked.Increment &restarted |> ignore System.Diagnostics.Trace.TraceInformation $"{name} Restarted {key.Label}" @@ -498,7 +497,7 @@ type internal AsyncMemoize<'TKey, 'TVersion, 'TValue when 'TKey: equality and 'T // TODO: Should unify starting and restarting let currentLogger = DiagnosticsThreadStatics.DiagnosticsLogger DiagnosticsThreadStatics.DiagnosticsLogger <- cachingLogger - use _ = Cancellable.UsingToken(internalCt) + log (Started, key) try diff --git a/src/Compiler/Service/TransparentCompiler.fs b/src/Compiler/Service/TransparentCompiler.fs index fe82627483f..ad0091d214b 100644 --- a/src/Compiler/Service/TransparentCompiler.fs +++ b/src/Compiler/Service/TransparentCompiler.fs @@ -1762,8 +1762,6 @@ type internal TransparentCompiler node { //use _ = // Activity.start "ParseFile" [| Activity.Tags.fileName, fileName |> Path.GetFileName |] - let! ct = NodeCode.CancellationToken - use _ = Cancellable.UsingToken(ct) // TODO: might need to deal with exceptions here: let tcConfigB, sourceFileNames, _ = ComputeTcConfigBuilder projectSnapshot From a03215e500ce7c5e11291292ec97405c99b6c0e6 Mon Sep 17 00:00:00 2001 From: 0101 <0101@innit.cz> Date: Tue, 16 Jan 2024 18:36:43 +0100 Subject: [PATCH 05/10] remove UsingToken --- src/Compiler/Service/TransparentCompiler.fs | 33 --------------------- 1 file changed, 33 deletions(-) diff --git a/src/Compiler/Service/TransparentCompiler.fs b/src/Compiler/Service/TransparentCompiler.fs index ad0091d214b..0d2822eadb2 100644 --- a/src/Compiler/Service/TransparentCompiler.fs +++ b/src/Compiler/Service/TransparentCompiler.fs @@ -1816,9 +1816,6 @@ type internal TransparentCompiler userOpName: string ) : NodeCode = node { - let! ct = NodeCode.CancellationToken - use _ = Cancellable.UsingToken(ct) - let! snapshot = FSharpProjectSnapshot.FromOptions(options, fileName, fileVersion, sourceText) |> NodeCode.AwaitAsync @@ -1840,9 +1837,6 @@ type internal TransparentCompiler userOpName: string ) : NodeCode = node { - let! ct = NodeCode.CancellationToken - use _ = Cancellable.UsingToken(ct) - let! snapshot = FSharpProjectSnapshot.FromOptions(options, fileName, fileVersion, sourceText) |> NodeCode.AwaitAsync @@ -1895,9 +1889,6 @@ type internal TransparentCompiler userOpName: string ) : NodeCode> = node { - let! ct = NodeCode.CancellationToken - use _ = Cancellable.UsingToken(ct) - ignore canInvalidateProject let! snapshot = FSharpProjectSnapshot.FromOptions options |> NodeCode.AwaitAsync @@ -1912,9 +1903,6 @@ type internal TransparentCompiler member this.GetAssemblyData(options: FSharpProjectOptions, fileName, userOpName: string) : NodeCode = node { - let! ct = NodeCode.CancellationToken - use _ = Cancellable.UsingToken(ct) - let! snapshot = FSharpProjectSnapshot.FromOptions options |> NodeCode.AwaitAsync return! this.GetAssemblyData(snapshot.ProjectSnapshot, fileName, userOpName) } @@ -1934,9 +1922,6 @@ type internal TransparentCompiler userOpName: string ) : NodeCode = node { - let! ct = NodeCode.CancellationToken - use _ = Cancellable.UsingToken(ct) - let! snapshot = FSharpProjectSnapshot.FromOptions options |> NodeCode.AwaitAsync match! this.ParseAndCheckFileInProject(fileName, snapshot.ProjectSnapshot, userOpName) with @@ -1951,9 +1936,6 @@ type internal TransparentCompiler userOpName: string ) : NodeCode = node { - let! ct = NodeCode.CancellationToken - use _ = Cancellable.UsingToken(ct) - let! snapshot = FSharpProjectSnapshot.FromOptions options |> NodeCode.AwaitAsync return! this.ParseFile(fileName, snapshot.ProjectSnapshot, userOpName) } @@ -1967,9 +1949,6 @@ type internal TransparentCompiler ) : NodeCode<(FSharpParseFileResults * FSharpCheckFileResults) option> = node { ignore builder - let! ct = NodeCode.CancellationToken - use _ = Cancellable.UsingToken(ct) - let! snapshot = FSharpProjectSnapshot.FromOptions(options, fileName, 1, sourceText) |> NodeCode.AwaitAsync @@ -2021,9 +2000,6 @@ type internal TransparentCompiler ) : NodeCode = node { ignore userOpName - let! ct = NodeCode.CancellationToken - use _ = Cancellable.UsingToken(ct) - let! snapshot = FSharpProjectSnapshot.FromOptions options |> NodeCode.AwaitAsync return! ComputeSemanticClassification(fileName, snapshot.ProjectSnapshot) } @@ -2046,9 +2022,6 @@ type internal TransparentCompiler userOpName: string ) : NodeCode = node { - let! ct = NodeCode.CancellationToken - use _ = Cancellable.UsingToken(ct) - let! snapshot = FSharpProjectSnapshot.FromOptions(options, fileName, fileVersion, sourceText) |> NodeCode.AwaitAsync @@ -2061,9 +2034,6 @@ type internal TransparentCompiler member this.ParseAndCheckProject(options: FSharpProjectOptions, userOpName: string) : NodeCode = node { - let! ct = NodeCode.CancellationToken - use _ = Cancellable.UsingToken(ct) - ignore userOpName let! snapshot = FSharpProjectSnapshot.FromOptions options |> NodeCode.AwaitAsync return! ComputeParseAndCheckProject snapshot.ProjectSnapshot @@ -2071,9 +2041,6 @@ type internal TransparentCompiler member this.ParseAndCheckProject(projectSnapshot: FSharpProjectSnapshot, userOpName: string) : NodeCode = node { - let! ct = NodeCode.CancellationToken - use _ = Cancellable.UsingToken(ct) - ignore userOpName return! ComputeParseAndCheckProject projectSnapshot.ProjectSnapshot } From 2ad0e812bcf5975ec284152678cb76b63cf86f1a Mon Sep 17 00:00:00 2001 From: 0101 <0101@innit.cz> Date: Tue, 16 Jan 2024 20:06:19 +0100 Subject: [PATCH 06/10] test improvement --- src/Compiler/Service/TransparentCompiler.fs | 1 + .../CompilerService/AsyncMemoize.fs | 16 ++++++++-------- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/Compiler/Service/TransparentCompiler.fs b/src/Compiler/Service/TransparentCompiler.fs index 0d2822eadb2..298c0e6b627 100644 --- a/src/Compiler/Service/TransparentCompiler.fs +++ b/src/Compiler/Service/TransparentCompiler.fs @@ -1949,6 +1949,7 @@ type internal TransparentCompiler ) : NodeCode<(FSharpParseFileResults * FSharpCheckFileResults) option> = node { ignore builder + let! snapshot = FSharpProjectSnapshot.FromOptions(options, fileName, 1, sourceText) |> NodeCode.AwaitAsync diff --git a/tests/FSharp.Compiler.ComponentTests/CompilerService/AsyncMemoize.fs b/tests/FSharp.Compiler.ComponentTests/CompilerService/AsyncMemoize.fs index 404d4f3213e..7344a45c835 100644 --- a/tests/FSharp.Compiler.ComponentTests/CompilerService/AsyncMemoize.fs +++ b/tests/FSharp.Compiler.ComponentTests/CompilerService/AsyncMemoize.fs @@ -122,9 +122,9 @@ let ``Job is restarted if first requestor cancels`` () = return key * 2 } - let eventLog = ConcurrentBag() - let memoize = AsyncMemoize() - memoize.OnEvent(fun (e, (_, k, _version)) -> eventLog.Add (DateTime.Now.Ticks, (e, k))) + let eventLog = ConcurrentStack() + let memoize = AsyncMemoize() + memoize.OnEvent(fun (e, (_, k, _version)) -> eventLog.Push (e, k)) use cts1 = new CancellationTokenSource() use cts2 = new CancellationTokenSource() @@ -153,7 +153,7 @@ let ``Job is restarted if first requestor cancels`` () = Assert.Equal(TaskStatus.Canceled, _task1.Status) - let orderedLog = eventLog |> Seq.sortBy fst |> Seq.map snd |> Seq.toList + let orderedLog = eventLog |> Seq.rev |> Seq.toList let expected = [ Started, key; Started, key; Finished, key ] Assert.Equal<_ list>(expected, orderedLog) @@ -220,9 +220,9 @@ let ``Job is restarted if first requestor cancels but keeps running if second re return key * 2 } - let eventLog = ConcurrentBag() - let memoize = AsyncMemoize() - memoize.OnEvent(fun (e, (_label, k, _version)) -> eventLog.Add (DateTime.Now.Ticks, (e, k))) + let eventLog = ConcurrentStack() + let memoize = AsyncMemoize() + memoize.OnEvent(fun (e, (_label, k, _version)) -> eventLog.Push (e, k)) use cts1 = new CancellationTokenSource() use cts2 = new CancellationTokenSource() @@ -249,7 +249,7 @@ let ``Job is restarted if first requestor cancels but keeps running if second re let! result = _task3 Assert.Equal(2, result) - let orderedLog = eventLog |> Seq.sortBy fst |> Seq.map snd |> Seq.toList + let orderedLog = eventLog |> Seq.rev |> Seq.toList let expected = [ Started, key; Started, key; Finished, key ] Assert.Equal<_ list>(expected, orderedLog) From 4a121032afae245913ad444163972fdd4fc0e441 Mon Sep 17 00:00:00 2001 From: 0101 <0101@innit.cz> Date: Wed, 17 Jan 2024 10:50:57 +0100 Subject: [PATCH 07/10] relax test condition --- .../CompilerService/AsyncMemoize.fs | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/FSharp.Compiler.ComponentTests/CompilerService/AsyncMemoize.fs b/tests/FSharp.Compiler.ComponentTests/CompilerService/AsyncMemoize.fs index 7344a45c835..92dda5bd044 100644 --- a/tests/FSharp.Compiler.ComponentTests/CompilerService/AsyncMemoize.fs +++ b/tests/FSharp.Compiler.ComponentTests/CompilerService/AsyncMemoize.fs @@ -151,8 +151,6 @@ let ``Job is restarted if first requestor cancels`` () = let! result = _task2 Assert.Equal(2, result) - Assert.Equal(TaskStatus.Canceled, _task1.Status) - let orderedLog = eventLog |> Seq.rev |> Seq.toList let expected = [ Started, key; Started, key; Finished, key ] From 22cc08061e54f9ce0a6d6a18a4b76878f7958fc4 Mon Sep 17 00:00:00 2001 From: 0101 <0101@innit.cz> Date: Wed, 17 Jan 2024 13:37:44 +0100 Subject: [PATCH 08/10] use thread-safe collections when collecting events from AsyncMemoize --- .../CompilerService/AsyncMemoize.fs | 52 +------------------ .../FSharpChecker/TransparentCompiler.fs | 36 ++++++------- 2 files changed, 20 insertions(+), 68 deletions(-) diff --git a/tests/FSharp.Compiler.ComponentTests/CompilerService/AsyncMemoize.fs b/tests/FSharp.Compiler.ComponentTests/CompilerService/AsyncMemoize.fs index 92dda5bd044..fcd10321ad8 100644 --- a/tests/FSharp.Compiler.ComponentTests/CompilerService/AsyncMemoize.fs +++ b/tests/FSharp.Compiler.ComponentTests/CompilerService/AsyncMemoize.fs @@ -75,10 +75,10 @@ let ``We can cancel a job`` () = failwith "Should be canceled before it gets here" } - let eventLog = ResizeArray() + let eventLog = ConcurrentQueue() let memoize = AsyncMemoize() memoize.OnEvent(fun (e, (_label, k, _version)) -> - eventLog.Add (e, k) + eventLog.Enqueue (e, k) if e = Canceled then jobCanceled.Set() |> ignore ) @@ -157,54 +157,6 @@ let ``Job is restarted if first requestor cancels`` () = Assert.Equal<_ list>(expected, orderedLog) } -// [] - if we decide to enable that -let ``Job keeps running if the first requestor cancels`` () = - task { - let jobStarted = new ManualResetEvent(false) - - let computation key = node { - jobStarted.Set() |> ignore - - for _ in 1 .. 5 do - do! Async.Sleep 100 |> NodeCode.AwaitAsync - - return key * 2 - } - - let eventLog = ConcurrentBag() - let memoize = AsyncMemoize() - memoize.OnEvent(fun (e, (_label, k, _version)) -> eventLog.Add (DateTime.Now.Ticks, (e, k))) - - use cts1 = new CancellationTokenSource() - use cts2 = new CancellationTokenSource() - use cts3 = new CancellationTokenSource() - - let key = 1 - - let _task1 = NodeCode.StartAsTask_ForTesting( memoize.Get'(key, computation key), ct = cts1.Token) - jobStarted.WaitOne() |> ignore - - let _task2 = NodeCode.StartAsTask_ForTesting( memoize.Get'(key, computation key), ct = cts2.Token) - let _task3 = NodeCode.StartAsTask_ForTesting( memoize.Get'(key, computation key), ct = cts3.Token) - - jobStarted.WaitOne() |> ignore - - cts1.Cancel() - - do! Task.Delay 100 - cts3.Cancel() - - let! result = _task2 - Assert.Equal(2, result) - - Assert.Equal(TaskStatus.Canceled, _task1.Status) - - let orderedLog = eventLog |> Seq.sortBy fst |> Seq.map snd |> Seq.toList - let expected = [ Started, key; Finished, key ] - - Assert.Equal<_ list>(expected, orderedLog) - } - [] let ``Job is restarted if first requestor cancels but keeps running if second requestor cancels`` () = task { diff --git a/tests/FSharp.Compiler.ComponentTests/FSharpChecker/TransparentCompiler.fs b/tests/FSharp.Compiler.ComponentTests/FSharpChecker/TransparentCompiler.fs index a6b7f6fcc41..57a9e266fe8 100644 --- a/tests/FSharp.Compiler.ComponentTests/FSharpChecker/TransparentCompiler.fs +++ b/tests/FSharp.Compiler.ComponentTests/FSharpChecker/TransparentCompiler.fs @@ -219,13 +219,13 @@ let ``Changes in a referenced project`` () = [] let ``File is not checked twice`` () = - let cacheEvents = ResizeArray() + let cacheEvents = ConcurrentQueue() testWorkflow() { withChecker (fun checker -> async { do! Async.Sleep 50 // wait for events from initial project check - checker.Caches.TcIntermediate.OnEvent cacheEvents.Add + checker.Caches.TcIntermediate.OnEvent cacheEvents.Enqueue }) updateFile "First" updatePublicSurface checkFile "Third" expectOk @@ -242,13 +242,13 @@ let ``File is not checked twice`` () = [] let ``If a file is checked as a dependency it's not re-checked later`` () = - let cacheEvents = ResizeArray() + let cacheEvents = ConcurrentQueue() testWorkflow() { withChecker (fun checker -> async { do! Async.Sleep 50 // wait for events from initial project check - checker.Caches.TcIntermediate.OnEvent cacheEvents.Add + checker.Caches.TcIntermediate.OnEvent cacheEvents.Enqueue }) updateFile "First" updatePublicSurface checkFile "Last" expectOk @@ -272,13 +272,13 @@ let ``We don't check files that are not depended on`` () = sourceFile "Third" ["First"], sourceFile "Last" ["Third"]) - let cacheEvents = ResizeArray() + let cacheEvents = ConcurrentQueue() ProjectWorkflowBuilder(project, useTransparentCompiler = true) { withChecker (fun checker -> async { do! Async.Sleep 50 // wait for events from initial project check - checker.Caches.TcIntermediate.OnEvent cacheEvents.Add + checker.Caches.TcIntermediate.OnEvent cacheEvents.Enqueue }) updateFile "First" updatePublicSurface checkFile "Last" expectOk @@ -302,8 +302,8 @@ let ``Files that are not depended on don't invalidate cache`` () = sourceFile "Third" ["First"], sourceFile "Last" ["Third"]) - let cacheTcIntermediateEvents = ResizeArray() - let cacheGraphConstructionEvents = ResizeArray() + let cacheTcIntermediateEvents = ConcurrentQueue() + let cacheGraphConstructionEvents = ConcurrentQueue() ProjectWorkflowBuilder(project, useTransparentCompiler = true) { updateFile "First" updatePublicSurface @@ -311,8 +311,8 @@ let ``Files that are not depended on don't invalidate cache`` () = withChecker (fun checker -> async { do! Async.Sleep 50 // wait for events from initial project check - checker.Caches.TcIntermediate.OnEvent cacheTcIntermediateEvents.Add - checker.Caches.DependencyGraph.OnEvent cacheGraphConstructionEvents.Add + checker.Caches.TcIntermediate.OnEvent cacheTcIntermediateEvents.Enqueue + checker.Caches.DependencyGraph.OnEvent cacheGraphConstructionEvents.Enqueue }) updateFile "Second" updatePublicSurface @@ -344,8 +344,8 @@ let ``Files that are not depended on don't invalidate cache part 2`` () = sourceFile "D" ["B"; "C"], sourceFile "E" ["C"]) - let cacheTcIntermediateEvents = ResizeArray() - let cacheGraphConstructionEvents = ResizeArray() + let cacheTcIntermediateEvents = ConcurrentQueue() + let cacheGraphConstructionEvents = ConcurrentQueue() ProjectWorkflowBuilder(project, useTransparentCompiler = true) { updateFile "A" updatePublicSurface @@ -353,8 +353,8 @@ let ``Files that are not depended on don't invalidate cache part 2`` () = withChecker (fun checker -> async { do! Async.Sleep 50 // wait for events from initial project check - checker.Caches.TcIntermediate.OnEvent cacheTcIntermediateEvents.Add - checker.Caches.DependencyGraph.OnEvent cacheGraphConstructionEvents.Add + checker.Caches.TcIntermediate.OnEvent cacheTcIntermediateEvents.Enqueue + checker.Caches.DependencyGraph.OnEvent cacheGraphConstructionEvents.Enqueue }) updateFile "B" updatePublicSurface checkFile "E" expectOk @@ -382,7 +382,7 @@ let ``Changing impl files doesn't invalidate cache when they have signatures`` ( { sourceFile "B" ["A"] with SignatureFile = AutoGenerated }, { sourceFile "C" ["B"] with SignatureFile = AutoGenerated }) - let cacheEvents = ResizeArray() + let cacheEvents = ConcurrentQueue() ProjectWorkflowBuilder(project, useTransparentCompiler = true) { updateFile "A" updatePublicSurface @@ -390,7 +390,7 @@ let ``Changing impl files doesn't invalidate cache when they have signatures`` ( withChecker (fun checker -> async { do! Async.Sleep 50 // wait for events from initial project check - checker.Caches.TcIntermediate.OnEvent cacheEvents.Add + checker.Caches.TcIntermediate.OnEvent cacheEvents.Enqueue }) updateFile "A" updateInternal checkFile "C" expectOk @@ -412,14 +412,14 @@ let ``Changing impl file doesn't invalidate an in-memory referenced project`` () SyntheticProject.Create("project", sourceFile "B" ["A"] ) with DependsOn = [library] } - let cacheEvents = ResizeArray() + let cacheEvents = ConcurrentQueue() ProjectWorkflowBuilder(project, useTransparentCompiler = true) { checkFile "B" expectOk withChecker (fun checker -> async { do! Async.Sleep 50 // wait for events from initial project check - checker.Caches.TcIntermediate.OnEvent cacheEvents.Add + checker.Caches.TcIntermediate.OnEvent cacheEvents.Enqueue }) updateFile "A" updateInternal checkFile "B" expectOk From 94570d2b83813895fa40f4926e932177e19e90aa Mon Sep 17 00:00:00 2001 From: Petr Pokorny Date: Thu, 18 Jan 2024 14:45:00 +0100 Subject: [PATCH 09/10] fix flaky test --- .../CompilerService/AsyncMemoize.fs | 35 ++++++++++++++----- 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/tests/FSharp.Compiler.ComponentTests/CompilerService/AsyncMemoize.fs b/tests/FSharp.Compiler.ComponentTests/CompilerService/AsyncMemoize.fs index 404d4f3213e..1f70ca3b3cb 100644 --- a/tests/FSharp.Compiler.ComponentTests/CompilerService/AsyncMemoize.fs +++ b/tests/FSharp.Compiler.ComponentTests/CompilerService/AsyncMemoize.fs @@ -23,7 +23,9 @@ let rec internal spinFor (duration: TimeSpan) = node { let sw = Stopwatch.StartNew() do! Async.Sleep 10 |> NodeCode.AwaitAsync - return! spinFor (duration - sw.Elapsed) + let remaining = duration - sw.Elapsed + if remaining > TimeSpan.Zero then + return! spinFor remaining } @@ -389,11 +391,21 @@ let ``Cancel running jobs with the same key`` cancelDuplicate expectFinished = let mutable started = 0 let mutable finished = 0 - let work () = node { + let job1started = new ManualResetEvent(false) + let job1finished = new ManualResetEvent(false) + + let jobCanContinue = new ManualResetEvent(false) + + let job2started = new ManualResetEvent(false) + let job2finished = new ManualResetEvent(false) + + let work onStart onFinish = node { Interlocked.Increment &started |> ignore - for _ in 1..10 do - do! Async.Sleep 10 |> NodeCode.AwaitAsync + onStart() |> ignore + waitFor jobCanContinue + do! spinFor (TimeSpan.FromMilliseconds 100) Interlocked.Increment &finished |> ignore + onFinish() |> ignore } let key1 = @@ -402,9 +414,9 @@ let ``Cancel running jobs with the same key`` cancelDuplicate expectFinished = member _.GetVersion() = 1 member _.GetLabel() = "key1" } - cache.Get(key1, work()) |> Async.AwaitNodeCode |> Async.Start + cache.Get(key1, work job1started.Set job1finished.Set) |> Async.AwaitNodeCode |> Async.Start - do! Task.Delay 50 + waitFor job1started let key2 = { new ICacheKey<_, _> with @@ -412,9 +424,16 @@ let ``Cancel running jobs with the same key`` cancelDuplicate expectFinished = member _.GetVersion() = key1.GetVersion() + 1 member _.GetLabel() = "key2" } - cache.Get(key2, work()) |> Async.AwaitNodeCode |> Async.Start + cache.Get(key2, work job2started.Set job2finished.Set ) |> Async.AwaitNodeCode |> Async.Start - do! Task.Delay 500 + waitFor job2started + + jobCanContinue.Set() |> ignore + + waitFor job2finished + + if not cancelDuplicate then + waitFor job1finished Assert.Equal((2, expectFinished), (started, finished)) } From a6ac7886a932aecee719a8a924eb982296620c49 Mon Sep 17 00:00:00 2001 From: Petr Pokorny Date: Thu, 18 Jan 2024 17:17:03 +0100 Subject: [PATCH 10/10] release note --- docs/release-notes/.FSharp.Compiler.Service/8.0.300.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/release-notes/.FSharp.Compiler.Service/8.0.300.md b/docs/release-notes/.FSharp.Compiler.Service/8.0.300.md index d9acfde71ca..30fd15ca592 100644 --- a/docs/release-notes/.FSharp.Compiler.Service/8.0.300.md +++ b/docs/release-notes/.FSharp.Compiler.Service/8.0.300.md @@ -11,4 +11,5 @@ * `implicitCtorSynPats` in `SynTypeDefnSimpleRepr.General` is now `SynPat option` instead of `SynSimplePats option`. ([PR #16425](https://github.com/dotnet/fsharp/pull/16425)) * `SyntaxVisitorBase<'T>.VisitSimplePats` now takes `SynPat` instead of `SynSimplePat list`. ([PR #16425](https://github.com/dotnet/fsharp/pull/16425)) -* Reduce allocations in compiler checking via `ValueOption` usage ([PR #16323](https://github.com/dotnet/fsharp/pull/16323)) \ No newline at end of file +* Reduce allocations in compiler checking via `ValueOption` usage ([PR #16323](https://github.com/dotnet/fsharp/pull/16323)) +* Reverted [#16348](https://github.com/dotnet/fsharp/pull/16348) `ThreadStatic` `CancellationToken` changes to improve test stability and prevent potential unwanted cancellations. ([PR #16536](https://github.com/dotnet/fsharp/pull/16536)) \ No newline at end of file