-
Notifications
You must be signed in to change notification settings - Fork 802
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
Test NodeCode #16576
Conversation
|
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. |
Good idea to test this. It definitely looks concerning. I'm surprised it didn't cause us any issues this whole time 🤔 |
tests/FSharp.Compiler.ComponentTests/CompilerService/AsyncMemoize.fs
Outdated
Show resolved
Hide resolved
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. |
Does this reproduce with anything else than And it was actually used before in
But maybe those places don't need the thread statics. |
Co-authored-by: Petr Pokorny <petr@innit.cz>
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. |
Works now, although I dislike passing state with Do we need release notes for this PR? |
tests/FSharp.Compiler.ComponentTests/CompilerService/AsyncMemoize.fs
Outdated
Show resolved
Hide resolved
I think we don't need release notes for this. It doesn't affect the users. |
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 |
ResizeArray is not threadsafe? |
True, but access to it is always under lock, unless I missed something. Also can it throw NRE due to a race condition? |
Yes, right. There is a lock. We supposedly always initialize the static field to AssertFalseDiagnosticsLogger. That's wierd. |
So I'd look at places when we call the setter. Lots of them. |
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. |
Now I've seen the exact exception, too, surfacing in VS. But I haven't found a way to reproduce it. |
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. |
I'm very sure your code is good. |
computations | ||
|> Seq.map (fun (Node x) -> | ||
async { | ||
DiagnosticsThreadStatics.DiagnosticsLogger <- diagnosticsLogger |
There was a problem hiding this comment.
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.
@0101 I suspect we cannot pass a non-thread safe logger here: fsharp/src/Compiler/Facilities/BuildGraph.fs Lines 199 to 208 in 37251d4
We're betting on the computation to actually never use it. I guess in some tasks it does use it causing mayhem. |
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. |
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.