Test NodeCode

[majocha]

I had a hunch something's not right with NodeCode:
#16536 (comment)

I suspect with increased parallelization brought by Transparent Compiler it tends to mangle thread statics it's supposed to preserve (?)

So, this is a failing test.

[github-actions]

⚠️ Release notes required, but author opted out

Warning

Author opted out of release notes, check is disabled for this pull request.
cc @dotnet/fsharp-team-msft

[vzarytovskii]

FWIW, I am currently getting rid of node code and replacing it with cancellable tasks, thread static preservation is something I've baked into state machine. But it's far from being finished.

[0101]

Good idea to test this. It definitely looks concerning. I'm surprised it didn't cause us any issues this whole time 🤔

[majocha]

Good idea to test this. It definitely looks concerning. I'm surprised it didn't cause us any issues this whole time 🤔

I think Threadpool mostly queues continuations on the same thread unless really pushed and our stuff is quite sequential. NodeCode.Parallel was added just recently with the graph-based type checking.

[0101]

Does this reproduce with anything else than NodeCode.Parallel?

And it was actually used before in IncrementalBuilder.

fsharp/src/Compiler/Service/IncrementalBuild.fs

Line 760 in 641d0ee

|> NodeCode.Parallel

fsharp/src/Compiler/Service/IncrementalBuild.fs

Line 829 in 641d0ee

|> NodeCode.Parallel

But maybe those places don't need the thread statics.

[vzarytovskii]

Nothing regarding changes in this code, but all the thread static management becomes more and more convoluted. It's really hard to follow-up what's actually going on.

[majocha]

Works now, although I dislike passing state with finally. The intent is unclear and it depends on scheduler implementation.

Do we need release notes for this PR?

[0101]

I think we don't need release notes for this. It doesn't affect the users.

[0101]

Hmm, now with this already merged and built into VSIX, I started seeing:

