1

I'm using an iterator to loop over a collection as follows:

Iterator<Entity> entityItr = entityList.iterator(); 

    while (entityItr.hasNext())
    {
        Entity curr = entityItr.next();

        for (Component c : curr.getComponents())
        {
            if (c instanceof PlayerControlled)
            {
                ((PlayerControlled) c).pollKeyboard();  
            }
        }
    }

However on the following line I get a ConcurrentModificationException

 Entity curr = entityItr.next();

Why is this happening when I'm not altering anything?

Many thanks

Edit - stack trace:

java.util.ConcurrentModificationException
at java.util.ArrayList$Itr.checkForComodification(Unknown Source)
at java.util.ArrayList$Itr.next(Unknown Source)
at cw.systems.Input.checkInputs(Input.java:31)
at cw.systems.Input.begin(Input.java:21)
at cw.misc.Game.render(Game.java:73)
at com.badlogic.gdx.backends.lwjgl.LwjglApplication.mainLoop(LwjglApplication.java:207)
at com.badlogic.gdx.backends.lwjgl.LwjglApplication$1.run(LwjglApplication.java:114)
Improve this question
9
  • 2
    The stack trace should explain it. Read it, and post it. The exception name and javadoc also explains it: you're modifying the collection while iterating on it. Commented Jan 18, 2015 at 11:58
  • 1
    Are you sure pollKeyboard cannot modify entityList somehow? Commented Jan 18, 2015 at 11:59
  • @SebastianRedl I think if pollKeyboard is modifying entityList somehow the Exception will be thrown from ((PlayerControlled) c).pollKeyboard(); not from Entity curr = entityItr.next(); . right ? Commented Jan 18, 2015 at 12:10
  • 1
    @EpicPandaForce This error is thrown to prevent more serious concurency problems which could be harder to find. What if you enter loop but before you will access element i some element before i was removed shifting other elements? You would end up skipping one element. Commented Jan 18, 2015 at 12:47
  • 1
    Using an iterator allows you to modify a collection while iterating on it, as long as you use a method of the iterator to modify it. And no, the iterator is not rebuilt again. At each iteration, the same iterator object is used to get the next element. Commented Jan 18, 2015 at 12:52

2 Answers 2

Reset to default
4

You must be modifying the list either:

  1. inside your iterator in the pollKeyboard method, without using the add or remove methods on the iterator; or
  2. in another thread

Therefore your exception is the expected behaviour. From the docs, if you have a single thread iterating the list:

if the list is structurally modified at any time after the iterator is created, in any way except through the iterator's own remove or add methods, the iterator will throw a ConcurrentModificationException

and if multiple threads uses the list at one time:

Note that this implementation is not synchronized. If multiple threads access an ArrayList instance concurrently, and at least one of the threads modifies the list structurally, it must be synchronized externally

Solution:

If only one thread accesses the list, make sure you use the entityItr.remove or add methods to modify the list.

For the multi-threaded case you can use Collections.synchronizedList if you do not have a locking object available.

First store a single central reference to your list as:

entityList = Collections.synchronizedList(theOriginalArrayList);

And then access it (with all readers and writers) as:

synchronized (entityList) {
  // Readers might do:
  itr = entityList.iterator();
  while (i.hasNext())
    ... do stuff ...
}

There are other ways to sync multi-threaded access, including copying the list to an array (inside a sync block) and iterating it for reading, or using a ReadWrite lock. They all depend on your exact requirement.

Improve this answer
Sign up to request clarification or add additional context in comments.

8 Comments

Having both a synchronizedList and a synchronized block seems like overkill.
@SebastianRedl . If you just want to call add you can do so directly on the returned list, but for iterators I'll quote the docs again: "synchronizedList returns a synchronized ... list. ... all access must be accomplished through the returned list ... It is imperative that the user manually synchronize on the returned list when iterating over it ... Failure to follow this advice may result in non-deterministic behavior".
Having just a synchronized block around all access code is enough. That makes the additional wrapper indeed overkill.
@MarkoTopolnik synchronizedList() allows you to avoid the sync block when you aren't iterating as methods such as add do their own sync block under the covers. However you still use the sync block for iterating. Therefore as a general pattern you should use synchronizedList(), sync on it for iteration, and use the direct methods for other operations. Yes, if you have a different sync object and use that for all your code you can, but you must sync on every operation.
Sure. But the wider point is that synchronizedList is not able to enforce thread safety of its public API. In fact, you just get an even more complex contract: synchronize this, don't synchronize that, and be very careful not to inadvertently pass the list to a 3rd-party method which may happen to iterate over it.
|
-1

It looks that there is another thread using the same collection and modifing it when this code is iterating over the collection.

ConcurrentModificationException

You can use navite java concurrent collestions instead. They are thread safe. However it's a good habbit to create immutable collections - they are thread safe and enforce you to design reliable code.

Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.