Skip to content

Fix GraphNode deadlock #18178

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 2, 2025
Merged

Fix GraphNode deadlock #18178

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 21 additions & 46 deletions src/Compiler/Facilities/BuildGraph.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
module FSharp.Compiler.BuildGraph

open System.Threading
open System.Threading.Tasks
open System.Globalization

[<RequireQualifiedAccess>]
Expand Down Expand Up @@ -40,55 +39,31 @@ type GraphNode<'T> private (computation: Async<'T>, cachedResult: ValueOption<'T
cachedResultNode
else
async {
let! ct = Async.CancellationToken
Interlocked.Increment(&requestCount) |> ignore
let enter = semaphore.WaitAsync(ct)

try
let! ct = Async.CancellationToken

// We must set 'taken' before any implicit cancellation checks
// occur, making sure we are under the protection of the 'try'.
// For example, NodeCode's 'try/finally' (TryFinally) uses async.TryFinally which does
// implicit cancellation checks even before the try is entered, as do the
// de-sugaring of 'do!' and other NodeCode constructs.
let mutable taken = false

try
do!
semaphore
.WaitAsync(ct)
.ContinueWith(
(fun _ -> taken <- true),
(TaskContinuationOptions.NotOnCanceled
||| TaskContinuationOptions.NotOnFaulted
||| TaskContinuationOptions.ExecuteSynchronously)
)
|> Async.AwaitTask

match cachedResult with
| ValueSome value -> return value
| _ ->
let tcs = TaskCompletionSource<'T>()

Async.StartWithContinuations(
async {
Thread.CurrentThread.CurrentUICulture <- GraphNode.culture
return! computation
},
(fun res ->
cachedResult <- ValueSome res
cachedResultNode <- async.Return res
computation <- Unchecked.defaultof<_>
tcs.SetResult(res)),
(fun ex -> tcs.SetException(ex)),
(fun _ -> tcs.SetCanceled()),
ct
)

return! tcs.Task |> Async.AwaitTask
finally
if taken then
semaphore.Release() |> ignore
do! enter |> Async.AwaitTask

match cachedResult with
| ValueSome value -> return value
| _ ->
Thread.CurrentThread.CurrentUICulture <- GraphNode.culture
let! result = computation
cachedResult <- ValueSome result
cachedResultNode <- async.Return result
computation <- Unchecked.defaultof<_>
return result
finally
// At this point, the semaphore awaiter is either already completed or about to get canceled.
// If calling Wait() does not throw an exception it means the semaphore was successfully taken and needs to be released.
try
enter.Wait()
semaphore.Release() |> ignore
with _ ->
()

Interlocked.Decrement(&requestCount) |> ignore
}

Expand Down
Loading