AsyncLocal diagnostics, clean slate #16779
Conversation
❗ Release notes required
|
|
/run fantomas |
|
Failed to run fantomas: https://github.com/dotnet/fsharp/actions/runs/8894616544 |
|
I added one more test to make sure it works with ListParallel. (In use with graph-based type checking). |
|
/run fantomas |
|
Failed to run fantomas: https://github.com/dotnet/fsharp/actions/runs/8924428488 |
| CheckDisposed() | ||
|
|
||
| // Prevent deadlocks in FSI. |
There was a problem hiding this comment.
I'd love to put a more meaningful comment here, but I'm not 100% sure what's going on here and I don't want to misrepresent anything.
There was a problem hiding this comment.
I can't reproduce the problem that this was supposed to solve. I'll get rid of this and see what happens in CI.
|
|
||
| let wrapThreadStaticInfo computation = | ||
| async { | ||
| let diagnosticsLogger = DiagnosticsThreadStatics.DiagnosticsLogger |
There was a problem hiding this comment.
NodeCode does this stacking and unwinding implicitly. Without it, we need to be explicit about it by using functions like CompilationGlobalsScope, UseDiagnistcsLogger etc. Most compiler tasks do use them anyway, so there is little need to alter the existing code.
There was a problem hiding this comment.
I thought this was the reason why the AsyncLocal<..> replacement was used, is it not?
There was a problem hiding this comment.
Yes, what I meant was: on the boundary, for example Async.AwaitNodeCode does this wrapping implicitly.
OTOH AsyncLocal context will always pass across continuations.
The argument for using AsyncLocal is that with NodeCode sometimes this wrapping doesn't work correctly:
#16712
There was a problem hiding this comment.
To clarify further, there are two orthogonal things here, as I understand it:
-
Passing loggers between threads in the flow of an async computation. It's what
NodeCode/AsyncLocaldoes. -
Stacking / unwinding of diagnostics context. This is not really related to async, single threaded compiler tasks can do it. This is generally done explicitly with
UseDiagnosticsLoggerand such, but incidentallyNodeCodedoes this inAwaitNodeCode,AwaitAsync, so it's possible we rely on this behavior somewhere.
| |> NodeCode.AwaitTask | ||
| |> Async.AwaitTask | ||
|
|
||
| // Prevent deadlocks. |
There was a problem hiding this comment.
Again, this is something I'd need help with. I understand that the idea is to run this synchronously whenever possible to get better callstacks, but I've observed this often deadlocks in CI tests.
There was a problem hiding this comment.
It deadlocked on the semaphore above, or somewhere else?
There was a problem hiding this comment.
It froze the CI tests IIRC, I don't think I ever got to repro this one locally, so this is kind of a black box to me.
src/Compiler/Interactive/fsi.fs
Outdated
| @@ -4618,7 +4617,7 @@ type FsiEvaluationSession | |||
| unresolvedReferences, | |||
| fsiOptions.DependencyProvider | |||
| ) | |||
| |> NodeCode.RunImmediateWithoutCancellation | |||
| |> Async.RunSynchronously | |||
There was a problem hiding this comment.
I can't recall why I changed this to RunSynchronously, probably when fighting with fsi deadlocks.
Just yank the NodeCodeBuilder and replace everything with asyncs, without looking back.
This may be useful info generally:
Removing NodeCode reveals some deadlocks. One in
RegisterAndImportReferencedAssemblieswhen called from FSI, there is also some locking inGraphNode. This is easily fixed withAsync.SwitchToThreadPoolbut not easy to pinpoint the cause, as it happens reliably in CI and not that much locally.I'm not sure what is the exact difference here but it appears NodeCode wrappers run some continuations asynchronously where standard Async does not.