Merge pull request #4116 from armanbilge/fix/mapref-access-semantics

Relax `access` semantics in `MapRef` implementations
typelevel · Aug 20, 2024 · f6690f8 · f6690f8
2 parents 447e140 + 895d210
commit f6690f8
Show file tree

Hide file tree

Showing 3 changed files with 10 additions and 51 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
@@ -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,28 +137,24 @@ 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)
           } else {
             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,31 +300,25 @@ 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)
             case Some(old) =>
               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)

diff --git 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
@@ -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]()