Skip to content

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Apr 4, 2025

Backport of #114227 to release/9.0-staging

Fixes: #113949

/cc @jkotas @VSadov

Customer Impact

A stress issue that under certain conditions may result in a circular dependency between contending/waiting on a Lock and performing static field initialization in System.Threading.Thread.
The result is a stack overflow crash.

  • Customer reported
  • Found internally

The issue was reported by internal team after examining WER crash buckets.

Regression

  • Yes
  • No

This is a result of two features -

  • GetCurrentProcessorId() API that indirectly introduced a static field initialization in System.Thread
  • managed Lock implementation with implicit assumption on NativeAOT that waiting on a lock will not run static constructors.

The root cause is not new, but it is a stress bug that is relatively difficult to observe and also requires an uncommon activation pattern for NativeAOT - dynamic library mode vs. more common regular .exe.

Testing

Ordinary unit tests. Examined that System.Threading.Thread no longer has a .cctor

Risk

Very low risk. The fix is basically moving one private static field to a different type.

@ghost ghost added the area-System.Threading label Apr 4, 2025
Copy link
Contributor

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

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

@teo-tsirpanis
Copy link
Contributor

Would it make sense to add a unit test that typeof(Thread) does not have a static constructor?

@VSadov
Copy link
Member

VSadov commented Apr 4, 2025

Would it make sense to add a unit test that typeof(Thread) does not have a static constructor?

I will consider as a follow up change.
(in the main branch, probably no need to backport that)

Thanks for suggestion!

@carlossanlop
Copy link
Contributor

Friendly reminder that code complete for the May release is next Monday April 14th. If you want this change included in that release, please take it to Tactics and merge the PR before EOD Monday.

@VSadov VSadov added the Servicing-consider Issue for next servicing release review label Apr 14, 2025
@jeffschwMSFT jeffschwMSFT added this to the 9.0.x milestone Apr 14, 2025
@jeffschwMSFT jeffschwMSFT added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Apr 14, 2025
@jeffschwMSFT jeffschwMSFT modified the milestones: 9.0.x, 9.0.5 Apr 14, 2025
@VSadov
Copy link
Member

VSadov commented Apr 14, 2025

Thanks!

@VSadov VSadov merged commit 8f4c768 into release/9.0-staging Apr 14, 2025
166 of 183 checks passed
@VSadov VSadov deleted the backport/pr-114227-to-release/9.0-staging branch April 14, 2025 21:41
@github-actions github-actions bot locked and limited conversation to collaborators May 15, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Threading Servicing-approved Approved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants