Remove java.util.concurrent.ConcurrentMap interface from ImmutableMap

[GoogleCodeExporter]

ImmutableMap implements ConcurrentMap but all the implementation
methods in ImmutableMap throw a UnsupportedOperationException.

While ImmutableMap could implement ConcurrentMap to show that the map
is concurrent safe, I don't see the point in doing so if all the
methods throw an exception.  Documenting that ImmutableMap is thread
safe seems sufficient.

There also is a practical reason not to implement ConcurrentMap.

Scala provides a Java compatible wrapper layer that wraps java.util.*
and java.util.concurrent.* classes in a thin Scala layer that allows
the underlying collection instance to be easily iterated and operated
on with Scala idioms.  Because ImmutableMap implements ConcurrentMap
one could pass the ImmutableMap to the Scala ConcurrentMap wrapper,
but any calls on those methods would throw exceptions, so I wouldn't
think you would want to allow a ImmutableMap to be wrapped in a Scala
ConcurrentMap wrapper.

Original issue reported on code.google.com by blair-ol...@orcaware.com on 14 Jun 2009 at 11:15

[GoogleCodeExporter]

Here was the conversation that happened when ImmutableMap was first
being written:

>  Idle thought: Would it help anyone if this implemented ConcurrentMap?  It
>  would just mean three more methods, all of which throw an exception.  Of 
course,
>  it's more than a little weird to implement an interface by throwing an 
exception
>  whenever of its new methods are called (though all the methods are declared
>  optional, but arguably other things are part of the interface -- like 
"iterating
>  over this map will never throw a ConcurrentModificationException."  (No, 
wait,
>  that's only true of "most" implementations, according to
>  
http://java.sun.com/javase/6/docs/api/java/util/concurrent/package-summary.html
>  "Concurrent Collections."  But it does mean "thread-safe for multiple 
readers.)
>
>  People do seem to declare fields of type ConcurrentMap, but I would assume 
that
>  in most cases they wouldn't benefit from the availability of an immutable
>  version.
>
>  Of course, when it's so easy to do, it may ultimately come down to a 
question of
>  whether implementing the interface is more confusing than not doing so.
>  Thoughts?

The answer was basically "oh, ok, good idea, sure, why not" at the
time.  However, revisiting the last question in this quoted message, I
think it may well be that it's more confusing to implement
ConcurrentMap than not.  Whether a data structure is "concurrent" or
not is meaningless when it can't even mutate in the first place.  I
lean toward removing ConcurrentMap from ImmutableMap before release.

Original comment by kevin...@gmail.com on 27 Jul 2009 at 8:36

[GoogleCodeExporter]

The only advantage I can see to having it implement ConcurrentMap would be that 
it 
could be passed through an API that required ConcurrentMap. One could argue 
that the 
only justification for such an interface is mutation, which appears to be 
correct at this 
point. So I don't suspect it will be tragic to remove it.

That said, my personal inclination is why not leave it as is? What harm does it 
cause? In 
the Scala example the same argument could be made (if I understand it 
correctly) for 
the unimplemented mutators in Map.

Original comment by f...@google.com on 27 Jul 2009 at 9:17

[GoogleCodeExporter]

I don't think something that is read-only needs any more bells and whistles to 
draw 
attention to it being thread-safe. Every collection (modulo WeakHashMap 
perhaps) fits 
that bill. And besides, Kevin you seem to be in doubt, so you know the drill! :D

Original comment by jim.andreou on 27 Jul 2009 at 9:26

[GoogleCodeExporter]

ConcurrentMap isn't about being thread-safe. Map can be a suitable interface 
for 
concurrent map implementations. ConcurrentMap is all about adding new atomic 
operations to ConcurrentMap.

So the real question is is it valid to pass an ImmutableMap to an API requiring 
a 
ConcurrentMap. Which is slightly different than what you just said. :-)

Original comment by f...@google.com on 27 Jul 2009 at 9:38

[GoogleCodeExporter]

