Skip to content

Memory Leak in Microsoft.Extensions.Caching.Memory when handling exceptions - #42355

Merged
eerhardt merged 3 commits into
dotnet:masterfrom
eerhardt:Fix42321
Sep 19, 2020
Merged

Memory Leak in Microsoft.Extensions.Caching.Memory when handling exceptions#42355
eerhardt merged 3 commits into
dotnet:masterfrom
eerhardt:Fix42321

Conversation

@eerhardt

@eerhardteerhardt commented Sep 16, 2020

Copy link
Copy Markdown
Member

When an exception is thrown inside MemoryCache.GetOrCreate, we are leaking CacheEntry objects. This is because they are not being Disposed properly, and the async local CacheEntryStack is growing indefinitely.

The fix is to ensure the CacheEntry objects are disposed correctly. In order to do this, I set a flag to indicate whether the CacheEntry.Value has been set. If it hasn't, Disposing the CacheEntry won't add it to the cache.

Fix#42321

@eerhardt
Memory Leak in Microsoft.Extensions.Caching.Memory when handling exce…
3ebbb0e
…ptions
When an exception is thrown inside MemoryCache.GetOrCreate, we are leaking CacheEntry objects. This is because they are not being Disposed properly, and the async local CacheEntryStack is growing indefinitely.
The fix is to ensure the CacheEntry objects are disposed correctly. In order to do this, I set a flag to indicate whether the CacheEntry.Value has been set. If it hasn't, Disposing the CacheEntry won't add it to the cache.
Fixdotnet#42321
@Dotnet-GitSync-Bot

Dotnet-GitSync-Bot commented Sep 16, 2020

Copy link
Copy Markdown
Collaborator

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost

ghost commented Sep 16, 2020

Copy link
Copy Markdown

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

Tratcher
Tratcher reviewed Sep 17, 2020
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.Caching.Abstractions/src/MemoryCacheExtensions.cs Outdated
Comment threadsrc/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs Outdated
stephentoub
stephentoub reviewed Sep 17, 2020
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.Caching.Abstractions/src/MemoryCacheExtensions.cs Outdated
This was referenced Sep 17, 2020
Merged
Merged
@eerhardt

eerhardt commented Sep 17, 2020

Copy link
Copy Markdown
MemberAuthor

I believe all current feedback has been addressed. I also renamed 2 variables to better describe what they are doing. PTAL.

@eerhardt
Fix another memory leak when one cache depends on another cache. The …
72d5741
…inner cache's entries will reference the outer cache entries through the ScopeLease object.
Null'ing out the CacheEntry._scope field when it is disposed fixes this issue.
@eerhardteerhardt mentioned this pull request Sep 17, 2020
Closed
@eerhardt
eerhardt merged commit 63c3dd1 into dotnet:masterSep 19, 2020
@eerhardt
eerhardt deleted the Fix42321 branch September 19, 2020 15:40
@eerhardt

eerhardt commented Sep 19, 2020

Copy link
Copy Markdown
MemberAuthor

/backport to release/5.0-rc2

@github-actions

github-actionsBot commented Sep 19, 2020

Copy link
Copy Markdown
Contributor

Started backporting to release/5.0-rc2: https://github.com/dotnet/runtime/actions/runs/262733314

@github-actionsgithub-actionsBot mentioned this pull request Sep 19, 2020
Merged
This was referenced Oct 7, 2020
Closed
Closed
@maryamariyanmaryamariyan mentioned this pull request Oct 7, 2020
Closed
@ghostghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stephentoubstephentoubstephentoub left review comments
@wtgodbewtgodbewtgodbe left review comments
@TratcherTratcherTratcher approved these changes
@davidfowldavidfowlAwaiting requested review from davidfowl
@maryamariyanmaryamariyanAwaiting requested review from maryamariyan
@ericstjericstjAwaiting requested review from ericstj
+1 more reviewer
@JunTaoLuoJunTaoLuoJunTaoLuo approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-Extensions-Caching

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Memory Leak in Microsoft.Extensions.Caching.Memory when handling exceptions

8 participants

@eerhardt@Dotnet-GitSync-Bot@Tratcher@JunTaoLuo@stephentoub@wtgodbe@maryamariyan@ericstj