Skip to content

[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

Merged
merged 2 commits into from
Jun 27, 2025

Conversation

mangod9
Copy link
Member

@mangod9 mangod9 commented Jun 20, 2025

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

  • Customer reported
  • Found internally

Found by customer here: #116755

Regression

  • Yes
  • No

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.

VSadov and others added 2 commits June 20, 2025 10:59
… 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>
Copy link
Contributor

@Copilot Copilot AI left a 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();

Copy link
Member

@jeffschwMSFT jeffschwMSFT left a 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

@jeffschwMSFT jeffschwMSFT added Servicing-consider Issue for next servicing release review area-VM-coreclr labels Jun 25, 2025
@jeffschwMSFT jeffschwMSFT added this to the 9.0.x milestone Jun 25, 2025
Copy link
Contributor

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

@rbhanda rbhanda modified the milestones: 9.0.x, 9.0.8 Jun 26, 2025
@rbhanda rbhanda added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jun 26, 2025
@mangod9 mangod9 merged commit b161846 into release/9.0-staging Jun 27, 2025
28 of 32 checks passed
@mangod9 mangod9 deleted the backpott_115546 branch June 27, 2025 06:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-VM-coreclr Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants