-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Fixed memory leak due to infinite static caches #2438
base: master
Are you sure you want to change the base?
Fixed memory leak due to infinite static caches #2438
Conversation
Use MemoryCache with fixed SlidingExpiration. Increasing the required framework version to 4.5.1 mbdavid#2421
LiteDB/LiteDB.csproj
Outdated
</ItemGroup> | ||
|
||
<ItemGroup Condition="'$(TargetFramework)' == 'netstandard1.3'"> | ||
<PackageReference Include="System.Reflection.TypeExtensions" Version="4.5.1" /> | ||
<PackageReference Include="System.Security.Cryptography.Algorithms" Version="4.3.1" /> | ||
<PackageReference Include="Microsoft.Extensions.Caching.Memory" Version="1.1.2" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not a maintainer of this lib, but I doubt adding Microsoft.Extensions.Caching.Memory dependency would be approved. Implementing cache expiration logic relying on what is available in BCL may be needed.
Also, since this is a general purpose library, few concerns regarding sliding cache expiration approach:
- It introduces implicit dependency on static DateTime.Now or similar, which may become a problem for unit testing (not only LiteDB itself, but any other projects that uses LiteDB in tests). Microsoft.Extensions.Caching.Memory has an ISystemClock abstraction. If LiteDB library would rely on system time for caching logic, there should be a way to supply similar abstraction to LiteDB, so implementation could be substituted if needed.
- The hard decision to have sliding expiration exactly to 1 minute may need to be parametrized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your review.
"dependency would be approved" - Absolutely agree. I'll try to rewrite this when I have some free time.
"which may become a problem for unit testing" - I absolutely agree, but I still have no idea how to fix this without seriously reworking the project architecture. Static methods and static caches are not the best solution.
"need to be parametrized" - Thanks, done. At the same time, now by default the parameter does not change the existing logic - indefinite cache
This reverts commit 1061987. # Conflicts: # LiteDB/Document/Expression/BsonExpression.cs # LiteDB/LiteDB.csproj
Revert all changes. |
Although I am not 100% sure, I am afraid this cache does not solve the problem. Would you mind providing unit tests and benchmarks (+results)? |
Use MemoryCache with fixed SlidingExpiration.
Increasing the required framework version to 4.5.1
Not the best solution, but better than endlessly eating up memory
#2421