Then assuming that a method requires a ConcurrentMap instead of a Map not to 
imply 
thread-safety but to invoke one of the ConcurrentMap's methods, then the answer 
to that 
question is 'no, passing a ImmutableMap is wrong'.

Original comment by jim.andreou on 27 Jul 2009 at 9:42

[GoogleCodeExporter]

I don't see how you can definitively argue that. I can think of plenty of 
correct ways to 
define an API where that would be appropriate. Simply put, I might have a data 
structure that is backed by a map, and could be consumed in either a mutable or 
immutable way (much like Map itself). I don't see a problem with insisting on a 
ConcurrentMap in either case, which ensures that if modifications are made they 
can 
be done so correctly in a concurrent environment (throwing an Exception if 
modifications are attempted on an immutable backing map).

I'm not arguing that this will always be the most intuitive approach. But it 
doesn't 
sound fully implausible, and I don't see any added benefit for being a Map 
instead of 
a ConcurrentMap.

Original comment by f...@google.com on 27 Jul 2009 at 9:50

[GoogleCodeExporter]

I prefer for ImmutableMap to implement ConcurrentMap. If only to make it clear 
that no explicit 
synchronization is necessary while reading the map. For standard maps, it's 
always necessary to perform 
manual synchronization or risk a ConcurrentModificationException.

  public void prettyPrintMap(Map<?, ?> map) {
    if (map instanceof ConcurrentMap) {
      for (Map.Entry<?, ?> entry : map.entrySet()) {
        System.out.println(entry.getKey() + ": " + entry.getValue());
      }
    } else {
      synchronized(map) {
        for (Map.Entry<?, ?> entry : map.entrySet()) {
          System.out.println(entry.getKey() + ": " + entry.getValue());
        }
      }
    }
  }

Original comment by limpbizkit on 28 Jul 2009 at 4:11

[GoogleCodeExporter]

Nowhere in the ConcurrentMap documentation does it say that classes 
implementing it guarantee that they 
will not throw a ConcurrentModificationException.  Not throwing 
ConcurrentModificationException is only 
documented in the ConcurrentHashMap class.  Going by the documentation I could 
implement 
ConcurrentMap in a new class but still throw ConcurrentModificationException's. 
 The sparse ConcurrentMap 
documentation indicates that its designed for mutable atomic operations on the 
map, which ImmutableMap 
doesn't implement.

Unfortunately, this seems to be a naming issue.  The name ConcurrentMap implies 
a certain behavior that one 
would like but its abstract methods imply a different behavior.  ConcurrentMap 
would be better named MutableConcurrentMap or AtomicMutableConcurrentMap, but 
we can't do anything about this.

Original comment by blair-ol...@orcaware.com on 28 Jul 2009 at 6:07

[GoogleCodeExporter]

Having "optional" methods on the collections interfaces with no programmatic 
way to determine whether they 
are supported or not was the one wart on the otherwise excellent collections 
framework.

An ImmutableMap at its heart is really only a Function with a predetermined set 
of valid inputs. It does have 
some additional semantics and performance under concurrent usage, but nothing 
that warrants it implementing 
an interface that contains nothing but mutator methods.

Original comment by jed.wesl...@gmail.com on 29 Jul 2009 at 4:50

[GoogleCodeExporter]

Jed,

"optional" methods are the unfortunate side-effect of single inheritance. If 
you try
creating a separate class for every new "capability" you will end up with an
unmaintainable mess.

For example:

InputStream <- BufferedInputStream
... next, imagine mark()/reset() were missing from InputStream and we wanted 
this
functionality ...

InputStream <- ResetableInputStream
            <- BufferedInputStream <- ResetableBufferedInputStream
... and so on ... every time you add a new "capability" the hierarchy gets 
worse and
worse.

I've yet to see anyone solve this sort of thing in a more graceful manner.

Original comment by gili.tza...@gmail.com on 31 Jul 2009 at 6:31

[GoogleCodeExporter]

This strange decision on our part has been corrected for the next RC.  Thanks 
for 
bringing it to our attention.

Original comment by kevin...@gmail.com on 7 Aug 2009 at 6:24

• Changed state: Fixed