From 1184a175f662a137be6e521285d51d1e14176201 Mon Sep 17 00:00:00 2001 From: Jongbeom Kim Date: Tue, 26 Sep 2023 15:40:26 +0900 Subject: [PATCH 1/2] Add a parameter `storeOnlyIfValue` in Cache.make.* --- .../src/main/scala/zio/cache/Cache.scala | 61 +++++++++--- .../src/test/scala/zio/cache/CacheSpec.scala | 93 ++++++++++++++++++- 2 files changed, 141 insertions(+), 13 deletions(-) diff --git a/zio-cache/shared/src/main/scala/zio/cache/Cache.scala b/zio-cache/shared/src/main/scala/zio/cache/Cache.scala index da44d61..9241421 100644 --- a/zio-cache/shared/src/main/scala/zio/cache/Cache.scala +++ b/zio-cache/shared/src/main/scala/zio/cache/Cache.scala @@ -103,9 +103,10 @@ object Cache { def make[Key, Environment, Error, Value]( capacity: Int, timeToLive: Duration, - lookup: Lookup[Key, Environment, Error, Value] + lookup: Lookup[Key, Environment, Error, Value], + storeOnlyIfValue: Boolean = false )(implicit trace: Trace): URIO[Environment, Cache[Key, Error, Value]] = - makeWith(capacity, lookup)(_ => timeToLive) + makeWith(capacity, lookup, storeOnlyIfValue)(_ => timeToLive) /** * Constructs a new cache with the specified capacity, time to live, and @@ -114,11 +115,12 @@ object Cache { */ def makeWith[Key, Environment, Error, Value]( capacity: Int, - lookup: Lookup[Key, Environment, Error, Value] + lookup: Lookup[Key, Environment, Error, Value], + storeOnlyIfValue: Boolean = false )( timeToLive: Exit[Error, Value] => Duration )(implicit trace: Trace): URIO[Environment, Cache[Key, Error, Value]] = - makeWithKey(capacity, lookup)(timeToLive, identity) + makeWithKey(capacity, lookup, storeOnlyIfValue)(timeToLive, identity) /** * Constructs a new cache with the specified capacity, time to live, and @@ -132,7 +134,8 @@ object Cache { */ def makeWithKey[In, Key, Environment, Error, Value]( capacity: Int, - lookup: Lookup[In, Environment, Error, Value] + lookup: Lookup[In, Environment, Error, Value], + storeOnlyIfValue: Boolean = false )( timeToLive: Exit[Error, Value] => Duration, keyBy: In => Key @@ -230,7 +233,12 @@ object Cache { map.remove(k, value) get(in) } else { - ZIO.done(exit) + exit match { + case Left(exit) => + ZIO.done(exit) + case Right(value) => + ZIO.done(Exit.Success(value)) + } } case MapValue.Refreshing( promiseInProgress, @@ -241,7 +249,12 @@ object Cache { if (hasExpired(ttl)) { promiseInProgress.await } else { - ZIO.done(currentResult) + currentResult match { + case Left(exit) => + ZIO.done(exit) + case Right(value) => + ZIO.done(Exit.Success(value)) + } } } } @@ -266,8 +279,9 @@ object Cache { map.remove(k, value) get(in) } else { + val rollbackResultIfError = if (storeOnlyIfValue) Some(completedResult) else None // Only trigger the lookup if we're still the current value, `completedResult` - lookupValueOf(in, promise).when { + lookupValueOf(in, promise, rollbackResultIfError).when { map.replace(k, completedResult, MapValue.Refreshing(promise, completedResult)) } } @@ -292,7 +306,11 @@ object Cache { def size(implicit trace: Trace): UIO[Int] = ZIO.succeed(map.size) - private def lookupValueOf(in: In, promise: Promise[Error, Value]): IO[Error, Value] = + private def lookupValueOf( + in: In, + promise: Promise[Error, Value], + rollbackResultIfError: Option[MapValue.Complete[Key, Error, Value]] = None + ): IO[Error, Value] = ZIO.suspendSucceed { val key = keyBy(in) lookup(in) @@ -302,7 +320,28 @@ object Cache { val now = Unsafe.unsafe(implicit u => clock.unsafe.instant()) val entryStats = EntryStats(now) - map.put(key, MapValue.Complete(new MapKey(key), exit, entryStats, now.plus(timeToLive(exit)))) + if (!storeOnlyIfValue) + map.put( + key, + MapValue.Complete(new MapKey(key), Left(exit), entryStats, now.plus(timeToLive(exit))) + ) + else { + exit match { + case Exit.Success(value) => + map.put( + key, + MapValue.Complete(new MapKey(key), Right(value), entryStats, now.plus(timeToLive(exit))) + ) + case Exit.Failure(cause) => + rollbackResultIfError match { + case None => + map.remove(key) + case Some(rollbackValue) => + map.put(key, rollbackValue) + } + } + } + promise.done(exit) *> ZIO.done(exit) } .onInterrupt(promise.interrupt *> ZIO.succeed(map.remove(key))) @@ -334,7 +373,7 @@ object Cache { final case class Complete[Key, Error, Value]( key: MapKey[Key], - exit: Exit[Error, Value], + exit: Either[Exit[Error, Value], Value], entryStats: EntryStats, timeToLive: Instant ) extends MapValue[Key, Error, Value] diff --git a/zio-cache/shared/src/test/scala/zio/cache/CacheSpec.scala b/zio-cache/shared/src/test/scala/zio/cache/CacheSpec.scala index 3d2cf46..5a3bdc2 100644 --- a/zio-cache/shared/src/test/scala/zio/cache/CacheSpec.scala +++ b/zio-cache/shared/src/test/scala/zio/cache/CacheSpec.scala @@ -114,13 +114,17 @@ object CacheSpec extends ZIOSpecDefault { _ <- cache.refresh(key) val1 <- cache.get(key).either _ <- cache.refresh(key) + val2 <- cache.get(key).either failure2 <- cache.refresh(key).either + failure3 <- cache.get(key).either _ <- cache.refresh(key) - val2 <- cache.get(key).either + val3 <- cache.get(key).either } yield assert(failure1)(isLeft(equalTo(error))) && assert(failure2)(isLeft(equalTo(error))) && + assert(failure3)(isLeft(equalTo(error))) && assert(val1)(isRight(equalTo(4))) && - assert(val2)(isRight(equalTo(7))) + assert(val2)(isRight(equalTo(5))) && + assert(val3)(isRight(equalTo(7))) }, test("should get the value if the key doesn't exist in the cache") { check(Gen.int) { salt => @@ -134,6 +138,91 @@ object CacheSpec extends ZIOSpecDefault { } } ), + suite("`refresh` method when storeOnlyIfValue is true")( + test("should update the cache with a new value") { + def inc(n: Int) = n * 10 + + def retrieve(multiplier: Ref[Int])(key: Int) = + multiplier + .updateAndGet(inc) + .map(key * _) + + val seed = 1 + val key = 123 + for { + ref <- Ref.make(seed) + cache <- Cache.make(1, Duration.Infinity, Lookup(retrieve(ref)), storeOnlyIfValue = true) + val1 <- cache.get(key) + _ <- cache.refresh(key) + _ <- cache.get(key) + val2 <- cache.get(key) + } yield assertTrue(val1 == inc(key)) && assertTrue(val2 == inc(val1)) + }, + test("should update the cache only if lookup return a value, not an error.") { + + val error = new RuntimeException("Must be a multiple of 3") + + def inc(n: Int) = n + 1 + + def retrieve(number: Ref[Int])(key: Int) = + number + .updateAndGet(inc) + .flatMap { + case n if n % 3 == 0 => + ZIO.fail(error) + case n => + ZIO.succeed(key * n) + } + + val seed = 2 + val key = 1 + for { + ref <- Ref.make(seed) + cache <- Cache.make(1, Duration.Infinity, Lookup(retrieve(ref)), storeOnlyIfValue = true) + failure1 <- cache.get(key).either + _ <- cache.refresh(key) + val1 <- cache.get(key).either + _ <- cache.refresh(key) + val2 <- cache.get(key).either + failure2 <- cache.refresh(key).either + val3 <- cache.get(key).either + _ <- cache.refresh(key) + val4 <- cache.get(key).either + } yield assert(failure1)(isLeft(equalTo(error))) && + assert(failure2)(isLeft(equalTo(error))) && + assert(val1)(isRight(equalTo(4))) && + assert(val2)(isRight(equalTo(5))) && + assert(val3)(isRight(equalTo(5))) && + assert(val4)(isRight(equalTo(7))) + }, + test("should update only if it is a value when the key doesn't exist in the cache") { + + val error = new RuntimeException("Must be a multiple of 3") + + def inc(n: Int) = n + 1 + + def retrieve(number: Ref[Int])(key: Int) = + number + .updateAndGet(inc) + .flatMap { + case n if n % 3 == 0 => + ZIO.fail(error) + case n => + ZIO.succeed(key * n) + } + + val seed = 2 + val key = 1 + val cap = 30 + for { + ref <- Ref.make(seed) + cache <- Cache.make(cap, Duration.Infinity, Lookup(retrieve(ref)), storeOnlyIfValue = true) + count0 <- cache.size + _ <- ZIO.foreachDiscard(1 to cap)(key => cache.refresh(key).either) + count1 <- cache.size + } yield assertTrue(count0 == 0) && assertTrue(count1 == cap / 3 * 2) + } + ), test("size") { check(Gen.int) { salt => for { From 873e990633b8e32ec426d0d4b356bfe8757f6bd2 Mon Sep 17 00:00:00 2001 From: Jongbeom Kim Date: Tue, 26 Sep 2023 15:53:18 +0900 Subject: [PATCH 2/2] Rename to `cacheValuesOnly` --- .../src/main/scala/zio/cache/Cache.scala | 35 +++++++------------ .../src/test/scala/zio/cache/CacheSpec.scala | 6 ++-- 2 files changed, 15 insertions(+), 26 deletions(-) diff --git a/zio-cache/shared/src/main/scala/zio/cache/Cache.scala b/zio-cache/shared/src/main/scala/zio/cache/Cache.scala index 9241421..ebab7fe 100644 --- a/zio-cache/shared/src/main/scala/zio/cache/Cache.scala +++ b/zio-cache/shared/src/main/scala/zio/cache/Cache.scala @@ -104,9 +104,9 @@ object Cache { capacity: Int, timeToLive: Duration, lookup: Lookup[Key, Environment, Error, Value], - storeOnlyIfValue: Boolean = false + cacheValuesOnly: Boolean = false )(implicit trace: Trace): URIO[Environment, Cache[Key, Error, Value]] = - makeWith(capacity, lookup, storeOnlyIfValue)(_ => timeToLive) + makeWith(capacity, lookup, cacheValuesOnly)(_ => timeToLive) /** * Constructs a new cache with the specified capacity, time to live, and @@ -116,11 +116,11 @@ object Cache { def makeWith[Key, Environment, Error, Value]( capacity: Int, lookup: Lookup[Key, Environment, Error, Value], - storeOnlyIfValue: Boolean = false + cacheValuesOnly: Boolean = false )( timeToLive: Exit[Error, Value] => Duration )(implicit trace: Trace): URIO[Environment, Cache[Key, Error, Value]] = - makeWithKey(capacity, lookup, storeOnlyIfValue)(timeToLive, identity) + makeWithKey(capacity, lookup, cacheValuesOnly)(timeToLive, identity) /** * Constructs a new cache with the specified capacity, time to live, and @@ -135,7 +135,7 @@ object Cache { def makeWithKey[In, Key, Environment, Error, Value]( capacity: Int, lookup: Lookup[In, Environment, Error, Value], - storeOnlyIfValue: Boolean = false + cacheValuesOnly: Boolean = false )( timeToLive: Exit[Error, Value] => Duration, keyBy: In => Key @@ -233,12 +233,7 @@ object Cache { map.remove(k, value) get(in) } else { - exit match { - case Left(exit) => - ZIO.done(exit) - case Right(value) => - ZIO.done(Exit.Success(value)) - } + ZIO.done(exit) } case MapValue.Refreshing( promiseInProgress, @@ -249,12 +244,7 @@ object Cache { if (hasExpired(ttl)) { promiseInProgress.await } else { - currentResult match { - case Left(exit) => - ZIO.done(exit) - case Right(value) => - ZIO.done(Exit.Success(value)) - } + ZIO.done(currentResult) } } } @@ -279,9 +269,8 @@ object Cache { map.remove(k, value) get(in) } else { - val rollbackResultIfError = if (storeOnlyIfValue) Some(completedResult) else None // Only trigger the lookup if we're still the current value, `completedResult` - lookupValueOf(in, promise, rollbackResultIfError).when { + lookupValueOf(in, promise, Some(completedResult)).when { map.replace(k, completedResult, MapValue.Refreshing(promise, completedResult)) } } @@ -320,17 +309,17 @@ object Cache { val now = Unsafe.unsafe(implicit u => clock.unsafe.instant()) val entryStats = EntryStats(now) - if (!storeOnlyIfValue) + if (!cacheValuesOnly) map.put( key, - MapValue.Complete(new MapKey(key), Left(exit), entryStats, now.plus(timeToLive(exit))) + MapValue.Complete(new MapKey(key), exit, entryStats, now.plus(timeToLive(exit))) ) else { exit match { case Exit.Success(value) => map.put( key, - MapValue.Complete(new MapKey(key), Right(value), entryStats, now.plus(timeToLive(exit))) + MapValue.Complete(new MapKey(key), exit, entryStats, now.plus(timeToLive(exit))) ) case Exit.Failure(cause) => rollbackResultIfError match { @@ -373,7 +362,7 @@ object Cache { final case class Complete[Key, Error, Value]( key: MapKey[Key], - exit: Either[Exit[Error, Value], Value], + exit: Exit[Error, Value], entryStats: EntryStats, timeToLive: Instant ) extends MapValue[Key, Error, Value] diff --git a/zio-cache/shared/src/test/scala/zio/cache/CacheSpec.scala b/zio-cache/shared/src/test/scala/zio/cache/CacheSpec.scala index 5a3bdc2..2581ac8 100644 --- a/zio-cache/shared/src/test/scala/zio/cache/CacheSpec.scala +++ b/zio-cache/shared/src/test/scala/zio/cache/CacheSpec.scala @@ -151,7 +151,7 @@ object CacheSpec extends ZIOSpecDefault { val key = 123 for { ref <- Ref.make(seed) - cache <- Cache.make(1, Duration.Infinity, Lookup(retrieve(ref)), storeOnlyIfValue = true) + cache <- Cache.make(1, Duration.Infinity, Lookup(retrieve(ref)), cacheValuesOnly = true) val1 <- cache.get(key) _ <- cache.refresh(key) _ <- cache.get(key) @@ -178,7 +178,7 @@ object CacheSpec extends ZIOSpecDefault { val key = 1 for { ref <- Ref.make(seed) - cache <- Cache.make(1, Duration.Infinity, Lookup(retrieve(ref)), storeOnlyIfValue = true) + cache <- Cache.make(1, Duration.Infinity, Lookup(retrieve(ref)), cacheValuesOnly = true) failure1 <- cache.get(key).either _ <- cache.refresh(key) val1 <- cache.get(key).either @@ -216,7 +216,7 @@ object CacheSpec extends ZIOSpecDefault { val cap = 30 for { ref <- Ref.make(seed) - cache <- Cache.make(cap, Duration.Infinity, Lookup(retrieve(ref)), storeOnlyIfValue = true) + cache <- Cache.make(cap, Duration.Infinity, Lookup(retrieve(ref)), cacheValuesOnly = true) count0 <- cache.size _ <- ZIO.foreachDiscard(1 to cap)(key => cache.refresh(key).either) count1 <- cache.size