Making java.util.Iterator.remove() for the iterators for EnumSet more resilient

[Joshua Bloch]

Mike et. al.,

I have serious reservations about this. It would be the first time (to my
knowledge) that we deliberately swept a concurrent modification under the
rug.  If we go to the effort of detecting it (which you're doing), the least
we should do is to report it. Also, this file (and most of java.util and
java.lang) does not use braces on single-line if/for/while/do statements.
 Combining these points, this:

 143             if (oldElements != elements[lastReturnedIndex]) {
 144                 size--; 145             }

Should be:

 143             if (oldElements != elements[lastReturnedIndex])
 144                 size--; 145             else

 146                 throw new ConcurrentModificationException();


But I have some doubts as to whether this is worth doing. There was a much
more serious case where we failed to throw a CME that we should have thrown,
and the CCC (an internal review body within Sun, and now Oracle)
ruled<http://bugs.sun.com/view_bug.do?bug_id=4902078>that it
represented an unacceptable compatibility violation.

If we're going to make this change to EnumSet, then we must reopen
4902078<http://bugs.sun.com/view_bug.do?bug_id=4902078>and fix that
too.

   Josh

P.S.  There is a much more serious problem with EnumMap that we might want
to fix.  It is demonstrated by this program:

   *public class Size {*

*    private enum Sex { MALE, FEMALE }*

**

*    public static void main(String[] args) {*

*        printSize(new HashMap<Sex, Sex>());*

*        printSize(new EnumMap<Sex, Sex>(Sex.class));*

*    }*

**

*    private static void printSize(Map<Sex, Sex> map) {*

*        map.put(Sex.MALE,   Sex.FEMALE);*

*        map.put(Sex.FEMALE, Sex.MALE);*

*        map.put(Sex.MALE,   Sex.MALE);*

*        map.put(Sex.FEMALE, Sex.FEMALE);*

*        Set<Map.Entry<Sex, Sex>> set =*

*            new HashSet<Map.Entry<Sex, Sex>>(map.entrySet());*

*        System.out.println(set.size());*

*    }*

*}*

*
*
Predict the behavior of this program.  Then run it and see what happens.
 Only two Map implementations display this disturbing behavior: EnumMap and
IdentityHashMap.  I'm less disturbed by IdentityHashMap, as it's such an
esoteric implementation.  But I could easily imagine someone getting hurt by
the behavior demonstrated by the example above.  Fixing it would decrease
the performance of iterating over entrySets of EnumMaps, but I think it
would probably be worthwhile. I violated the principle of least astonishment
here, and engaged in premature optimization.  Mea culpa, twice.


            Josh


On Tue, Jan 25, 2011 at 11:24 AM, Mike Duigou <mike.duigou at oracle.com>wrote:

> Hi Neil;
>
> This sounds like an excellent suggestion and the changes look good to me. I
> have created an RFE change request (CR# 7014637) and posted the webrev at
> cr.openjdk.java.net:
>
> http://cr.openjdk.java.net/~mduigou/7014637/webrev.00/webrev/
>
> Mike
>
> On Jan 25 2011, at 06:28 , Neil Richards wrote:
>
> > The Javadoc for java.util.Iterator.remove() states that the behaviour
> > of the method is "unspecified" if the underlying collection has been
> > modified.
> >
> > Ideally, Iterator.remove() would be resilient - which is to say, it
> > would not modify the underlying collection if the element to be
> > removed has already been removed from the collection.
> >
> > In the general case, I can see the rationale behind the Javadoc
> > defining things the way they are - it would be overly onerous to
> > require resilient behaviour on iterators for all types of collection.
> >
> > However, in the specific case of the iterators for java.util.EnumSet,
> > I believe it is simple to implement them such that their remove()
> > method does behave in the resilient manner described above.
> >
> > Given that it's basicaly as easy for these to behave resiliently as
> > for them not to, I believe to would be beneficial to change them to be
> > resilient in this fashion.
> >
> > To that end, please find attached a zip file containing a webrev which
> > modifies the Iterators for EnumSet (specifically
> > java.util.RegularEnumSet.EnumSetIterator and
> > java.util.JumboEnumSet.EnumSetIterator) so that they are resilient,
> > together with a couple of testcases to demonstrate the enhancement.
> >
> > I have searched the Java Bug Database, but have not found a bug (or
> > RFE) relating to this item.
> > Therefore, I'm unsure if I should raise an RFE for it there or in the
> > OpenJDK bugzilla.
> >
> > As always, any comments, queries or guidance on any of the above
> > gratefully received.
> >
> > - Neil
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/core-libs-dev/attachments/20110125/808ef7d1/attachment.html>