Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve concurrent behavior of Java ConcurrentMap wrapper #10027

Merged
merged 2 commits into from Jun 2, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions project/MimaFilters.scala
Expand Up @@ -36,6 +36,9 @@ object MimaFilters extends AutoPlugin {
ProblemFilters.exclude[MissingClassProblem]("scala.runtime.PStatics"), // private[scala]
ProblemFilters.exclude[MissingClassProblem]("scala.runtime.PStatics$"), // private[scala]

// internal to wrappers
ProblemFilters.exclude[NewMixinForwarderProblem]("scala.collection.convert.JavaCollectionWrappers#JMapWrapperLike.getOrElseUpdate"),
ProblemFilters.exclude[NewMixinForwarderProblem]("scala.collection.convert.JavaCollectionWrappers#JMapWrapperLike.updateWith"),
)

override val buildSettings = Seq(
Expand Down
11 changes: 11 additions & 0 deletions src/library/scala/collection/convert/JavaCollectionWrappers.scala
Expand Up @@ -332,6 +332,7 @@ private[collection] object JavaCollectionWrappers extends Serializable {
else
None
}
override def getOrElseUpdate(key: K, op: => V): V = underlying.computeIfAbsent(key, _ => op)

def addOne(kv: (K, V)): this.type = { underlying.put(kv._1, kv._2); this }
def subtractOne(key: K): this.type = { underlying remove key; this }
Expand All @@ -354,6 +355,10 @@ private[collection] object JavaCollectionWrappers extends Serializable {

override def update(k: K, v: V): Unit = underlying.put(k, v)

override def updateWith(key: K)(remappingFunction: Option[V] => Option[V]): Option[V] = Option {
underlying.compute(key, (_, v) => remappingFunction(Option(v)).getOrElse(null.asInstanceOf[V]))
}

// support Some(null) if currently bound to null
override def remove(k: K): Option[V] = {
var result: Option[V] = None
Expand Down Expand Up @@ -434,6 +439,8 @@ private[collection] object JavaCollectionWrappers extends Serializable {

override def get(k: K) = Option(underlying get k)

override def getOrElseUpdate(key: K, op: => V): V = underlying.computeIfAbsent(key, _ => op)
som-snytt marked this conversation as resolved.
Show resolved Hide resolved

override def isEmpty: Boolean = underlying.isEmpty
override def knownSize: Int = if (underlying.isEmpty) 0 else super.knownSize
override def empty = new JConcurrentMapWrapper(new juc.ConcurrentHashMap[K, V])
Expand All @@ -452,6 +459,10 @@ private[collection] object JavaCollectionWrappers extends Serializable {
case _ if isEmpty => None
case _ => Try(last).toOption
}

override def updateWith(key: K)(remappingFunction: Option[V] => Option[V]): Option[V] = Option {
underlying.compute(key, (_, v) => remappingFunction(Option(v)).getOrElse(null.asInstanceOf[V]))
}
}

@SerialVersionUID(3L)
Expand Down
32 changes: 30 additions & 2 deletions test/junit/scala/collection/convert/MapWrapperTest.scala
@@ -1,13 +1,14 @@
package scala.collection.convert

import java.util
import java.{util => jutil}

import org.junit.Assert._
import org.junit.Test
import org.junit.runner.RunWith
import org.junit.runners.JUnit4

import scala.jdk.CollectionConverters._
import scala.util.chaining._

@RunWith(classOf[JUnit4])
class MapWrapperTest {
Expand Down Expand Up @@ -63,7 +64,7 @@ class MapWrapperTest {
// regression test for https://github.com/scala/bug/issues/10663
@Test
def testHashCodeEqualsMatchesJavaMap(): Unit = {
val jmap = new util.HashMap[String, String]()
val jmap = new jutil.HashMap[String, String]()
jmap.put("scala", "rocks")
jmap.put("java interop is fun!", "ya!")
jmap.put("Ĺởồҝ ïŧ\\'ş ūŋǐčōđẹ", "whyyyy")
Expand All @@ -79,4 +80,31 @@ class MapWrapperTest {
assertTrue(jmap == mapWrapper)
assertTrue(mapWrapper == jmap)
}

// was: induce intermittent failure due to contention, where updater is called more than once
@Test def `t12586 updateWith should delegate to compute`: Unit = {
val limit = 100 // retries until trigger
@volatile var count = 0
val jmap = new jutil.concurrent.ConcurrentHashMap[String, String]()
class Loki extends Runnable {
@volatile var done = false
def run(): Unit = {
while (!done) {
jmap.put("KEY", "VALUE")
//Thread.`yield`()
}
}
}
val loki = new Loki
val runner = new Thread(loki).tap(_.start)
val wrapped = jmap.asScala
def updater(old: Option[String]) = { count += 1 ; old.map(_ * 2) }
for (i <- 1 to limit) {
count = 0
wrapped.updateWith("KEY")(updater)
assertEquals(s"index $i", 1, count)
}
loki.done = true
runner.join()
}
}