Sign in to follow this  
Addictman

[java] Annoying error java.util.ConcurrentModificationException

Recommended Posts

This is basically what I am told: java.util.ConcurrentModificationException at java.util.AbstractList$Itr.checkForComodification(AbstractList.java:448) at java.util.AbstractList$Itr.next(AbstractList.java:419) at java.util.AbstractCollection.remove(AbstractCollection.java:254) Now, I read the API and it does actually mentioned this Exception. However, I still really can't say what's causing it in my code.. I have an ArrayList, and I add/remove to/from it. That's basically it. At a seemingly random time, it decides to crash on me, and print out this exception. Any known pitfalls and pratfalls here? :)

Share this post


Link to post
Share on other sites
It means what it says :) You've tried to make two concurrent modifications to the same collection, which usually means:
- You're iterating over a collection in one place, and trying to modify (without using the iterator) it at the same time - usually hidden in some other method.

- You've got two threads that are using the same collection, and one is iterating over while the other modifying it.

The first is usually predictable and easily fixed. The second usually ends up showing up at seemingly random times so that sounds like what you've got this time. Remember that even if you don't actually create and threads yourself you still get other threads in the form of the AWT event handler etc.

Share this post


Link to post
Share on other sites
Quote:
Original post by OrangyTang
It means what it says :) You've tried to make two concurrent modifications to the same collection, which usually means:

- You're iterating over a collection in one place, and trying to modify (without using the iterator) it at the same time - usually hidden in some other method.


So basically if I have keyboard actions or mouse actions that modify the renderqueue (add/remove) while rendering (iteration)is going on, I'm fucked? :) sounds damn fragile. Since the listeners run on the AWT thread, I guess that indicates the problem of multithreading as well. such lovely business ;)

Oh well. Thanks for the clarification anyways!

Share this post


Link to post
Share on other sites
The short answer is that you have designed your program wrong if this ever happens and is non-trivial to solve.

Usually, anything that has to iterate over something that might change (e.g. the render thread has to read lots of data from different places) it makes a COPY to iterate over - because if it doesn't then you get effects similar to tearing anyway, so you should NEVER have a problem with the CCME (because your fix to make your rendering prettier avoids this problem).

E.g.

ArrayList readableCopy = (ArrayList) originalList.clone();

Share this post


Link to post
Share on other sites
Redmilamber, I tried cloning the ArrayList, just to test if that would fix it. It didn't, so I'm still lost I guess.

Right now, the rendering process iterates and calls methods on the cloned arraylist, so that the real list is not modified during iteration anymore.

Share this post


Link to post
Share on other sites
clone() won't work because it only returns a shallow copy of the collection. Which means both copies share the underlying data structure. You have 2 options to overcome this. Use the toArray method and use a for loop to go through the array. Or you can create your own copy:

//initialize
ArrayList drawList = new ArrayList();

//drawing
public void draw() {
drawList.clear();
drawList.addAll(objectList);

for(Iterator it=drawList.iterator();it.hasNext();) {
//draw it
}
}

Share this post


Link to post
Share on other sites
Or you can synchronize on the array list when you modify it. Or better still, create the list as a synchronized collection:


ArrayList myList = (ArrayList)Collections.synchronizedList(new ArrayList());


Share this post


Link to post
Share on other sites
Better yet, I took a completely different approach and made my own RenderQueue class based off of a primitive array. now, it not only works seamlessly, but it tuned off several milliseconds as well ;) So I am happy again.

Share this post


Link to post
Share on other sites
Quote:
Original post by Aldacron
Or you can synchronize on the array list when you modify it. Or better still, create the list as a synchronized collection:


Yes, except that synchronizing the ArrayList can cause a big performance hit.

Share this post


Link to post
Share on other sites
Yes, synchronizing everything might not choke my game at this specific stage, but in the end when the renderqueue's fill up with lotsa objects, and the number of actions modifying/editing these queues grow (running on a different thread), the combine access time for the synchronous methods will probably cause a noticable setback.

