I have created an issue for fixing the introduced regression and created a regression test that would have caught the error (fix a bug, write a test...).
8008785: IdentityHashMap.values().toArray(V[]) broken by JDK-8008167 http://cr.openjdk.java.net/~mduigou/JDK-8008785/0/webrev Mike On Feb 24 2013, at 12:27 , Peter Levart wrote: > Hi Mike, > > I thought I saw that too when you commited the change, but then re-examinig > the whole source in detail, I couldn't spot it again. I must have stared at > the wrong third of change... > > Regards, Peter > > On 02/24/2013 07:53 PM, Mike Duigou wrote: >> Ouch, this would have been introduced by me. >> >> I will check to see how this could have passed the pre-commit regression >> testing. I suspect that a regression test needs to be improved. >> >> Mike >> >> On Feb 24 2013, at 10:48 , Alan Bateman wrote: >> >>> On 24/02/2013 15:49, Peter Levart wrote: >>>> Hi Alan, >>>> >>>> I checked and it seems all 3 IHM views [keySet|values|entrySet] have a >>>> fail-fast iterator implementation >>>> (IdentityHashMap.IdentityHashMapIterator) and all 3 are (were) using the >>>> iterator for .toArray implementations. So this patch tries to preserve the >>>> behavior when there is a concurrent modification (which is only possible >>>> from other thread and is illegal usage anyway since IHM is not >>>> thread-safe) while executing the toArray methods on the views... >>>> >>>> Do you see something I don't see? >>> My apologies, I see it does check the modification count in >>> IdentityHashMapIterator.nextIndex. >>> >>> However, as this forced me to looks at the changes-set again then the copy >>> loop in Values.toArray has caught by eye: >>> >>> for (int si = 0; si < tab.length; si += 2) { >>> if (tab[si++] != null) { // key present ? >>> // more elements than expected -> concurrent >>> modification from other thread >>> if (ti >= size) { >>> throw new ConcurrentModificationException(); >>> } >>> a[ti++] = (T) tab[si]; // copy value >>> } >>> } >>> >>> Looks like si is incrementing by 3 rather than 2 (which ironically will >>> cause a CME later because there will be fewer elements copied than >>> expected). >>> >>> Do you concur? If so then we can create a bug to change this to test >>> tab[si] and copy in tab[si+1]. >>> >>> -Alan >
