SI-7115 JMapWrapper.get can incorrectly return Some(null)

This isn't incorrect.  Trying to use a single-threaded interface in a concurrent context is supposed to break in various unpleasant ways.

Documentation has been added to encourage one to avoid wrapping a concurrent map in the generic wrapper (which assumes a single thread), and pointing out that synchronized maps do not maintain synchronization for non-atomic operations (including get).

More docs.

3 files changed, 24 insertions, 1 deletions

diff --git a/src/library/scala/collection/convert/DecorateAsScala.scala b/src/library/scala/collection/convert/DecorateAsScala.scala
index c724831c54..5448f5f91c 100644
--- a/src/library/scala/collection/convert/DecorateAsScala.scala
+++ b/src/library/scala/collection/convert/DecorateAsScala.scala
@@ -135,6 +135,12 @@ trait DecorateAsScala {
    * If the Java `Map` was previously obtained from an implicit or explicit
    * call of `asMap(scala.collection.mutable.Map)` then the original
    * Scala `Map` will be returned.
+   * 
+   * If the wrapped map is synchronized (e.g. from `java.util.Collections.synchronizedMap`),
+   * it is your responsibility to wrap all 
+   * non-atomic operations with `underlying.synchronized`.
+   * This includes `get`, as `java.util.Map`'s API does not allow for an
+   * atomic `get` when `null` values may be present.
    *
    * @param m The `Map` to be converted.
    * @return An object with an `asScala` method that returns a Scala mutable
diff --git a/src/library/scala/collection/convert/WrapAsScala.scala b/src/library/scala/collection/convert/WrapAsScala.scala
index d4ab451b0d..ab151a6778 100644
--- a/src/library/scala/collection/convert/WrapAsScala.scala
+++ b/src/library/scala/collection/convert/WrapAsScala.scala
@@ -133,7 +133,13 @@ trait WrapAsScala {
    * If the Java `Map` was previously obtained from an implicit or
    * explicit call of `mapAsScalaMap(scala.collection.mutable.Map)` then
    * the original Scala Map will be returned.
-   *
+   * 
+   * If the wrapped map is synchronized (e.g. from `java.util.Collections.synchronizedMap`),
+   * it is your responsibility to wrap all 
+   * non-atomic operations with `underlying.synchronized`.
+   * This includes `get`, as `java.util.Map`'s API does not allow for an
+   * atomic `get` when `null` values may be present.
+   * 
    * @param m The Map to be converted.
    * @return A Scala mutable Map view of the argument.
    */
diff --git a/src/library/scala/collection/convert/Wrappers.scala b/src/library/scala/collection/convert/Wrappers.scala
index 7d1d6b3781..9f9732c62f 100644
--- a/src/library/scala/collection/convert/Wrappers.scala
+++ b/src/library/scala/collection/convert/Wrappers.scala
@@ -288,6 +288,13 @@ private[collection] trait Wrappers {
     override def empty: Repr = null.asInstanceOf[Repr]
   }
 
+  /** Wraps a Java map as a Scala one.  If the map is to support concurrent access,
+    * use [[JConcurrentMapWrapper]] instead.  If the wrapped map is synchronized 
+    * (e.g. from `java.util.Collections.synchronizedMap`), it is your responsibility 
+    * to wrap all non-atomic operations with `underlying.synchronized`.
+    * This includes `get`, as `java.util.Map`'s API does not allow for an
+    * atomic `get` when `null` values may be present.
+    */
   case class JMapWrapper[A, B](underlying : ju.Map[A, B]) extends mutable.AbstractMap[A, B] with JMapWrapperLike[A, B, JMapWrapper[A, B]] {
     override def empty = JMapWrapper(new ju.HashMap[A, B])
   }
@@ -314,6 +321,10 @@ private[collection] trait Wrappers {
     def replace(k: A, oldval: B, newval: B) = underlying.replace(k, oldval, newval)
   }
 
+  /** Wraps a concurrent Java map as a Scala one.  Single-element concurrent
+    * access is supported; multi-element operations such as maps and filters
+    * are not guaranteed to be atomic.
+    */
   case class JConcurrentMapWrapper[A, B](underlying: juc.ConcurrentMap[A, B]) extends mutable.AbstractMap[A, B] with JMapWrapperLike[A, B, JConcurrentMapWrapper[A, B]] with concurrent.Map[A, B] {
     override def get(k: A) = {
       val v = underlying get k