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

Don't wait for finalizers in 'IReferenceTrackerHost::ReleaseDisconnectedReferenceSources' #110551

Merged

Conversation

Sergio0694
Copy link
Contributor

Contributes to #109538

This PR updates the IReferenceTrackerHost::ReleaseDisconnectedReferenceSources implementation for CoreCLR and NativeAOT to match what .NET Native was doing, and not wait for finalizers, to avoid deadlocks in ASTA scenarios (UWP). Finalizers will just continue running normally, and if the process is suspended (which can only happen on UWP anyway), they'll resume when the process is resumed later on. Worst case scenario this would only cause a non-optimal memory use cleanup before suspension (which is better than a deadlock anyway). Can be revisited and improved for .NET 10 if needed.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Dec 9, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.

@Sergio0694 Sergio0694 requested a review from jkotas December 9, 2024 21:12
@Sergio0694
Copy link
Contributor Author

@jkotas I'm targeting main and can then cherry-pick into release/9.0, is this the right workflow for this repo or should I be targeting the release branch first and then cherry-pick from there into main later? I can do whichever is preferred 🙂

@jkotas
Copy link
Member

jkotas commented Dec 9, 2024

I'm targeting main and can then cherry-pick into release/9.0, is this the right workflow for this repo

Yes, the right workflow for this repo is to merge the change into main first.

Copy link
Member

@jkoritzinsky jkoritzinsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a comment wording nit, otherwise LGTM

@jkotas jkotas merged commit 68da1a4 into dotnet:main Dec 10, 2024
85 of 88 checks passed
@jkotas
Copy link
Member

jkotas commented Dec 10, 2024

/backport to release/9.0-staging

Copy link
Contributor

Started backporting to release/9.0-staging: https://github.com/dotnet/runtime/actions/runs/12247939420

hez2010 pushed a commit to hez2010/runtime that referenced this pull request Dec 14, 2024
…tedReferenceSources' (dotnet#110551)

This PR updates the IReferenceTrackerHost::ReleaseDisconnectedReferenceSources implementation for CoreCLR and NativeAOT to match what .NET Native was doing, and not wait for finalizers, to avoid deadlocks in ASTA scenarios (UWP). Finalizers will just continue running normally, and if the process is suspended (which can only happen on UWP anyway), they'll resume when the process is resumed later on. Worst case scenario this would only cause a non-optimal memory use cleanup before suspension (which is better than a deadlock anyway). Can be revisited and improved for .NET 10 if needed.

Contributes to dotnet#109538
Sergio0694 added a commit to Sergio0694/runtime that referenced this pull request Jan 7, 2025
…tedReferenceSources' (dotnet#110551)

This PR updates the IReferenceTrackerHost::ReleaseDisconnectedReferenceSources implementation for CoreCLR and NativeAOT to match what .NET Native was doing, and not wait for finalizers, to avoid deadlocks in ASTA scenarios (UWP). Finalizers will just continue running normally, and if the process is suspended (which can only happen on UWP anyway), they'll resume when the process is resumed later on. Worst case scenario this would only cause a non-optimal memory use cleanup before suspension (which is better than a deadlock anyway). Can be revisited and improved for .NET 10 if needed.

Contributes to dotnet#109538
@github-actions github-actions bot locked and limited conversation to collaborators Jan 9, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Interop-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants