Updates cache api to make getOrElseUpdate methods atomic #9615

gurkankaymak · 2019-09-02T17:49:46Z

Hi, updated cache api to make getOrElseUpdate methods atomic

I don't know why AsyncCacheApi does not use the AsyncCache of Caffeine, that way AsyncCacheApi.getOrElseUpdate could lazily evaluate orElse part.

I'm not sure if the current implementation of AsyncCacheApi.getOrElseUpdate is acceptable, I couldn't find a way to make it atomic and evaulate orElse lazily with the sync api. Maybe just for this method the previous way could more preferable (get-if not found-put instead of atomic)

I could update the pr according to your comments/recommendations

ben-manes · 2019-09-03T06:37:14Z

cache/play-caffeine-cache/src/main/scala/play/api/cache/caffeine/CaffeineCacheApi.scala

-      case Some(value) => Future.successful(value)
-      case None        => orElse.flatMap(value => set(key, value, expiration).map(_ => value))
-    }
+    orElse.map(orElseValue => sync.getOrElseUpdate(key, expiration)(orElseValue))


Does this always compute a new future and maybe add it to the cache?

actually it should be computed and put if there is no value with the given key

It just reads to me, not having written scala in many years, that it always computes a new future value. I think the intent is to only compute if absent. Could this class use the AsyncCache in caffeine instead of the synchronous one?

I asked the same question in pr description, that bothered me as well 😃

I don't know why AsyncCacheApi does not use the AsyncCache of Caffeine, that way AsyncCacheApi.getOrElseUpdate could lazily evaluate orElse part

I couldn't find a way to make it atomic and evaulate orElse lazily with the sync api.

I don't know why AsyncCacheApi does not use the AsyncCache of Caffeine, that way AsyncCacheApi.getOrElseUpdate could lazily evaluate orElse part

I would say the main reason is that then access from both async and sync cache can keep cached data consistent. It is also more convenient to not implement them both from scratch so it is less code to maintain, test, etc.

But I suspect we can implement play.cache.caffeine.NamedCaffeineCache in terms of com.github.benmanes.caffeine.cache.AsyncCache instead and have some sync methods that will just wrap values using CompletableFuture.completedFuture. Another possible constraint is if EhCache has a way to do that.

Note that AsyncCache has a synchronous() method to provide a Cache view. The async cache internally maintains K=>Future<V> and the synchronous view tries to provide a K=>V view by blocking on the future as appropriate. That may be useful in simplifying the code.

That makes sense to me.

It would be better to open a new PR though, so it will be focused on this other change and easier to review.

ok, I'll open a new pr for using com.github.benmanes.caffeine.cache.AsyncCache and update on this pr on top of that, thanks @ben-manes, @marcospereira

hi @marcospereira, I created a pr for this #9623

marcospereira · 2019-09-03T22:19:49Z

cache/play-caffeine-cache/src/main/scala/play/api/cache/caffeine/DefaultCaffeineExpiry.scala

