Map.forEach

[Martin Buchholz]

On Mon, Dec 9, 2013 at 7:49 PM, Mike Duigou <mike.duigou at oracle.com> wrote:

>
> On Dec 9 2013, at 17:50 , Martin Buchholz <martinrb at google.com> wrote:
>
> Current ConcurrentMap.forEach
>
> http://gee.cs.oswego.edu/dl/concurrent/dist/docs/java/util/concurrent/ConcurrentMap.html#replaceAll-java.util.function.BiFunction-
> has two different "specs" for the default method:
>
> *Implementation Requirements:* The default implementation is equivalent
> to, for this map:
>
>
>  for (Map.Entry<K,V> entry : map.entrySet())
>    action.accept(entry.getKey(), entry.getValue());
>
>
> *Implementation Note:* The default implementation assumes that
> IllegalStateException thrown by getKey() or getValue() indicates that the
> entry no longer exists. Operation continues for subsequent entries.
> But these are contradictory!
>
>
> I intentionally omitted the exception handling from the pseudo code to
> make it more readable. Barring entries being removed, the pseudo-code is
> accurately describes what happens. This is true for both the ConcurrentMap
> and Map implementations.
>
> I'd prefer to change pseudo-code, if that's what's wanted, than the
> implementations.
>
>  Furthermore, given that any Map might end up giving us ISE, shouldn't we
> be using the ConcurrentMap implementation in Map (and specifying the
> ISE-skipping behavior?)  Whether or not to skip ISE should not be an
> Implementation Note, I think - it should be "spec".
>
>
> Skipping removed entries makes sense for ConcurrentMap. For the basic Map
> it's assumed that it's not concurrent and thus an ISE is actually a sign
> that a CME should be thrown (which is what it does)
>

 Hmmm... it was time that I studied Map.forEach....  I see you convert to
ISE to CME ...

(Synchronized maps (like Hashtable) do not implement ConcurrentMap.  Is
that a bug?)

Imagine a third party implementation of a synchronized map (again, like
Hashtable).  Its existing methods are synchronized, but in jdk8 it inherits
Map.forEach (not ConcurrentMap.forEach, because like Hashtable, it does not
implement ConcurrentMap) and so might throw CME even though its
implementation may never throw this and its entrySet may be designed for
concurrent traversal.

Another argument - before jdk8, whether a Map also implemented
ConcurrentMap was mostly symbolic, and could not affect behavior.  Users
could add "implements ConcurrentMap" purely for its documentation value.
 Where possible, we should try to preserve that property.

Oh, and seriously, should Hashtable implement ConcurrentMap today?  It
appears to implement all of its methods in a thread-safe way.