-
Notifications
You must be signed in to change notification settings - Fork 5.1k
[9.0] Backport 115546 FLS initialization fix to 9. #116872
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
Conversation
… SetupThread() (#115546) * fix * PR feedback Co-authored-by: Jan Kotas <jkotas@microsoft.com> * moved profiler init before FinalizerThreadCreate * Update src/coreclr/vm/ceemain.cpp Co-authored-by: Jan Kotas <jkotas@microsoft.com> * create finalizer thread earlier only on Windows. * Update src/coreclr/vm/ceemain.cpp Co-authored-by: Jan Kotas <jkotas@microsoft.com> --------- Co-authored-by: Jan Kotas <jkotas@microsoft.com>
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.
Pull Request Overview
This PR backports a fix to the FLS initialization for version 9.0 by reordering the finalizer thread initialization to occur before threads are created and registered for OS destruction.
- Moves FinalizerThread::WaitForFinalizerThreadStart() earlier in EEStartupHelper()
- Adjusts the global exception handling and thread setup order
- Removes the obsolete SyncBlockCache::Attach() implementation
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
File | Description |
---|---|
src/coreclr/vm/syncblk.cpp | Removed the unused SyncBlockCache::Attach() function. |
src/coreclr/vm/finalizerthread.cpp | Added a wait call to synchronize finalizer thread initialization. |
src/coreclr/vm/ceemain.cpp | Reordered initialization steps to ensure FLS and COM are set up properly. |
Comments suppressed due to low confidence (1)
src/coreclr/vm/ceemain.cpp:906
- [nitpick] Consider adding an inline comment that explains the dependency order between waiting for the finalizer thread to start and subsequent calls (e.g., SetupThread()), to aid future maintainers in understanding the sequencing rationale.
InitializeExceptionHandling();
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.
lgtm. we will take for consideration in 9.0.x
Tagging subscribers to this area: @mangod9 |
Backports #115546 to 9.0.
Summary
In the previous fix we tried to call FinalizerThread::WaitForFinalizerThreadStart(); as late as possible in EEStartupHelper() - to make sure finalizer has enough time to do its part in initializing COM and FLS slot.
But if calls that happen before FinalizerThread::WaitForFinalizerThreadStart(); can result in Thread objects created and registered for OS destruction, then those threads will see g_flsIndex != FLS_OUT_OF_INDEXES, that includes the call to SetupThread(), which EEStartupHelper() itself does.
The change moves FinalizerThread::WaitForFinalizerThreadStart(); earlier, before any Threads could be created and registered for OS destruction.
We still have GC heap initialization happening before that, which will typically result in finalizer thread completing its part before the EE init thread needs to check for the completion.
Customer Impact
Found by customer here: #116755
Regression
Testing
Validated by the customer who reported it: #116755 (comment)
Risk
Medium. Touches critical code but this fixes an issue with an earlier servicing fix: #113055.