Concurrent Modification when changing maps?

Newbie & Debugging Questions
#1

I created a simple system to change levels. Upon reaching level 2 I get a concurrent modification error any time I try to erase all the objects in my list. Using an iterator doesn’t work, using ArrayList.clear() doesn’t work either. Iv’e exhausted all my options as a nub, what gives?


public void update(float delta){
		
		if(MapManager.isLoading){
			
			for(int i = 0; i < itemList.size(); i++){
				itemList.remove(i);
			}
			
			MapManager.isLoading = false;
			
		}
		
	if(!MapManager.isLoading){
		for(Iterator<Item> itemIter = itemList.iterator(); itemIter.hasNext();){
			Item item = itemIter.next();
			item.update(delta);
			
			if(!item.isAlive()){
				itemIter.remove();
			}
			
			if(EntityManager.playerReference.getRectangle().overlaps(item.getRectangle())){
				item.doSomething();
			}
			
		 	}
		}
	}
	
}
#2

Does the first loop work correctly?

Say you have 2 items in that list:


iteration 1: i = 0; itemList.size()=2 -> remove 0
iteration 2: i = 1; itemList.size()=1 -> does not get executed, one item left!

So shouldn’t it be:

for(int i = itemList.size() - 1; i >= 0; i--) {
    itemList.remove(i);
}

or

itemList.clear();
#3

You (ususally) cannot remove objects from a collection you are currently iterating over. If you want to clear a list completely, use .clear(). Otherwise, create a list where you collect references to the entitites you want to delte in a first iteration and delete them in a second iteration with .remove(entity).

To be more precise:

for(int i = 0; i < itemList.size(); i++){
itemList.remove(i);
}

can be replaced with itemList.clear();

If you still get ConcurrentModException, you are iterating over itemList somewhere else :slight_smile:

#4

[quote]If you still get ConcurrentModException, you are iterating over itemList somewhere else :slight_smile:
[/quote]
That is the most plausible cause if the issue still persists after replacing the first loop with “itemList.clear();”

His second loop looks correct to me, only thing that is redundant is the “if(!MapManager.isLoading)” statement because the loop gets executed anyway, “MapManager.isLoading” always gets set to false in the first block when it is true in the beginning.

#5

In addition to the discussions above, you could also replace the first for loop with an Iterator that removes from your list, just like your second for loop. See the following: http://stackoverflow.com/questions/18448671/how-to-avoid-concurrentmodificationexception-while-removing-elements-from-arr

#6

Well I have tried using itemList.clear() but it gives me the same error. In my map changing code I have the below, and it succeeds in adding them to the list but as soon as they start updating it crashes and gives me a concurrent modification error.

Update : I checked to see if the itemList was of the correct size and it is. It’s only after adding the objects that it completely boots itself.

	public static void loadMapItems(TiledMap tiledMap, TiledMapRenderer renderer) {
		isLoading = true;
		currentRenderer = renderer;

		
		ItemManager.itemList.clear();
		
		if (tiledMap != null) {
			MapLayer collisionObjectLayer = tiledMap.getLayers().get("Objetos");
			MapObjects objects = collisionObjectLayer.getObjects();

			for (MapObject Object : objects.getByType(MapObject.class)) {
				float posX = Object.getProperties().get("x", float.class);
				float posY = Object.getProperties().get("y", float.class);

				if (Object.getProperties().get("item") != null) {

					if (Object.getProperties().get("item").equals("moneda")) {
						ItemManager.itemList.add(new ItemMoneda(posX, posY));
						System.out.println("Item :Moneda: created");
					}

					if (Object.getProperties().get("item").equals("mina")) {
						ItemManager.itemList.add(new ItemMina(posX, posY));
						System.out.println("Item :Mina: Created");
					}

					if (Object.getProperties().get("item").equals("Santi")) {
						ItemManager.itemList.add(new ItemSanti(posX, posY));
						System.out.println("Item :Santi: Created");
					}

				}

				if (Object.getProperties().get("enemy") != null) {

					if (Object.getProperties().get("enemy").equals("malo")) {
						ItemManager.itemList.add(new ItemMalo(posX, posY));
						System.out.println("Item :Malo: Created");
					}

				}

			}
		}
		
	
		
		System.out.println("toot");
		isLoading = false;

	}
#7

That’s fine and all, but what exactly are you doing concurrently?
All the code you provided does in no way enable anyone to help you with your core issue…

When are you calling which method? What is the call hierachy? Are you using more than one thread?

#8

Looks to me like you’re attempting to use the isLoading flag as a mutex.

That’s unlikely to be thread safe.
You need to use proper synchronization.

#9

The only times I iterate over the itemList is when I’m rendering() and updating() the objects. They are not called elsewhere but in the class. I am in the process of using synchronization to see if @Abuse 's help works.

#10

I am also not using multiple threads.

#11

Synchronization did not work.

#12

Well, since you said you were not using multiple threads you don’t have to bother with synchronization.
Providing additional information may be useful if you want to receive help, such as a stack trace including line numbers in your code snippets.
At least for me it isn’t clear where the error exactly occurs.

#13

So where else is isLoading used?
If there aren’t other threads referencing it, then it’s completely superfluous.
If there are other threads referencing it, then it’s almost certainly unsafe.
Either way, it’s a pointer to a problem in design or understanding.

Rather than giving us a useless fragment of code, and having us guess at the cause, stick your code on github in its entirety so we can tell you precisely what the problem is.

Alternatively, learn to use a debugger.

#14

I wonder if this is a job for CopyOnWriteArrayList?

If iterations outnumber mutations (which is usually the case if you are using the list in a render loop), then this could be just the ticket.

It pretty much works the same as an ArrayList, but with elements that make it thread safe.

I think we are getting to the point where almost any use of Synchronization is suspect, given all the new thread-safe and functional-programming based tools that have been added to Java.

#15

The JVM begs to differ: [quote] I get a concurrent modification error …
[/quote]

#16

[/quote]
Not strictly true.

[quote]Note that this exception does not always indicate that an object has been concurrently modified by a different thread. If a single thread issues a sequence of method invocations that violates the contract of an object, the object may throw this exception. For example, if a thread modifies a collection directly while it is iterating over the collection with a fail-fast iterator, the iterator will throw this exception.
[/quote]
The provided code fragment could be doing this, but there’s no way for us to know if collisionObjectLayer.getObjects() is the same Collection as ItemManager.itemList

#17

You really need to add stacktraces and correct line numbers to this sort of questions. This way we are just guessing.

But since I am usually good at guessing, I’ll give it a try: could it be, that you are adding items to the collisionObjectsLayer, when you are adding them to the ItemManager.itemList?

#18

Maybe you should consider using two lists for the game entities?


List<Entity> oldList; 
List<Entity> newList;

I hope you can get rid from your persuasive problems …


#19

You said in your opening post that ArrayList.clear() fails with ConcurrentModificationException.

This PROVES multiple threads are modifying the ArrayList.

#20

isn’t that just a mod-count check, which could be created by something in the loop ?