+    currentDuration
+  }
+
+  def expireAfterRead(key: String, value: ExpirableCacheValue[Any], currentTime: Long, currentDuration: Long): Long = {


If I understand correctly, this will reset the duration for every read, which is not the existing behavior for policy().expireVariably().get().put(...). Is that correct, @ben-manes?

This is called on every read, however this implementation returns currentDuration. This indicates that no change to the entry's lifetime should occur due to this read, and the cache will not modify the lifetime.

It looks like this configuration is that a lifetime is set by its creation time, and not modified thereafter. The set(k, v, duration) may modify the entry's lifetime on an update. So I believe that the expireAfterUpdate may need similar logic to expireAfterCreate.

You're right policy().expireVariably().get().put does not use CaffeineDefaultExpiry, it uses a custom Expiry that behaves same on expireAfterCreate and expireAfterUpdate methods

marcospereira · 2019-09-03T22:30:41Z

cache/play-caffeine-cache/src/main/scala/play/api/cache/caffeine/CaffeineCacheApi.scala

-        cache.policy().expireVariably().get().put(key, value, Long.MaxValue, TimeUnit.DAYS)
-      case finite: FiniteDuration =>
-        val seconds = finite.toSeconds
-        if (seconds <= 0) {


I don't think we are handling this case where the duration is negative anymore. Ideally, if there is no test for it, we should add one to ensure we don't have a regression here.

marcospereira · 2019-09-03T22:49:09Z

cache/play-caffeine-cache/src/main/scala/play/api/cache/caffeine/CaffeineCacheApi.scala

-      case Some(value) => Future.successful(value)
-      case None        => orElse.flatMap(value => set(key, value, expiration).map(_ => value))
-    }
+    orElse.map(orElseValue => sync.getOrElseUpdate(key, expiration)(orElseValue))


I don't know why AsyncCacheApi does not use the AsyncCache of Caffeine, that way AsyncCacheApi.getOrElseUpdate could lazily evaluate orElse part

I would say the main reason is that then access from both async and sync cache can keep cached data consistent. It is also more convenient to not implement them both from scratch so it is less code to maintain, test, etc.

But I suspect we can implement play.cache.caffeine.NamedCaffeineCache in terms of com.github.benmanes.caffeine.cache.AsyncCache instead and have some sync methods that will just wrap values using CompletableFuture.completedFuture. Another possible constraint is if EhCache has a way to do that.

…e interface as discussed playframework#9615

updated cache apis to make getOrElseUpdate methods atomic

…efaultExpiry.java for this reason "class play.cache.caffeine.CaffeineDefaultExpiry does not have a correspondent in current version"

… top of playframework#9623

…xpiry" from binary compatibility

mkurz · 2019-10-02T21:35:12Z

@gurkankaymak I saw you pushed some commits here. Is there still something missing or is this pull request ready for another review?

…he getOrElseUpdate method

gurkankaymak · 2019-10-03T06:49:59Z

@gurkankaymak I saw you pushed some commits here. Is there still something missing or is this pull request ready for another review?

@mkurz just pushed a new commit in the EhCache part, pr could be reviewed now

marcospereira

Thanks, @gurkankaymak.

I think it will also be useful to add some details about this change in our migration guide. For example, when using EhCache, the orElse part is now eargerly computed even when the value is present. This may be surprising to users, and since we plan to move EhCache support out of Play, I would say that it would be better to not bother changing it (the migration section can then focus exclusively on Caffeine implementation which is the default one).

WDYT?

cache/play-caffeine-cache/src/main/java/play/cache/caffeine/CaffeineDefaultExpiry.java

cache/play-caffeine-cache/src/main/scala/play/api/cache/caffeine/CaffeineCacheApi.scala

cache/play-caffeine-cache/src/main/scala/play/api/cache/caffeine/DefaultCaffeineExpiry.scala

cache/play-caffeine-cache/src/main/scala/play/api/cache/caffeine/ExpirableCacheValue.scala

cache/play-caffeine-cache/src/test/scala/play/api/cache/caffeine/CaffeineCacheApiSpec.scala

- made ExpirableCacheValue.scala and DefaultCaffeineExpiry.scala package private

gurkankaymak · 2019-10-04T13:57:17Z

Thanks, @gurkankaymak.

I think it will also be useful to add some details about this change in our migration guide. For example, when using EhCache, the orElse part is now eargerly computed even when the value is present. This may be surprising to users, and since we plan to move EhCache support out of Play, I would say that it would be better to not bother changing it (the migration section can then focus exclusively on Caffeine implementation which is the default one).

WDYT?

other than eagerly evaluating the orElse part in EhCache there aren't much things that affect api users, I can think of the following things that can be mentioned in the migration guide, please let me know if any additions/suggestions to these

eagerly evaluating the orElse in EhCache
depricating CaffeineDefaultExpiry
making getOrElseUpdate methods atomic

updated Cache APIs to make getOrElseUpdate methods atomic

cache/play-caffeine-cache/src/main/scala/play/api/cache/caffeine/CaffeineCacheApi.scala

ben-manes · 2019-10-08T15:06:41Z

I would suggest leaving the ehcache version non-atomic as always evaluating defeats the purpose of a cache. Instead it seems like an incentive to migrate. There are probably other implementations, like redis/memcached, which would not be atomic either.

marcospereira · 2019-10-08T17:57:19Z

other than eagerly evaluating the orElse part in EhCache there aren't much things that affect api users, I can think of the following things that can be mentioned in the migration guide, please let me know if any additions/suggestions to these

eagerly evaluating the orElse in EhCache

deprecating CaffeineDefaultExpiry

making getOrElseUpdate methods atomic

It looks like to me that it would be better not to touch the EhCache implementation, but instead only the Caffeine one and make it clear at the migration guide that EhCache's getOrElseUpdate is still not atomic. So, the changes that make sense to me are:

Undo EhCache changes
deprecating CaffeineDefaultExpiry
making getOrElseUpdate methods atomic

And document these on the migration guide. Does that make sense to you?

Best.

gurkankaymak · 2019-10-09T07:10:33Z

I would suggest leaving the ehcache version non-atomic as always evaluating defeats the purpose of a cache. Instead it seems like an incentive to migrate. There are probably other implementations, like redis/memcached, which would not be atomic either.

I see your point, orElse part could do db/api call or some other expensive calculation, this would defeat the purpose of the cache

marcospereira

Some minor comments, but I think we are almost there. Thanks for your patience, @gurkankaymak. Things to do before merging this:

Squashing commits to organize them around the final changes better
Add tests to ensure orElse is NOT computed when the cache (sync and async) has the value

Besides that, it is looking good to me.

cache/play-caffeine-cache/src/main/java/play/cache/caffeine/CaffeineDefaultExpiry.java

cache/play-caffeine-cache/src/main/scala/play/api/cache/caffeine/CaffeineCacheApi.scala

updated CaffeineCacheApi to make getOrElseUpdate methods atomic

ben-manes

lgtm, for what that's worth!

marcospereira

LGTM. Thank you, @gurkankaymak!

mkurz · 2022-10-26T13:10:07Z

cache/play-caffeine-cache/src/main/scala/play/api/cache/caffeine/DefaultCaffeineExpiry.scala

+
+  private def calculateExpirationTime(durationMaybe: Option[Duration]): Long = {
+    durationMaybe match {
+      case Some(duration) if duration.isFinite && duration.lteq(0.second) => 1.second.toNanos


@gurkankaymak it's been a while, but do you remember why you gave the element 1 second to live? My guess was you did that because back then the ehcache implementation set the time to life for an element to 1 second as well if expiration was actually set to 0 seconds.
See this comment: https://github.com/playframework/playframework/pull/5923/files#r1005645691

Or was there another reason for doing that? Because I will set that to 0 in #11516 as I can see no sense in it.

@mkurz I don't think there was a specific reason for it, I agree with your guess, probably I was preserving the behaviour at the time while making the methods atomic

gurkankaymak closed this Sep 2, 2019

gurkankaymak reopened this Sep 2, 2019

gurkankaymak changed the title ~~fixes #8605~~ Updates cache api to make getOrElseUpdate methods atomic Sep 3, 2019

ben-manes reviewed Sep 3, 2019

View reviewed changes

marcospereira reviewed Sep 4, 2019

View reviewed changes

gurkankaymak added a commit to gurkankaymak/playframework that referenced this pull request Sep 10, 2019

implemented NamedCaffeineCache in terms of AsyncCache instead of Cach…

c74c084

…e interface as discussed playframework#9615

gurkankaymak mentioned this pull request Sep 10, 2019

implemented NamedCaffeineCache in terms of AsyncCache #9623

Merged

gurkankaymak added a commit to gurkankaymak/playframework that referenced this pull request Sep 14, 2019

implemented NamedCaffeineCache in terms of AsyncCache instead of Cach…

4a4bde1

…e interface as discussed playframework#9615

gurkankaymak added a commit to gurkankaymak/playframework that referenced this pull request Oct 1, 2019

implemented NamedCaffeineCache in terms of AsyncCache instead of Cach…

fe5da36

…e interface as discussed playframework#9615

gurkankaymak force-pushed the 8605-AtomicCacheApi branch from a70b2ba to d3efbdc Compare October 1, 2019 13:40

gurkankaymak added 7 commits October 1, 2019 16:44

fixes playframework#8605

f91d79b

updated cache apis to make getOrElseUpdate methods atomic

renamed class

c975f0a

removed class causes binary compatibility issue, re-created CaffeineD…

53dbdf2

…efaultExpiry.java for this reason "class play.cache.caffeine.CaffeineDefaultExpiry does not have a correspondent in current version"

scalafmt changes

18a09c8

rebased onto master and fixed conflicting parts to continue the pr on…

59e0929

… top of playframework#9623

excluded the deleted java class "play.cache.caffeine.CaffeineDefaultE…

4eca8ca

…xpiry" from binary compatibility

scalafmt changes

4b7e42e

gurkankaymak force-pushed the 8605-AtomicCacheApi branch from d3efbdc to 4b7e42e Compare October 1, 2019 13:44

extracted setting expiration logic to a method to be able to use in t…

d831512

…he getOrElseUpdate method

gurkankaymak requested a review from marcospereira October 3, 2019 06:50

marcospereira reviewed Oct 4, 2019

View reviewed changes

gurkankaymak added 2 commits October 4, 2019 09:08

removed duplicated header

d1f5518

- made CaffeineDefaultExpiry.java depricated instead of removing it

62daad1

- made ExpirableCacheValue.scala and DefaultCaffeineExpiry.scala package private

gurkankaymak added 2 commits October 8, 2019 15:20

added explanations to the migration guide

b9d547e

fixes playframework#8605

0bab60a

updated Cache APIs to make getOrElseUpdate methods atomic

ben-manes reviewed Oct 8, 2019

View reviewed changes

cache/play-caffeine-cache/src/main/scala/play/api/cache/caffeine/CaffeineCacheApi.scala Outdated Show resolved Hide resolved

gurkankaymak added 4 commits October 9, 2019 15:35

reverted EhCacheApi changes back to non-atomic state

c97a997

removed the part of the comment that became misleading

7d6d0f6

updated migration guide

53d99ba

removed unused import

8533554

marcospereira suggested changes Oct 11, 2019

View reviewed changes

cache/play-caffeine-cache/src/main/java/play/cache/caffeine/CaffeineDefaultExpiry.java Show resolved Hide resolved

cache/play-caffeine-cache/src/main/scala/play/api/cache/caffeine/CaffeineCacheApi.scala Outdated Show resolved Hide resolved

gurkankaymak added 4 commits October 14, 2019 14:03

added tests to ensure lazy evaluation of orElse

9ba02f9

fixes playframework#8605

dc3915d

updated CaffeineCacheApi to make getOrElseUpdate methods atomic

import style change from scalafmt

3039d5d

scalafmt change in a test file

03b9ec5

gurkankaymak requested a review from marcospereira October 14, 2019 17:06

ben-manes approved these changes Oct 15, 2019

View reviewed changes

marcospereira approved these changes Oct 15, 2019

View reviewed changes

mergify bot merged commit e7fb86f into playframework:master Oct 15, 2019

dwijnand added this to the Play 2.8.0-M6 milestone Oct 15, 2019

mkurz mentioned this pull request Oct 26, 2022

Don't cache elements for 1 second, makes no sense #11516

Draft

mkurz reviewed Oct 26, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates cache api to make getOrElseUpdate methods atomic #9615

Updates cache api to make getOrElseUpdate methods atomic #9615

gurkankaymak commented Sep 2, 2019

ben-manes Sep 3, 2019

gurkankaymak Sep 3, 2019

ben-manes Sep 3, 2019

gurkankaymak Sep 3, 2019

marcospereira Sep 3, 2019

ben-manes Sep 4, 2019

marcospereira Sep 4, 2019

gurkankaymak Sep 6, 2019

gurkankaymak Sep 10, 2019

marcospereira Sep 3, 2019

ben-manes Sep 4, 2019

gurkankaymak Sep 6, 2019

marcospereira Sep 3, 2019

marcospereira Sep 3, 2019

mkurz commented Oct 2, 2019

gurkankaymak commented Oct 3, 2019

marcospereira left a comment

gurkankaymak commented Oct 4, 2019

ben-manes commented Oct 8, 2019

marcospereira commented Oct 8, 2019 •

edited

gurkankaymak commented Oct 9, 2019

marcospereira left a comment

ben-manes left a comment

marcospereira left a comment

mkurz Oct 26, 2022

gurkankaymak Oct 26, 2022

Updates cache api to make getOrElseUpdate methods atomic #9615

Updates cache api to make getOrElseUpdate methods atomic #9615

Conversation

gurkankaymak commented Sep 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkurz commented Oct 2, 2019

gurkankaymak commented Oct 3, 2019

marcospereira left a comment

Choose a reason for hiding this comment

gurkankaymak commented Oct 4, 2019

ben-manes commented Oct 8, 2019

marcospereira commented Oct 8, 2019 • edited

gurkankaymak commented Oct 9, 2019

marcospereira left a comment

Choose a reason for hiding this comment

ben-manes left a comment

Choose a reason for hiding this comment

marcospereira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcospereira commented Oct 8, 2019 •

edited