3

I have a synchronized method called getChunks(). This is the only way the set of chunks are called in the whole program, yet when iterating through them (by calling getChunks()) I get a concurrentModificationException. This would be because the ChunkManager class, which runs in a seperate thread, would have generated a new chunk. But the only way it accesses the chunks is though getChunks()...

The getChunks() method: 

public synchronized Set<WorldChunk> getChunks() {
    return chunks;
}

The render() method, where the exception occurs

public void render() {
    for(WorldChunk wc : chunkMap.getChunks()) { // <-- This is the line where the exception occurs
        wc.render();
    }
}

The ChunkManager Class

public class ChunkManager implements Runnable {

    private static final int CHUNK_UPDATE_DELAY_MILLIS = 100;

    private ChunkMap chunkMap;

    public ChunkManager(ChunkMap chunkMap) {
        this.chunkMap = chunkMap;
        new Thread(this).start();
    }

    @Override
    public void run() {
        while(true) {
            manageChunks();
        }
    }

    private void manageChunks() {
        int cx = (int) ((Camera.getX() + WorldChunk.CHUNK_WIDTH / 2) / WorldChunk.CHUNK_WIDTH);
        int cz = (int) ((Camera.getZ() + WorldChunk.CHUNK_DEPTH / 2) / WorldChunk.CHUNK_DEPTH);
        int renderDistance = 2;
        for(int icx = cx - renderDistance; icx < cx + renderDistance; icx++) {
            for(int icz = cz - renderDistance; icz < cz + renderDistance; icz++) {
                if(!chunkMap.hasChunk(icx, icz)) {
                    chunkMap.genChunk(icx, icz, false);
                }
            }
        }
        for(WorldChunk wc : chunkMap.getChunks()) {
            if((Math.abs(wc.getX() - Camera.getX()) + Math.abs(wc.getZ() - Camera.getZ())) > WorldChunk.CHUNK_WIDTH + WorldChunk.CHUNK_DEPTH)  {
                wc.unload();
            }
        }
        try {
            Thread.sleep(CHUNK_UPDATE_DELAY_MILLIS);
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
    }

}

EDIT StackTrace:

Exception in thread "main" java.util.ConcurrentModificationException
at java.util.HashMap$HashIterator.nextEntry(HashMap.java:894)
at java.util.HashMap$KeyIterator.next(HashMap.java:928)
at java.util.AbstractCollection.addAll(AbstractCollection.java:333)
at java.util.HashSet.<init>(HashSet.java:117)
at com.ryxuma.kalidus.world.World.render(World.java:35)
at com.ryxuma.kalidus.Core.renderPerspective(Core.java:35)
at org.heatstroke.Heatstroke$Runner.run(Heatstroke.java:365)
at org.heatstroke.Heatstroke.start(Heatstroke.java:124)
at com.ryxuma.kalidus.Core.main(Core.java:60)
Marek Sebera
  • 39,650
  • 37
  • 158
  • 244
Ryxuma
  • 672
  • 5
  • 18
  • Some thread updates chunks while you're in method render, you can avoid it by synchronizing around `chunkMap` – Marek Sebera Nov 29 '13 at 17:49
  • @MarekSebera But I thought by synchronizing the getChunks() method, the thread that updates the chunks would wait for the iteration to finish before chaning it – Ryxuma Nov 29 '13 at 17:52
  • Any stacktrace you can post? Which line throws the exception? – Daniel Nov 29 '13 at 17:52
  • @Ryxuma: No; it just waits for that method to return. It doesn't have anything to do with the object it returns. – SLaks Nov 29 '13 at 18:07
  • 1
    Possible duplicate of [ConcurrentModificationException despite using synchronized](http://stackoverflow.com/questions/1655362/concurrentmodificationexception-despite-using-synchronized) – Raedwald Mar 28 '16 at 15:02

2 Answers2

2

Your operation to get a reference to the set is thread safe, but nothing you do with that set is thread safe from that point. A simple solution is to either

  • return a copy of the set
  • lock the set while iterating
  • use a set which doesn't throw CME like CopyOnWriteArraySet (Which can be more expensive when you update it)

EDIT: For those interested, if you want a ConcurrentHashSet there is a trick you can use 

Set <E> set = Collections.newSetFromMap(new ConcurrentHashMap<E, Boolean>());
Peter Lawrey
  • 525,659
  • 79
  • 751
  • 1,130
1

What does your wc.unload() method do?

If you try to add/remove elements from a Set while you are iterating on it, you can receive this error.

Look here: http://docs.oracle.com/javase/1.5.0/docs/api/java/util/ConcurrentModificationException.html

Specially this part:

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.

For a solution, the easiest I can think of, keep track of all the elements you want to remove and remove them after you are done iterating by using Set.removeAll().

If you want, take a look at java.util.concurrent where you can find collections that allow you to modify as you iterate (keep an eye for how they work though, as that can be not what you expect).

Daniel
  • 21,933
  • 14
  • 72
  • 101