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

Keep actual parameter values out of CommandCacheKey in RelationalCommandCache #34631

Closed
wants to merge 1 commit into from

Conversation

cliffankh0z
Copy link

@cliffankh0z cliffankh0z commented Sep 9, 2024

Memory leak when storing the actual parameter values in MemoryCache in CommandCacheKey. Parameter values can be anything used as parameteres for the queries; lists of Guids, even objects used for entity inits etc. This keeps them from being garbage collected and thereby creating memory leaks. Store only what we actually need to compare parameter values.

Anyway, the intent of the code is clear. If a maintainer could fix this in the correct manner and style that would be greatly appreciated!

  • [x ] I've read the guidelines for contributing and seen the walkthrough
  • I've posted a comment on an issue with a detailed description of how I am planning to contribute and got approval from a member of the team
  • The code builds and tests pass locally (also verified by our automated build checks)
  • Commit messages follow this format:
        Summary of the changes
        - Store only the data we need

        Fixes #34028
  • Tests for the changes have been added (for bug fixes / features)
  • Code follows the same patterns and style as existing code in this repo

#34028

Only store necessary info for paramter values RelationalCommandCacheKey to prevent memory leaks.
@cliffankh0z
Copy link
Author

@dotnet-policy-service agree

@dotnet-policy-service agree

@@ -106,11 +106,16 @@ void IPrintableExpression.Print(ExpressionPrinter expressionPrinter)
}
}

private readonly struct CommandCacheKey(Expression queryExpression, IReadOnlyDictionary<string, object?> parameterValues)
Copy link
Author

@cliffankh0z cliffankh0z Sep 9, 2024

Choose a reason for hiding this comment

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

parameterValues contains all user data objects in queries before AsEnumerable or ToList, handle as radioactive. We only need meta data anyway

@cliffankh0z
Copy link
Author

cliffankh0z commented Sep 11, 2024

It may not seem much, but this a confirmed problem in production for us. Clearing IMemoryCache instantly drops memory by 600mb in some cases. If you are unlucky and a huge query is executed while creating this cache entry for the first time, all those entities will remain in memory through some wierd links that GC deems still alive objects.
In huge codebases it is hard to discern which queries are problematic, as all parameter values in IQueryable, before AsEnumerable or ToList is called will we considered parameterer values here. Sometimes the argument / parameter value can look like IEnumerable-Guid- and look benign, but it is actually a selection on some other entites, so those are now put into IMemoryCache.
But EF Core should handle query parameters with gloves anyway and not chain them up in a memory cache when we only need meta data on the parameters.

To debug the issue we used optionsBuilder.UseMemoryCache(_memoryCache); to easily be able to inspect the same IMemoryCache as EF uses

// ReSharper disable once ArrangeRedundantParentheses
if ((value == null) != (otherValue == null))
{
if (value != otherValue)
Copy link
Author

Choose a reason for hiding this comment

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

Keep old behaviour, where we return true on first object[], or compare all values?

@yinzara
Copy link
Contributor

yinzara commented Oct 1, 2024

This PR lacks unit tests to verify that it fixes the memory leak issue described.

@yinzara
Copy link
Contributor

yinzara commented Oct 1, 2024

@cliffankh0z you can just abandon this PR now in favor of:
#34803

Thank you very much for doing the initial work for this but I've got it now reproduced with unit tests and fixed as well.

@cliffankh0z cliffankh0z closed this Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants