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

[mkurz]

While looking at #11506 and working on #11515 I was wondering why we put elements in the cache for 1 seconds if the expiration is set to 0 (which btw for us in Play means do not cache it - some libraries may treat that as indefinitely, just a note)

This was started in #5923 where ehcache, which was our first cache implementation long before caffeine, set the time to live to 1 second.

This behaviour is now causing problems for #11515. I can not see the sense of this behaviour.

[mkurz]

First, Play's ehcache implementation introduced that behaviour first, see https://github.com/playframework/playframework/pull/5923/files#r1005645691

[gmethvin]

As I mentioned in my reply on that PR, we should consider cases where the duration is a positive value less than 1 second. If the duration is actually zero or less I agree it's reasonable behavior not to cache at all.

[mkurz]

The 1 second for caffeine was introduced in #9615 - there is no comment why this was done, so my guess would be the author just copied the ehcache behaviour... Update: comment was added.

[mkurz]

See ben-manes/caffeine#803

[mkurz]

So in ben-manes/caffeine#803 it turns out I was right and returning 0 will therefore always end up in a cache miss for the next read from the cache from the same key, so it's like not caching at all.

[mkurz]

This if condition was done just to be on par with the ehcache implementation. Actually belows change in calculateExpirationTime from 1 to 0 should already be enough.

[felipebonezi]

@mkurz I didn't understand.
For me, if some uses the set(Key: String, value: Any, expiration: Duration) method it won't be saved into syncCache. So, it seems that this method wouldn't work...

IMHO doesn't matter if your pass 0+ seconds, must work.

[mkurz]

It does work for 1 and more seconds, but an element will not be cached if 0 seconds or lower.

The condition means

if (expiration is infinite/forever OR expiration is greater than 0 seconds)
  then cache it
end if

[mkurz]

It's just a negation of the if below case Some(duration) if duration.isFinite && duration.lteq(0.second) =>...

[felipebonezi]

So, what I've said seems to be true.
If someone uses 0 seconds it wouldn't be saved, right?

Why wouldn't we set it into the cache as a value without expiration when is defined as 0 secs?

[mkurz]

If someone uses 0 seconds it wouldn't be saved, right?

Yes, that is the current behavour in Play 2.8. as well. OK, not really, currently, if you pass 0 that means cache it just for 1 second.... this is what I am changing right now because it does not make sense in my opinion...

Maybe this code explains the current behavour best:

playframework/cache/play-ehcache/src/main/scala/play/api/cache/ehcache/EhCacheApi.scala

Lines 190 to 204 in ccf99ba

	val element = new Element(key, value)
	expiration match {
	case infinite: Duration.Infinite => element.setEternal(true)
	case finite: FiniteDuration =>
	val seconds = finite.toSeconds
	if (seconds <= 0) {
	element.setTimeToLive(1)
	} else if (seconds > Int.MaxValue) {
	element.setTimeToLive(Int.MaxValue)
	} else {
	element.setTimeToLive(seconds.toInt)
	}
	}
	cache.put(element)
	Done

Why wouldn't we set it into the cache as a value without expiration when is defined as 0 secs?

Because someone, long long time ago, decided that when passing 0 as expiration it should mean "do not cache" (or like: cache for 1 second) in Play. Be aware, that different libraries use different approaches. So for ehcache if you pass 0 that means cache it forever, however, in caffeine if you return 0 from expireAfterCreate it means do not cache it at all (see this coment "...How well could expireAfter(expiry) fit your use-case? That returns a duration, which may be zero to indicate immediate expiration...")
So different libraries use different approaches. And Play choose to say 0 expiration = no caching. I didn't decided that.
So my changes here do not change anything actually, I just do not cache it for 1 second but do not cache it at all anymore because it's senseless.

[mkurz]

@felipebonezi Please let me know if you don't understand what I am saying. In my opinion my code does not really change the behaviour of the cache.

[felipebonezi]

Perfect, I do understand now.
Thanks for being patient with me, hehehehe. 🤣

[gmethvin]

If there is an existing value in the cache, I think you want to remove the element from the cache rather than just doing nothing.

[mkurz]

Yeah that is something to consider, thanks!