From 280706a451650b6e32d74b4dd2bb67d6cf8b61a2 Mon Sep 17 00:00:00 2001 From: Arman Bilge Date: Tue, 13 Aug 2024 23:03:02 +0000 Subject: [PATCH 1/2] Relax `access` semantics in `MapRef` implementations Following https://github.com/typelevel/cats-effect/pull/2901 --- .../main/scala/cats/effect/std/MapRef.scala | 31 ++++++------------- 1 file changed, 10 insertions(+), 21 deletions(-) diff --git a/std/shared/src/main/scala/cats/effect/std/MapRef.scala b/std/shared/src/main/scala/cats/effect/std/MapRef.scala index 425f43943c..a071951e90 100644 --- a/std/shared/src/main/scala/cats/effect/std/MapRef.scala +++ b/std/shared/src/main/scala/cats/effect/std/MapRef.scala @@ -23,7 +23,6 @@ import cats.effect.kernel._ import cats.syntax.all._ import java.util.concurrent.ConcurrentHashMap -import java.util.concurrent.atomic.AtomicBoolean /** * This is a total map from `K` to `Ref[F, V]`. @@ -138,18 +137,14 @@ object MapRef extends MapRefCompanionPlatform { def access: F[(Option[V], Option[V] => F[Boolean])] = delay { - val hasBeenCalled = new AtomicBoolean(false) val init = chm.get(k) if (init == null) { val set: Option[V] => F[Boolean] = { (opt: Option[V]) => opt match { case None => - delay(hasBeenCalled.compareAndSet(false, true) && !chm.containsKey(k)) - case Some(newV) => - delay { - // it was initially empty - hasBeenCalled.compareAndSet(false, true) && chm.putIfAbsent(k, newV) == null - } + delay(!chm.containsKey(k)) + case Some(newV) => // it was initially empty + delay(chm.putIfAbsent(k, newV) == null) } } (None, set) @@ -157,9 +152,9 @@ object MapRef extends MapRefCompanionPlatform { val set: Option[V] => F[Boolean] = { (opt: Option[V]) => opt match { case None => - delay(hasBeenCalled.compareAndSet(false, true) && chm.remove(k, init)) + delay(chm.remove(k, init)) case Some(newV) => - delay(hasBeenCalled.compareAndSet(false, true) && chm.replace(k, init, newV)) + delay(chm.replace(k, init, newV)) } } (Some(init), set) @@ -305,20 +300,15 @@ object MapRef extends MapRefCompanionPlatform { class HandleRef(k: K) extends Ref[F, Option[V]] { def access: F[(Option[V], Option[V] => F[Boolean])] = sync.delay { - val hasBeenCalled = new AtomicBoolean(false) val init = map.get(k) init match { case None => val set: Option[V] => F[Boolean] = { (opt: Option[V]) => opt match { case None => - sync.delay(hasBeenCalled.compareAndSet(false, true) && !map.contains(k)) - case Some(newV) => - sync.delay { - // it was initially empty - hasBeenCalled - .compareAndSet(false, true) && map.putIfAbsent(k, newV).isEmpty - } + sync.delay(!map.contains(k)) + case Some(newV) => // it was initially empty + sync.delay(map.putIfAbsent(k, newV).isEmpty) } } (None, set) @@ -326,10 +316,9 @@ object MapRef extends MapRefCompanionPlatform { val set: Option[V] => F[Boolean] = { (opt: Option[V]) => opt match { case None => - sync.delay(hasBeenCalled.compareAndSet(false, true) && map.remove(k, old)) + sync.delay(map.remove(k, old)) case Some(newV) => - sync.delay( - hasBeenCalled.compareAndSet(false, true) && map.replace(k, old, newV)) + sync.delay(map.replace(k, old, newV)) } } (init, set) From 895d210424b41579761e066fb46444bdace49c15 Mon Sep 17 00:00:00 2001 From: Arman Bilge Date: Wed, 14 Aug 2024 18:00:14 +0000 Subject: [PATCH 2/2] Remove irrelevant tests --- .../scala/cats/effect/std/MapRefJVMSpec.scala | 15 --------------- .../test/scala/cats/effect/std/MapRefSpec.scala | 15 --------------- 2 files changed, 30 deletions(-) diff --git a/tests/jvm/src/test/scala/cats/effect/std/MapRefJVMSpec.scala b/tests/jvm/src/test/scala/cats/effect/std/MapRefJVMSpec.scala index 45d4e87eae..c6ebecbd9a 100644 --- a/tests/jvm/src/test/scala/cats/effect/std/MapRefJVMSpec.scala +++ b/tests/jvm/src/test/scala/cats/effect/std/MapRefJVMSpec.scala @@ -104,21 +104,6 @@ class MapRefJVMSpec extends BaseSpec { op.map(a => a must_=== true) } - "access - setter should fail if called twice" in real { - val op = for { - r <- MapRef.ofScalaConcurrentTrieMap[IO, Unit, Int] - _ <- r(()).set(Some(0)) - accessed <- r(()).access - (value, setter) = accessed - cond1 <- setter(value.map(_ + 1)) - _ <- r(()).set(value) - cond2 <- setter(None) - result <- r(()).get - } yield cond1 && !cond2 && result == Some(0) - - op.map(a => a must_=== true) - } - "tryUpdate - modification occurs successfully" in real { val op = for { r <- MapRef.ofScalaConcurrentTrieMap[IO, Unit, Int] diff --git a/tests/shared/src/test/scala/cats/effect/std/MapRefSpec.scala b/tests/shared/src/test/scala/cats/effect/std/MapRefSpec.scala index 2ee9166631..b4cc3b2162 100644 --- a/tests/shared/src/test/scala/cats/effect/std/MapRefSpec.scala +++ b/tests/shared/src/test/scala/cats/effect/std/MapRefSpec.scala @@ -237,21 +237,6 @@ class MapRefSpec extends BaseSpec { op.map(a => a must_=== true) } - "access - setter should fail if called twice" in real { - val op = for { - r <- MapRef.ofConcurrentHashMap[IO, Unit, Int]() - _ <- r(()).set(Some(0)) - accessed <- r(()).access - (value, setter) = accessed - cond1 <- setter(value.map(_ + 1)) - _ <- r(()).set(value) - cond2 <- setter(None) - result <- r(()).get - } yield cond1 && !cond2 && result == Some(0) - - op.map(a => a must_=== true) - } - "tryUpdate - modification occurs successfully" in real { val op = for { r <- MapRef.ofConcurrentHashMap[IO, Unit, Int]()