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

[release/6.0] Ensure that tls_destructionMonitor is initialized if a thread is attached to the runtime. #65406

Merged
merged 4 commits into from
Mar 8, 2022

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Feb 15, 2022

Backport of #65358 to release/6.0
Fixes:#64509

/cc @VSadov

Customer Impact

In a case when a thread has not been created by the runtime, but instead "wandered" into managed code - by the means of reverse p-invoke or the like, we do not have an entry point frame that would perform pre-mortem cleanup for the data that VM associates with the thread when the thread exits.
Instead we rely on c++ destructor for a thread-static variable that calls cleanup.

The issue here is that thread-static variable could be initialized lazily (it was observed on libc Linux) - if there is no "use" of the variable, there is no initialization and thus there is no destruction.

As a result the cleanup code is not called and we have a small, but steady leak every time a native thread that has seen managed code is subsequently terminated.

This change "uses" the thread-static sentinel variable whenever a thread is attached to the runtime, thus making the destructor called reliably.

Testing

I have verified manually that

  • the repro scenario does cause a slow but steady memory leak (on Ubuntu 16.04 with 6.0 app)
  • there is no leak after this fix is applied.

Risk

Low. We use the same cleanup strategy as before.
The change primarily ensures that destructor call cannot be optimized away if a thread was attached to the runtime.

@VSadov VSadov added the Servicing-consider Issue for next servicing release review label Feb 15, 2022
@VSadov VSadov requested a review from jkotas February 15, 2022 23:12
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.

Approved. We should consider for 6.0.x

@jeffschwMSFT jeffschwMSFT added this to the 6.0.x milestone Feb 22, 2022
@rbhanda rbhanda modified the milestones: 6.0.x, 6.0.4 Feb 22, 2022
@rbhanda rbhanda added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Feb 22, 2022
@carlossanlop carlossanlop merged commit c746820 into release/6.0 Mar 8, 2022
@carlossanlop carlossanlop deleted the backport/pr-65358-to-release/6.0 branch March 8, 2022 21:00
@ghost ghost locked as resolved and limited conversation to collaborators Apr 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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