3

I am getting a concurrent modification exception on the following code:

        for(Iterator<Tile> iter = spawner.activeTiles.iterator(); iter.hasNext();) {
            Tile tile = iter.next();
            canvas.drawRect(tile, tile.getColor());
        }

I understand that concurrent modification happens when it is changed while it is iterating(adding/removing inside of the iteration). I also understand that they can happen when multithreading which is where I think my problem is.

In my game I have a few timers that run on threads. I have a spawner, which adds values to activeTiles on each tick. I then have a 2 timers, one for fading in and one for fading out. Without giving away my game, the tile is basically removed from the list when the fade out has finished, or when the player taps the tile. So there are a few instances where the tiles are removed from the list of tiles:

for(Iterator<Tile> iter = spawner.activeTiles.iterator(); iter.hasNext();) {
                Tile tile = iter.next();

                if(tile.contains(x, y) && tile.equals(spawner.activeTiles.get(0))) {
                    vibrator.vibrate(50);

                    tile.setBroken(true);
                    score ++;
                    spawner.setTileDelayInfo();
                    iter.remove();

and before each new spawn, it removes all of the failed tiles:

private void removeFailedTiles() {
    for(Iterator<Tile> iter = activeTiles.iterator(); iter.hasNext();) {
        Tile tile = iter.next();

        if(tile.isFailed()) {
            iter.remove();
        }
    }
}

It almost seems to happen randomly. So I think it has to do something with timing, but I am new to this kind of exception and don't really know what to look for or why this is happening.

Improve this question
2
  • If it "seems to happen randomly" it's likely a multithreading issue. Commented May 18, 2015 at 22:22
  • 2
    @Ata, he's using an iterator, is not the same question. Actually, using the iterator is the answer to that question, so he's doing it the right way! Commented May 18, 2015 at 22:28

4 Answers 4

Reset to default
2

The good news: you nailed the root cause of the problem in your question - you can't have multiple threads accessing a list at the same time unless they're all just reading.

You can address this in one of two ways, depending on how the rest of your code operates. The most 'correct' way is steffen's answer: any list access should be guarded with a synchronized block, and that includes holding the lock for the full duration of any list iterations. Note that if you do this, you want to do as little work as possible while holding the lock - in particular, it's a bad idea to do any sort of listener callbacks while holding a lock.

Your second option is to use a CopyOnWriteArrayList, which is thread-safe and doesn't require any external synchronization - but any modifications to the list (add/remove/replace calls) become significantly more expensive.

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

Comments

1

Multithreading can be a source of ConcurrentModificationExceptions. It can happen when one thread is modifying the structure of the collection while another thread has an Iterator iterating over it. This can lead to unexpected states in your application when the state of the collection changes when a section of code needs a consistent view of the data. This is needed when you're iterating over a collection of Tiles.

You need to syncrhonize access to the activeTiles collection. Anything that modifies this collection structurally (add or remove), or iterates over it, must synchronize on this collection.

Add a synchronized (activeTiles) block around all code that iterates or structuraly modifies activeTiles. This includes all 3 code snippets you've provided here.

Alternatively, you can make the 3 methods corresponding to your code snippets synchronized.

Either way, no other Thread can execute any of the synchronized blocks until another Thread is finished with its syncrhonized section, preventing the ConcurrentModificationException.

Improve this answer

11 Comments

adding synchronized on function would synchronize access to function but not to the data-structures that are being used inside it. It can still happen that two different functions are accessing same list.
@Ata Adding synchronized on these methods will lock on the object instance, which is the same object instance in each thread. The effect is the same as in synchronized(activeTiles); the code that iterates or modifies activeTiles can only be executed by one thread at a time.
Yes, all code that either iterates over the activeTiles or structurally modifies it must be synchronized.
What is the syntax for this? Ive never used synchronized before
@KyleJensen There is the syntax synchronized methods and the syntax for synchronized blocks.
|
1

It's not safe to remove elements with an Iterator that supports element-removal, when you're iterating the collection in another thread.

Acquire a Lock in all threads on activeTiles before iterating them.

Improve this answer

Comments

1

You might want to make your list thread-safe. Use Collections.synchronizedList().

threadSafeActiveTiles = Collections.synchronizedList(activeTiles);

Mind that you must synchronize on that list when iterating over it:

synchronized (threadSafeActiveTiles) {
    for (Iterator<Tile> it = threadSafeActiveTiles.iterator(); it.hasNext();) {
        Tile tile = it.next();
        // ...
    }
}

You then can safely have multiple threads modifying the list, which seems to be your case.

The list returned by Collections.synchronizedList() saves you from having to use the synchronized block (above) in single operations on that list, like add(e), size(), get(i) and so on...

Improve this answer

1 Comment

synchronizedList isn't sufficient if you're iterating over the list, since there's no lock being held over the full length of the iteration - so you can still get ConcurrentModificationExceptions just like it wasn't.

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.