It took a little extra work writing the lists as a static array, and I need to set a limit for how many objects that can enter the queues, but in the end it all turns out good. However, I'd like some feedback on the code itself. It is likely to be error-prone, and quite possibly there are several optimization tricks that can be done.

note: dont take synchronous issues in to mind right now. I didnt worry about it yet.



public class RenderList {

public final static int DEFAULT_SIZE = 1000;
private IRender[] objectList;
public int nextIndex;
public int highestIndex;

public RenderList()
{
this(DEFAULT_SIZE);
}

public RenderList(int size)
{
objectList = new IRender[size];

nextIndex=0;
highestIndex = -1;
}

/* registers an object to the list */
public void registerObject(IRender ob)
{
if( nextIndex>=objectList.length ||
highestIndex>=objectList.length)
return;

if(nextIndex==-1 || nextIndex>highestIndex)
{
objectList[++highestIndex]=ob;
nextIndex=highestIndex+1;
}
else
{
objectList[nextIndex]=ob;
// often the next index might also be free
if(objectList[nextIndex+1]==null)
{
nextIndex++;
}
// otherwise, lets search for it.
else
nextIndex=getNextFreeIndex();
}
}

/* gets the index of an object. -1 if its not in the list */
public int memberArray(IRender ob)
{
for(int i=0;i<(highestIndex+1);i++)
{
if(objectList[i]==null)
continue;

if(objectList[i].equals(ob))
return i;
}
return -1;
}

/* removes an object from the list */
public void removeObject(IRender ob)
{
int index=memberArray(ob);
if(index==-1)
{
return;
}

objectList[index]=null;

// if this index is less than nextIndex,
// nextIndex becomes this index
if(index<nextIndex || nextIndex==-1)
nextIndex=index;

// if the highest index is this index, or higher
// than this index, reduce highest index
if(highestIndex>=index)
{
while(highestIndex>=0)
{
if(objectList[highestIndex]==null)
highestIndex--;
else
{
if(nextIndex>(highestIndex+1))
nextIndex=(highestIndex+1);
break;
}
}
}


}

/* get a free index where a new object can be inserted */
public int getNextFreeIndex()
{
for(int i=0;i<highestIndex;i++)
{
if(objectList[i]==null)
return i;
}
return highestIndex+1;
}

/* retrieve an object from the list */
public IRender getObject(int i)
{
if(i<=objectList.length)
{
return objectList[i];
}
return null;
}

public void render(Graphics2D g)
{

for(int i=0;i<(highestIndex+1);i++)
{
if(objectList[i]==null)
continue;

objectList[i].render(g);
}
}

public int getListsize()
{
return objectList.length;
}
}

Share this post


Link to post
Share on other sites
You are doing a lot of work that is already done for you in Arraylist. And ArrayList is also already optimized for you. Plus when you render code iterates over the entire list, it includes empty objects, which eats up cycles. It would be better to try and work your way around the concurrency issue.


import java.util.ArrayList;


