Improve concurrent behavior of Java ConcurrentMap wrapper

[som-snytt]

Improve the atomicity of updateWith by calling compute on the Java ConcurrentMap.

Similarly, getOrElseUpdate calls computeIfAbsent.

Note that null values are not supported.

The 2.13.8 version of concurrentMap.asScala.getOrElseUpdate calls underlying.putIfAbsent, which NPEs on null; the new version calls underlying.computeIfAbsent, which ignores a null computed value.

Code which relies on the new behavior will throw an exception when used with a previous version of the library.

Fixes scala/bug#12586

[danicheg]

It'd be nice if you add @igabaydulin to co-authors. I guess most of the work was done by them. Thank you!

[som-snytt]

I don't mind giving credit per https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors

I need to know what info to use.

"most of the work" would include writing the test and what to patch. But I appreciate the nice bug report.

[igabaydulin]

@som-snytt Igor Gabaydulin <igabaydulin@gmail.com>

[danicheg]

@som-snytt I wasn't about the counting efforts of fixing this bug in the statement 'most of the work', rather just asking for adding into co-authors, in a way you pointed it out, thanks 👍🏻

[som-snytt]

Thanks again. Oh hey, it worked, I see the tiny avatar.

[som-snytt]

I'll bring out my next album under the name JMapRapper.

The collection.Conversers may comment whether to retain the second commit, or add more.

The Wrappers don't come pre-wrapped for mima:

[error] scala-library: Failed binary compatibility check against org.scala-lang:scala-library:2.13.8! Found 2 potential problems (filtered 5)
[error]  * in other version, classes mixing scala.collection.convert.JavaCollectionWrappers$JMapWrapperLike need be recompiled to wire to the new static mixin forwarder method all super calls to method getOrElseUpdate(java.lang.Object,scala.Function0)java.lang.Object
[error]    filter with: ProblemFilters.exclude[NewMixinForwarderProblem]("scala.collection.convert.JavaCollectionWrappers#JMapWrapperLike.getOrElseUpdate")
[error]  * in other version, classes mixing scala.collection.convert.JavaCollectionWrappers$JMapWrapperLike need be recompiled to wire to the new static mixin forwarder method all super calls to method updateWith(java.lang.Object,scala.Function1)scala.Option
[error]    filter with: ProblemFilters.exclude[NewMixinForwarderProblem]("scala.collection.convert.JavaCollectionWrappers#JMapWrapperLike.updateWith")

[som-snytt]

The linearization is

class JConcurrentMapWrapper (scala.collection.convert.JavaCollectionWrappers.JConcurrentMapWrapper) has method getOrElseUpdate owned by class JConcurrentMapWrapper
trait Product (scala.Product) has <none> owned by <none>
trait Map (scala.collection.concurrent.Map) has method getOrElseUpdate owned by trait Map
class AbstractJMapWrapper (scala.collection.convert.JavaCollectionWrappers.AbstractJMapWrapper) has method getOrElseUpdate owned by trait JMapWrapperLike
trait Serializable (java.io.Serializable) has <none> owned by <none>
trait JMapWrapperLike (scala.collection.convert.JavaCollectionWrappers.JMapWrapperLike) has method getOrElseUpdate owned by trait JMapWrapperLike
trait StrictOptimizedMapOps (scala.collection.StrictOptimizedMapOps) has <none> owned by <none>
trait StrictOptimizedIterableOps (scala.collection.StrictOptimizedIterableOps) has <none> owned by <none>
class AbstractMap (scala.collection.mutable.AbstractMap) has method getOrElseUpdate owned by trait MapOps
trait Map (scala.collection.mutable.Map) has method getOrElseUpdate owned by trait MapOps
trait MapOps (scala.collection.mutable.MapOps) has method getOrElseUpdate owned by trait MapOps

where the order of concurrent.Map is the motivating issue.

JMapWrapperLike introduces delegation and also some null handling.

It's desirable for JConcurrentMapWrapper to revert overrides from concurrent.Map and also the null handling.

I think it's easier and clearer not to be too clever in minimizing duplication (by using super.)

My other observation is that I was under the mistaken impression that all users are trading performance for convenience. However, the code comment that WeakHashMap is a wrapper means that perRunCaches exercises the getOrElseUpdate that now delegates in JMapWrapperLike. That was in MacroRuntimes.

[som-snytt]

That is a spurious red ex.

[som-snytt]

Inspired by the recently linked tickets, I updated the PR description.

The workaround is to inline underlying.computeIfAbsent, but obtaining the underlying is a nuisance.

[ijuma]

It would be useful to describe in a bit more detail what is the expected behavior with this change.

java.util.ConcurrentHashMap assumes that no key is inserted when the function passed to computeIfAbsent returns null. In Scala, that would be the equivalent to passing a function that returns Option (and have it return None).

scala> new java.util.concurrent.ConcurrentHashMap[String, String]
val res2: java.util.concurrent.ConcurrentHashMap[String,String] = {}

