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/5.0-rc2] Memory Leak in Microsoft.Extensions.Caching.Memory when handling exceptions #42494

Merged
merged 3 commits into from
Sep 21, 2020

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Sep 19, 2020

Backport of #42355 to release/5.0-rc2

/cc @eerhardt

Customer Impact

Customers reported a memory leak with MemoryCache. It’s possible to get leaks in two cases:

  1. If an exception is thrown from GetOrCreate, we aren’t cleaning up scope lease objects correctly.
  2. When 2 caches have a dependency between them, and the inner cache items are longer lived than the outer, the outer cache items aren’t getting released because the inner cache entries are keeping them rooted.

Testing

New unit tests were added for both scenarios.

Risk

  1. There's still some risk of an exception being thrown after the value was set, such as when setting up the change tokens, timeouts, etc..
  2. Another risk is that if someone is relying on the behavior of not calling SetValue to mean "cache null". In that case, they would need to start calling SetValue(null)

…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.

Fix #42321
…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.
@ghost
Copy link

ghost commented Sep 21, 2020

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

@ericstj ericstj added the Servicing-consider Issue for next servicing release review label Sep 21, 2020
@ericstj ericstj added this to the 5.0.0 milestone Sep 21, 2020
@ericstj
Copy link
Member

ericstj commented Sep 21, 2020

@eerhardt -- did you send mail on this?

@eerhardt
Copy link
Member

@eerhardt -- did you send mail on this?

No. I didn't know we needed to send mail. I thought we just update the top post with the template.

@eerhardt
Copy link
Member

@eerhardt -- did you send mail on this?

I did now.

The test failures are in System.Drawing, and System.Text.Json, which are unrelated to this change.

@jeffhandley jeffhandley added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Sep 21, 2020
@jeffhandley
Copy link
Member

Approved in email. Feel free to merge, @eerhardt.

@eerhardt eerhardt merged commit 25db678 into release/5.0-rc2 Sep 21, 2020
@eerhardt eerhardt deleted the backport/pr-42355-to-release/5.0-rc2 branch September 21, 2020 20:11
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Extensions-Caching Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants