Peculiar behavior of ArrayList remove() - Why?

user_3848411 0
Viewed 81

When we remove -1 from and empty ArrayList it throws ConcurrentModificationException and when we remove 0 from same empty ArrayList it throws NoSuchElementException.

Please find the code below :

    public class Test {
    public static void main(String[] argv) {

        ArrayList<Integer> list = new ArrayList<Integer>();
        Iterator<Integer> it = list.iterator();
        try {
            list.remove(-1);
        } catch (IndexOutOfBoundsException e) {

        }
        try {
            it.next();// Throwing ConcurrentModificationException
        } catch (ConcurrentModificationException e) {
            System.err.println("ConcurrentModificationException 1");
        } catch (NoSuchElementException e) {
            System.err.println("NoSuchElementException 1 ");
        }

        list = new ArrayList<Integer>();
        it = list.iterator();
        try {
            list.remove(0);
        } catch (IndexOutOfBoundsException e) {
        }
        try {
            it.next();// Throwing NoSuchElementException
        } catch (NoSuchElementException e) {
            System.err.println("NoSuchElementException 2");
        } catch (ConcurrentModificationException e) {
            System.err.println("ConcurrentModificationException 2 ");
        }

    }
}

From my understanding NoSuchElementException is fine but why ConcurrentModificationException is thrown ?

arraylist java
2 Answers

If you check the code for ArrayList. First a range check is performed, then a modification count is added.

rangeCheck(index);
modCount++;

In the range check method, the range check is for positive numbers only.

if (index >= size)
     throw new IndexOutOfBoundsException(outOfBoundsMsg(index));

So remove(0) doesn't add a mod count, but remove(-1) does. The modCount causes the iterator to throw a ConcurrentModificationException.

user_20674920

It is (IMO) a bug, albeit a very minor one.

The code looks like this (in Java 8):

 public E remove(int index) {
     rangeCheck(index);
     modCount++;
     E oldValue = elementData(index);

The rangeCheck call checks that index < size, but not index >= 0. Presumably, that is to avoid a redundant check ... because the elementData call does a non-optimizable array bounds check that deals with the negative index case.

But the catch is that the modCount++ is happening before the array bounds check. Ooops!

A potential fix would be to move the modCount++ after the elementData call, but this may not be correct. The deal is that modCount is a volatile, and therefore reading / writing it has an implicit synchronizing effect for the access to the elementData that happens next.

Would they fix it if you reported it? Probably not!

  • Fixing it has the potential to break code that "works" if you do it the way I suggested above. Admittedly the code that you are breaking is incorrect anyway because it is relying on the serendipitous synchronization due to the volatile. Even so, people whose code broke would scream. (Not least because this would be a difficult breakage to diagnose.)

  • Alternatively, if you add an explicit size >= 0 check, you most likely make working code measurably slower.

  • The "bug" only affects code that is incorrect anyway. You should not be calling ArrayList::remove concurrently with an iteration.

user_1399850
Related