scala> res2.computeIfAbsent("foo", _ => null)
val res9: String = null

scala> res2.get("foo")
val res10: String = null

scala> res2.containsKey("foo")
val res11: Boolean = false

On the other hand, the Scala getOrElseUpdate assumes null as the by-name parameter to be the expected value (which is not allowed by ConcurrentHashMap and the NPE seems to be desired).

scala> scala.collection.mutable.Map[String, String]()
val res18: scala.collection.mutable.Map[String,String] = HashMap()

scala> res18.getOrElseUpdate("foo", null)
val res19: String = null

scala> res18.get("foo")
val res20: Option[String] = Some(null)

scala> res18.contains("foo")
val res21: Boolean = true

I looked at the tests added via this PR and it didn't seem like it tested null usage to verify the behavior actually makes sense for Scala collections (unless I missed something).

[som-snytt]

I tweaked the description and title, as the change is not about null (see the ticket); however, the description notes a caveat about backward compatibility.

A previous commit aligned handling of null values for non-concurrent map wrapper, where it's possible to witness Some(null). That is tested in a separate test.

The test for this PR is just to demonstrate that concurrency is correctly supported.

[ijuma]

Thanks for the clarification @som-snytt. The following seems like an undesirable change though, no?

The 2.13.8 version of concurrentMap.asScala.getOrElseUpdate calls underlying.putIfAbsent, which NPEs on null; the new version calls underlying.computeIfAbsent, which ignores a null computed value.

I'd expect concurrentMap.asScala.getOrElseUpdate('foo", null) to throw a NPE since null values are not supported by the underlying map.

[som-snytt]

@ijuma I think it falls under the umbrella of "follow the behavior established by Java".

I see Java's put and replace throw, and Scala's put and update throw, but updateWith does not:

scala> m.asScala.updateWith("x")(_ => Some(null))
val res5: Option[String] = None

since Scala lacks compute methods, it's a compromise on naming.

At some cost, maybe it's possible to wrap the remapper, and then if it is invoked and returns null, call a method that throws? The wrapper just delegates and would not guarantee it would throw, as the Java side could be a concurrent map that does allow null.

[som-snytt]

Linked PR bends over backward to support null value policy. The Scala API must be revisited in future so that people can have nice stuff.

[SethTisue]

(Does anyone want to argue that that PR can't/shouldn't wait until 2.13.10? Since we've already got a 2.13.9 release candidate.)

[SethTisue]

On second thought: if we do anything further for 2.13.9, perhaps it should be to simply revert this PR, to give us more time to decide what the final state should be, and then later we could move to that final state all at once.

When I commented 3 days ago my frame of mind was that I was very reluctant to consider any 2.13.9 changes now that we already have a release candidate, but we probably needn't be so strict about reversions — a reversion isn't really a new change, it just restores the previous status quo.

@lrytz said he'd have a look at the details before weighing in.

[ijuma]

My take is that 2.12 and 2.13 should be very careful about changing behavior at this point, so best to err on the side of caution.

[som-snytt]

My take is that the bug is really terrible and must be fixed. The Onion headline: "ConcurrentMap not actually concurrent."

I think null policy is nice to have. Compare the previous fix in #9344 where I also unhappily got involved. If the set of users of the Java wrappers cared about nulls as a critical issue, it would have had more visibility earlier.

I don't think the fix to throw NPE is a critical fix by any stretch.

[lrytz]

"ConcurrentMap not actually concurrent."

To make this explicit (and repeat from the PR description): by using computeIfAbsent and compute, the getOrElseUpdate / updateWith methods become atomic (depending on the underlying Java collection). I agree this is an important fix.

What's the advantage of the new overrides that this PR adds to the non-concurrent wrappers in JMapWrapperLike?

IMO, the following behavior in current 2.13.x is a regression we should iron out before 2.13.9:

scala> val j = new java.util.HashMap[String, String]()
val j: java.util.HashMap[String,String] = {}

scala> import scala.jdk.CollectionConverters._
import scala.jdk.CollectionConverters._

scala> j.asScala.getOrElseUpdate("", null)
val res0: String = null

scala> j.containsKey("") // should be `true`
val res1: Boolean = false

To fix it, we could

• revert the changes to non-concurrent wrappers of this PR, or
• merge Preserve null policy in wrapped Java Maps #10129
• [edit: or both]

I'm fine with either solution. The new PR would fix null handling also for the concurrent wrapper, but that doesn't seem very urgent / important as the two ConcurrentMap implementations in the JDK don't support null.

[som-snytt]

I'm willing to submit whatever sequence of PRs makes Seth's life easier.

I think the contribution from ijuma and the follow-up represent normal "RC" tightening of screws, in the sense of "if there is anyone present who can show just cause why these two PRs may not be merged, speak now or forever hold your peace."

Maybe lightbend can release the PRs under their special paid license plan!

[lrytz]

I'll escalate and see what can be arranged 😅

[SethTisue]

#10129 is merged for 2.13.9. I'll update the release thread on Discourse.