System.AggregateException : One or more errors occurred. ---> Object reference not set to an instance ofanobject.
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
at async Microsoft.VisualStudio.FSharp.Editor.WorkspaceExtensions.CheckerExtensions.FSharpChecker-ParseAndCheckDocumentUsingTransparentCompiler@355(<Unknown Parameters>)
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
at async Microsoft.VisualStudio.FSharp.Editor.WorkspaceExtensions.CheckerExtensions.FSharpChecker-ParseAndCheckDocument@448(<Unknown Parameters>)
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
at async Microsoft.VisualStudio.FSharp.Editor.WorkspaceExtensions.Document-GetFSharpParseAndCheckResultsAsync@538(<Unknown Parameters>)
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
at async StartupCode$FSharp-Editor(<Unknown Parameters>)
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
at async Microsoft.CodeAnalysis.CodeRefactorings.CodeRefactoringService.<>c__DisplayClass12_0.<GetRefactoringFromProviderAsync>b__0(<Unknown Parameters>)
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
at async Microsoft.CodeAnalysis.Extensions.IExtensionManagerExtensions.PerformFunctionAsync[T](<Unknown Parameters>)--->(Inner Exception #0) System.NullReferenceException : Object reference not set to an instance ofanobject.
at Internal.Utilities.Collections.Utils.replayDiagnostics@31.Invoke(Tuple`2 arg1)
at Microsoft.FSharp.Collections.SeqModule.Iterate[T](FSharpFunc`2 action,IEnumerable`1 source)
at Internal.Utilities.Collections.Utils.replayDiagnostics@31-1.Invoke(IEnumerable`1 source)
at async StartupCode$FSharp-Compiler-Service[TKey,TVersion,TValue,d](<Unknown Parameters>)
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
at async StartupCode$FSharp-Compiler-Service[a,b](<Unknown Parameters>)<---

I can't quite explain it, even if somehow there was no logger set on a thread, we should still get AssertFalseDiagnosticsLogger which will just do nothing. Wonder how it becomes null or where the NRE is actually coming from.

[majocha]

ResizeArray is not threadsafe?

[0101]

True, but access to it is always under lock, unless I missed something. Also can it throw NRE due to a race condition?

[majocha]

Yes, right. There is a lock. We supposedly always initialize the static field to AssertFalseDiagnosticsLogger. That's wierd.

[majocha]

So I'd look at places when we call the setter. Lots of them.

[0101]

Well it seems to have been caused by this PR. I haven't seen it before. Wonder if we somehow depended on the incorrect behavior before.

[majocha]

Now I've seen the exact exception, too, surfacing in VS. But I haven't found a way to reproduce it.

[0101]

Yeah a reliable repro would be nice, but not sure it's even possible. I've tried to get it in debug but couldn't so far.

[majocha]

I'm very sure your code is good.
It seems there are some (compiler?) bugs with how DiagnosticsThreadStatics initialization gets optimized?
I tried to make the initialization more straighforward in #16602, let's see how it goes.

[majocha]

This is not safe, as the logger we're passing is most probably not thread-safe. So we're betting on the computation to actually never use it.

[majocha]

Hmm, now with this already merged and built into VSIX, I started seeing:

System.AggregateException : One or more errors occurred. ---> Object reference not set to an instance ofanobject.
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
at async Microsoft.VisualStudio.FSharp.Editor.WorkspaceExtensions.CheckerExtensions.FSharpChecker-ParseAndCheckDocumentUsingTransparentCompiler@355(<Unknown Parameters>)
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
at async Microsoft.VisualStudio.FSharp.Editor.WorkspaceExtensions.CheckerExtensions.FSharpChecker-ParseAndCheckDocument@448(<Unknown Parameters>)
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
at async Microsoft.VisualStudio.FSharp.Editor.WorkspaceExtensions.Document-GetFSharpParseAndCheckResultsAsync@538(<Unknown Parameters>)
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
at async StartupCode$FSharp-Editor(<Unknown Parameters>)
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
at async Microsoft.CodeAnalysis.CodeRefactorings.CodeRefactoringService.<>c__DisplayClass12_0.<GetRefactoringFromProviderAsync>b__0(<Unknown Parameters>)
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
at async Microsoft.CodeAnalysis.Extensions.IExtensionManagerExtensions.PerformFunctionAsync[T](<Unknown Parameters>)--->(Inner Exception #0) System.NullReferenceException : Object reference not set to an instance ofanobject.
at Internal.Utilities.Collections.Utils.replayDiagnostics@31.Invoke(Tuple`2 arg1)
at Microsoft.FSharp.Collections.SeqModule.Iterate[T](FSharpFunc`2 action,IEnumerable`1 source)
at Internal.Utilities.Collections.Utils.replayDiagnostics@31-1.Invoke(IEnumerable`1 source)
at async StartupCode$FSharp-Compiler-Service[TKey,TVersion,TValue,d](<Unknown Parameters>)
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
at async StartupCode$FSharp-Compiler-Service[a,b](<Unknown Parameters>)<---

I can't quite explain it, even if somehow there was no logger set on a thread, we should still get AssertFalseDiagnosticsLogger which will just do nothing. Wonder how it becomes null or where the NRE is actually coming from.

@0101 I suspect we cannot pass a non-thread safe logger here:

fsharp/src/Compiler/Facilities/BuildGraph.fs

Lines 199 to 208 in 37251d4

	computations
	|> Seq.map (fun(Nodex)->
	async{
	DiagnosticsThreadStatics.DiagnosticsLogger <- diagnosticsLogger
	DiagnosticsThreadStatics.BuildPhase <- phase
	return! x
	})
	|> Async.Parallel
	|> wrapThreadStaticInfo
	|> Node

We're betting on the computation to actually never use it. I guess in some tasks it does use it causing mayhem.

[0101]

Yeah you're right. We should either make it a thread-safe one or pass a separate one to each computation and then merge the results.

[majocha]

Yeah you're right. We should either make it a thread-safe one or pass a separate one to each computation and then merge the results.

Yes, I think I had something like this in the AsyncLocal branch.

[majocha]

@0101 see also #16712 😒