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

[.NET 7] Fix spurious OOMs after upgrading to 7 #79158

Merged
merged 2 commits into from
Jan 5, 2023

Conversation

mangod9
Copy link
Member

@mangod9 mangod9 commented Dec 2, 2022

Description

This is fixing the issue (#78959) where customers are noticing spurious OOMs within their applications after upgrading to 7.

This is porting two PRs into 7:
#77478
#78973

Problem was that a BGC was in progress when the full GC was requested by setting the last_gc_before_oom flag, and at the end of the BGC we turned off the last_gc_before_oom flag.

Fix is simply not to turn off the flag in the case of BGC. This is a slightly incomplete fix hence the next PR (#78973) was required

Customer Impact

Reported by multiple customers where they are noticing OOMs after upgrading to .NET 7. Two customers have tried the private fix and confirmed it fixes the issue.

Regression

This is a regression from the 6.0.x version since the bug exists in Regions code.

Testing

Validated by at least two customers who had reported the issue.

…t#77478)

Problem was that a BGC was in progress when the full GC was requested by setting the last_gc_before_oom flag, and at the end of the BGC we turned off the last_gc_before_oom flag.

Fix is simply not to turn off the flag in the case of BGC.
… 1 GC. (dotnet#78973)

This is a refinement suggested by Maoni for earlier PR#77478 - the last_gc_before_oom flag should also not be reset by an intervening gen 1 GC.

This was noticed by Maoni due to a customer issue with spurious OOM exceptions with .NET Core 7.
@ghost ghost assigned mangod9 Dec 2, 2022
@mangod9 mangod9 marked this pull request as draft December 2, 2022 16:42
@ghost
Copy link

ghost commented Dec 2, 2022

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

Issue Details

null

Author: mangod9
Assignees: -
Labels:

area-GC-coreclr

Milestone: -

@PeterSolMS
Copy link
Contributor

Looks good to me.

@mangod9 mangod9 marked this pull request as ready for review January 4, 2023 20:39
@mangod9 mangod9 changed the title [Draft] Fix spurious OOMs [.NET 7] Fix spurious OOMs Jan 4, 2023
@mangod9 mangod9 changed the title [.NET 7] Fix spurious OOMs [.NET 7] Fix spurious OOMs after upgrading to 7 Jan 4, 2023
@carlossanlop
Copy link
Member

@mangod9 if you haven't yet done so, please send the email to Tactics, and add the servicing-consider label.

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 will take for consideration in 7.0.x

@jeffschwMSFT jeffschwMSFT added the Servicing-consider Issue for next servicing release review label Jan 4, 2023
@jeffschwMSFT jeffschwMSFT added this to the 7.0.x milestone Jan 4, 2023
@rbhanda rbhanda modified the milestones: 7.0.x, 7.0.3 Jan 5, 2023
@rbhanda rbhanda added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jan 5, 2023
@carlossanlop
Copy link
Member

Approved by Tactics (7.0.3).
Signed off by area owners.
No OOB changes needed.
CI green.
Ready to merge. :shipit:

@carlossanlop carlossanlop merged commit 8db9e3d into dotnet:release/7.0 Jan 5, 2023
@hoerup
Copy link

hoerup commented Jan 11, 2023

Was this fix included in 7.0.2 release ?

@mangod9
Copy link
Member Author

mangod9 commented Jan 11, 2023

It missed the window for 7.0.2, will be included in 7.0.3

@ghost ghost locked as resolved and limited conversation to collaborators Feb 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-GC-coreclr Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants