1

I'm trying to write a game, so every frame I call the doDraw() method where I'm using an iterator to loop through all GameObjects and print them all on screen:

Iterator<GameObject> itr = mObjList.iterator();
    while (itr.hasNext()) {
        GameObject obj = itr.next(); // this line gives me the error
        ...
        // print object
    }

The only method that adds item to the list, is this:

public void click(int x, int y) {
    // adds new object to the list on a click event
    mObjList.add(new GameObject(x, y));
}

Most of the times it works. But sometimes I get this error:

java.util.ConcurrentModificationException

From the line with "itr.next()". From what I've googled, I figured this is because the click() event sometimes happen before the draw() finishes drawing every object, so it's changing the list while the iterator is using it. I suppose this is what's wrong?

But I'm not experienced with threads. How could I possibly fix this? Maybe I'm doing this whole thing wrong and I should use a completely different method to print all objects on screen?

Improve this question

5 Answers 5

Reset to default
4

If the expected number of reads and traversals greatly outnumbers the number of updates to the list, use a CopyOnWriteArrayList.

Otherwise, synchronize on the list (or a dedicated mutex object) when iterating and mutating:

private List<GameObject> mObjList = /* whatever */;
private final Object mListMutex = new Object();

// snip...

synchronized (mListMutex) {
    for (GameObject obj : mObjList) {
        // do your thang
    }
}

// snip...
public void click(int x, int y) {
    GameObject obj = new GameObject(x, y);
    synchronized (mListMutex) {
        mObjList.add(obj);
    }
}
Improve this answer
Sign up to request clarification or add additional context in comments.

Comments

2

I'd like add to the answer from @aleph_null. @aleph_null is correct that this exception happens when you try to modify a collection while you are iterating across it -- only the remove() method on the iterator is allowed. The iterator is trying to protect itself from changes happening to the collection underneath it.

I would not, however, recommend synchronization as the right solution. If you need the behavior of adding stuff to a list while you are processing it then I recommend adding to another list and then calling addAll() once you stop iterating. More GC intensive for sure but cleaner.

Edit:

Sorry, I missed the fact that the click() is an asynchronous event handed by another thread. I assumed that the click() was called inside the loop. You will have to synchronize around the list when you add in click() and around the addAll(). You could use an AtomicReference to record the click and then act on it after the iterator finished but only if you were guaranteed of only one item being clicked on at a time.

Improve this answer

6 Comments

Unfortunately, your suggestion still leaves a synchronization problem. the click method would be adding an element to a temporary list in the UI thread and the same list would be used in the addAll method on a different thread. You would be moving the sync problems from one list to another.
Yes, but it's less of a problem. Queueing up the items requires a lock on a thread-safe queue add - quick. Sychronizing on the list means the adding thread has to block until the draw iteration is complete - a lengthy operation. The drawing thread can empty the queue after each frame-draw iteration is complete, as suggested by @Gray.
..or empty the queue before the iteration starts, or it may be possible to queue up the updates to whatever the draw thread waits on when it is not drawing, (eg. if it's a UI thread, post it a Windows message/BeginInvoke/whateverItIsInJavaIcantRemember). Whatever - the list only gets updated from the drawing thread so no lengthy locking. Basically, the longer a lock is taken, the poorer the performance and the greater the chance of deadlocks.
In fact, +1 for daring to suggest that synchronization over a lengthy operation might be a bad idea.
I missed the fact that click() was an event handled in another thread. I've edited my answer. Thanks.
|
0

The solution is to not use an iterator, or somehow ping-pong between lists so that the one you're modifying is not the one you're iterating through. You could also try to use synchronization, but that requires some fairly sophisticated skills to do well without creating deadlocks.

Improve this answer

13 Comments

I don't think we should be making people afraid of synchronization when it's clearly what's needed. If you don't want to deal with synchronization you have no business building a multithreaded application. And deadlocks are in fact very easy to avoid, especially for trivial synchronization like this (where you're probably not going to need to synchronize on two things at once).
How can I do this without an iterator? And what exactly do you mean by ping-pong between lists?
I've read in tutorials that the "synchronize" keyword is very slow and should be avoided unless there's no other way. Since this is a performance critical part of the code (method running many times per second). Shouldn't I be worried?
"Since this is a performance critical part of the code (method running many times per second). Shouldn't I be worried?" - is this really a part of the code that is critical? did you do any measurement to verify that belief, or was it just a gut feeling? Just do the synchronization, and then when it becomes slow, then go fix it via a non-blocking datastructure.
@VIBrunazo synchronize is not that slow. It used to be very slow, now it's not so bad. It will have an impact, but if you need it, then you need it, and usually it's fast enough not to worry about. Like Chii says, try it and then change it if you have real problem
|
0

noooo, synchronization errors are nasty... especially since they seem to occur randomly. While traversing a collection with an iterator, you cannot modify the collection (unless you use the iterator's remove method, but that's an exception). Doing so results in the exception you're looking at... but only sometimes. The error will occur only when mObjList.add gets called right when you're traversing the iterator.

Improve this answer

1 Comment

The right way to fix this issue is through synchronization. It's not that difficult once you understand the concept... I recommend searching for tutorials... in particular, you can use the "synchronized" keyword or a Semaphore. They will make sure they only ONE of the two conflicting code segments runs concurrently.
0

Your guess about why you're getting the ConcurrentModificationException is right on.

To fix this, you could synchronize on the list itself:

synchronized(mObjList) {
    Iterator itr = mObjList.iterator();
    while (itr.hasNext()) {
        // ...
    }
}

This will allow your thread to acquire a lock on the list itself. If you wrap your access to the list from the other thread in a similar fashion, it will prevent your threads from stepping on each other (see Intrinsic Locks).

I can't tell from context if this would wreak unnecessary havoc in your other thread, but if the list is small-ish it shouldn't be problem. It will certainly cause less problems than an intermittent exception.

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.