Skip to content
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.

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

Test NodeCode #16576

Merged
merged 9 commits into from
Jan 25, 2024
Merged

Test NodeCode #16576

merged 9 commits into from
Jan 25, 2024

Conversation

majocha
Copy link
Contributor

@majocha majocha commented Jan 23, 2024

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.

@majocha majocha requested a review from a team as a code owner January 23, 2024 20:03
Copy link
Contributor

github-actions bot commented Jan 23, 2024

⚠️ 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

@majocha majocha marked this pull request as draft January 23, 2024 20:15
@majocha majocha marked this pull request as ready for review January 23, 2024 20:44
@majocha majocha marked this pull request as draft January 23, 2024 21:27
@vzarytovskii
Copy link
Member

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
Copy link
Contributor

0101 commented Jan 24, 2024

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

@majocha
Copy link
Contributor Author

majocha commented Jan 24, 2024

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 0101 mentioned this pull request Jan 24, 2024
@0101
Copy link
Contributor

0101 commented Jan 24, 2024

Does this reproduce with anything else than NodeCode.Parallel?

And it was actually used before in IncrementalBuilder.

|> NodeCode.Parallel

|> NodeCode.Parallel

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

@majocha majocha marked this pull request as ready for review January 24, 2024 16:10
@vzarytovskii
Copy link
Member

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
Copy link
Contributor Author

majocha commented Jan 24, 2024

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
Copy link
Contributor

0101 commented Jan 24, 2024

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

@T-Gro T-Gro added the NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes label Jan 25, 2024
@0101 0101 merged commit 4c0a592 into dotnet:main Jan 25, 2024
28 of 29 checks passed
@0101
Copy link
Contributor

0101 commented Jan 25, 2024

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 of an object.
   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 of an object.
   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
Copy link
Contributor Author

majocha commented Jan 25, 2024

ResizeArray is not threadsafe?

@0101
Copy link
Contributor

0101 commented Jan 25, 2024

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

@majocha
Copy link
Contributor Author

majocha commented Jan 25, 2024

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

@majocha
Copy link
Contributor Author

majocha commented Jan 25, 2024

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

@0101
Copy link
Contributor

0101 commented Jan 25, 2024

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
Copy link
Contributor Author

majocha commented Jan 26, 2024

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

@0101
Copy link
Contributor

0101 commented Jan 26, 2024

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
Copy link
Contributor Author

majocha commented Jan 29, 2024

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.

computations
|> Seq.map (fun (Node x) ->
async {
DiagnosticsThreadStatics.DiagnosticsLogger <- diagnosticsLogger
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

majocha commented Feb 14, 2024

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 of an object.
   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 of an object.
   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:

computations
|> Seq.map (fun (Node x) ->
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
Copy link
Contributor

0101 commented Feb 14, 2024

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
Copy link
Contributor Author

majocha commented Feb 14, 2024

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
Copy link
Contributor Author

majocha commented Feb 14, 2024

@0101 see also #16712 😒

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants