Rare deadlock in GraphNode.GetOrComputeValue

[majocha]

GraphNode.GetOrComputeValue can deadlock when the threadpool is under some pressure.

See: https://dev.azure.com/dnceng-public/public/_build/results?buildId=886415&view=logs&j=966a6946-91f0-5397-81f4-d998467880f9&t=344839a9-08c6-5829-65fb-26f32c5a016f&l=17626

Repro steps

This is reproducible in #17872 locally. For example:
in VS select all BuildGraphTests in Test Explorer and "run until failure".
The tests run in parallel and after few dozens or some more iterations the test case Many requests to get a value asynchronously might evaluate the computation more than once even when some requests get canceled will get stuck here

fsharp/tests/FSharp.Compiler.Service.Tests/BuildGraphTests.fs

Line 221 in a90f6d1

Async.RunImmediate(work)

[T-Gro]

Thank you Jakub for finding this.
Is it right to assume you only spotted this with parallel tests?

[majocha]

Thank you Jakub for finding this. Is it right to assume you only spotted this with parallel tests?

Yes, it showed up once in CI recently. I confirmed a repro locally but also only in a parallel test run.

[majocha]

The issue is subtle. We attach a continuation that sets the taken flag:

fsharp/src/Compiler/Facilities/BuildGraph.fs

Lines 57 to 65 in 2c3bdcb

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

ExecuteSynchronously is there to ensure it is always executed before we get to this finally block:

fsharp/src/Compiler/Facilities/BuildGraph.fs

Lines 88 to 90 in 2c3bdcb

	finally
	if taken then
	semaphore.Release() |> ignore

But SemaphoreSlim enforces asynchronous continuations, overriding ExecuteSynchronously.

In effect, on rare occasions when the cancellation coincides with taking the samaphore, the finally block is executed before setting the flag. As a result, the semaphore is never released.