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

Memory Leak!! with CaffeineCache #556

Open
rahul8383 opened this issue Jul 18, 2021 · 3 comments
Open

Memory Leak!! with CaffeineCache #556

rahul8383 opened this issue Jul 18, 2021 · 3 comments

Comments

@rahul8383
Copy link

What happened:

  /**
    * Create a new Caffeine cache
    */
  def apply[V](implicit config: CacheConfig): CaffeineCache[V] =
    apply(Caffeine.newBuilder().build[String, Entry[V]]())

When the above Companion Object is used to instantiate CaffeineCache, a cache entry with TTL is not evicted by the underlying Caffeine even though CaffeineCaches doGet() call returns None after the TTL expiry.

When the CaffeineCache returns None, it makes the end user think as if the cache entry was evicted. But the entry is still present in the Heap and could not be garbage collected piling up Memory Usage as the underlying Caffeine is not set with any eviction policy.

What you expected to happen:

When the CaffeineCache doGet() call returns None, the cache entry should either be evicted or should be evicted in the next maintenance run of Caffeine.

How to reproduce it (as minimally and precisely as possible):

  1. I was able to reproduce the issue locally using 0.28.0 version.
  2. Instantiate CaffeineCache using Default Companion Object. val caffeinecache = CaffeineCache[CacheDetails]
  3. Add entries to the cache with TTL using the doPut() call.
  4. Check Heap Usage of the Program. The number of bytes used by the Cache Entry Object increases even though TTL was set with every doPut() call.

Anything else we need to know?:

Caffeine.newBuilder().build[String, Entry[V]]() doesn't add any eviction policy by default. Hence, the cache entries remain in Heap even after TTL expiry.

scalacache does provide another companion object interface where the underlying Caffeine Object can be passed by the end-user. To not encounter this issue, the Caffeine object can be created by the end-user with an appropriate eviction policy.

  /**
    * Create a new cache utilizing the given underlying Caffeine cache.
    *
    * @param underlying a Caffeine cache
    */
  def apply[V](underlying: CCache[String, Entry[V]])(implicit config: CacheConfig): CaffeineCache[V] =
    new CaffeineCache(underlying)

How can we resolve the issue?:

These are various ways I could think of. I am sure there could be better ways.

  1. When a cache key is expired, remove the key in doGet() call. The framework currently returns None. Note: There could be performance implications with this approach in applications with heavy reads - not a good way to approach this issue.
  2. Remove setting TTL for every doPut() call. Remove the second layer of expiry check in CaffeineCache in doGet() call. Take TTL as a parameter while instantiating CaffeineCache and set the TTL in the underlying Caffeine object. For more advanced users, we can take the APIs exposed by Caffeine for implementing eviction policies as an Optional parameter and let Caffeine handle the eviction of the Key.
@zarthross
Copy link
Contributor

Resolving the issue with idea 1, 'When the cache key is expired, remove the key in the doGet()' probably is bad because if a doGet was just called, its likely a doPut is to follow, overriding the value anyways.

2 isn't great either, because I would expect my scalacache to behave the same regardless of what the underlying cache is.

Perhaps a middle ground would be to add some settings to the Caffeine.apply methods, with sensible defaults, such as use a weak or soft underlying cache by default. This would prevent the OOM, but keep the behavior the same anyways. We could allow the user to set a global TTL if they want as well.

@petersondrew
Copy link

I'm getting started with ScalaCache + Caffeine and I'm trying to understand the implications of this. I'm unclear as to whether the TTL on ScalaCache's put is honored and setting something like maximumSize on the underlying Caffeine cache is sufficient, or if the implication is that only setting a cache-level TTL on the underlying Caffeine cache via expireAfterWrite works to enforce a TTL?

@zarthross
Copy link
Contributor

zarthross commented Oct 18, 2021

@petersondrew The put ttl is honored in that if you do a get and the ttl is expired you get None, but the memory isn't freed after the TTL when using the default Caffeine settings. Settings something like maximumSize or using weakValues or softValues on the Caffiene store will prevent the OOM from happening.

Really it boils down to when the memory is freed. As far as the get goes it respects the TTL just fine so the behavior from a cache perspective is fine. (Hope that helps)

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

No branches or pull requests

3 participants