public class RenderList {

private ArrayList objectList; // containes IRender objects
private ArrayList renderObjects; // containes IRender objects

public RenderList() {
objectList = new ArrayList();
renderObjects = new ArrayList();
}

/* registers an object to the list */
public void registerObject(IRender ob) {
if(!objectList.contains(ob) {
objectList.add(ob);
}
}

/* gets the index of an object. -1 if its not in the list */
public int memberArray(IRender ob) {
int result = -1;

if(objectList.contains(ob)) {
result = objectList.indexOf(ob);
}

return result;
}

/* removes an object from the list */
public void removeObject(IRender ob) {
if(objectList.contains(ob)) {
objectList.remove(ob);
}
}

/* retrieve an object from the list */
public IRender getObject(int i) {
IRender result = null;

if(objectList.size() > i) {
result = (IRender)objectList.get(i);
}

return result;
}

public void render(Graphics2D g) {
renderObjects.clear();
renderObjects.addAll(objectList);

for(Iterator it=renderObjects.iterator();itHasNext();) {
IRender temp = (IRender)it.next();
temp.render(g);
}
}

public int getListsize() {
return objectList.size();
}
}

[\source]

Share this post


Link to post
Share on other sites
Thanks for the valiant attempt, but I still get the concurrent exception...

And btw, my list never iterates over the entire list, it only iterates between 0..highestIndex, and all the empty elements come after highestIndex. It is possible that gaps form, but they are always cleared up at the next tick.

I decided I hate ArrayList :(

Share this post


Link to post
Share on other sites
Quote:
Original post by Addictman
Thanks for the valiant attempt, but I still get the concurrent exception...

And btw, my list never iterates over the entire list, it only iterates between 0..highestIndex, and all the empty elements come after highestIndex. It is possible that gaps form, but they are always cleared up at the next tick.

I decided I hate ArrayList :(


Your missing the point. Creating your own RenderList will avoid the ConcurrentModificationException, but you have probably not removed the problem. Instead it will manifest itself by producing other more obscure crashes later. When writing a multithreaded application you need to know all about synchronization, or you will be fucked sooner or later.

To synchronize your code you need to add the following block around every place you use the list, wich includes iterators:

synchronized (renderList) {
// use renderList, or its iterator here
}


Also be carefull when you iterate the list. You can not modify the original list when using it's iterator. Maybe you should use the get function instead.

Good luck!

Share this post


Link to post
Share on other sites
I remember having this error. I think I treated it in the same way you did, but I know that it's not a good solution.

Maybe you could just queue up changes and carry them out between iterations?

Share this post


Link to post
Share on other sites
Quote:
Original post by CaptainJester
Yes, except that synchronizing the ArrayList can cause a big performance hit.


That's a rather generic statement. The performance hit from using a synchronized list is highly app dependant, and you have to be making heavy use of the list for the hit to be 'big'. This is a case of 'use what works until profiling says otherwise'.

Share this post


Link to post
Share on other sites
Guest Anonymous Poster
Quote:
Original post by Addictman
Thanks for the valiant attempt, but I still get the concurrent exception...

And btw, my list never iterates over the entire list, it only iterates between 0..highestIndex, and all the empty elements come after highestIndex. It is possible that gaps form, but they are always cleared up at the next tick.

I decided I hate ArrayList :(


Sounds like perhaps you chose the wrong data structure.

Just your description there suggests that a plain old linkedlist could be easier and faster for you (although that won't remove your bugs in your program that are causing CME's)

Why did you choose an AL in the first place? Was it because you didn't know any others?

redmilamber

Share this post


Link to post
Share on other sites
Guest Anonymous Poster
Quote:
Original post by Aldacron
Quote:
Original post by CaptainJester
Yes, except that synchronizing the ArrayList can cause a big performance hit.


That's a rather generic statement. The performance hit from using a synchronized list is highly app dependant, and you have to be making heavy use of the list for the hit to be 'big'. This is a case of 'use what works until profiling says otherwise'.


There's a thread on this around here where I make the same point "In general, synchronized data structures offer no noticeable penalties these days" and there's a microbenchmark which demonstrates something like a 10% performance difference between synch and non-synch Collections classes.

http://gamedev.net/community/forums/topic.asp?topic_id=263837

redmilamber

Share this post


Link to post
Share on other sites
Quote:
Original post by Aldacron
Quote:
Original post by CaptainJester
Yes, except that synchronizing the ArrayList can cause a big performance hit.


That's a rather generic statement. The performance hit from using a synchronized list is highly app dependant, and you have to be making heavy use of the list for the hit to be 'big'. This is a case of 'use what works until profiling says otherwise'.


That is why I said CAN CAUSE.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this