ConcurrentModificationException on synced lists...

Drountian

I have the skeleton of a very simple space invaders inspired game here...currently all the projectiles and enemies drawn on the screen are being kept in synchronized lists. I think I correctly obtained these from Collections.synchronizedList() on lines 11 and 12 of SpaceInvaders...but when the enemy list is edited in getCollision() (SpaceInvaders 94) there is a ConcurrentModificationException thrown...this code should be synchronized on both lists...there are synchronization blocks around the all of this method. Why is this exception being thrown? Any Ideas?

OS: Windows 7 Processing Version: 2.2.1

Code Files: SpaceInvaders:

java.util.List enemies;
java.util.List friendlyShots;
Player player;
Object lock;

void setup () {
  size(720, 600, P2D);

  //Only friendly shots should be acessed by multiple threads simultaneously...but I was getting errors
  //and initialized both lists as synchronized.
  enemies = java.util.Collections.synchronizedList(new ArrayList<Enemy>());
  friendlyShots = java.util.Collections.synchronizedList(new ArrayList<Projectile>());

  //Setup the enemies array, uses two loops to place the enemies easily.
  synchronized(enemies) {
    player = new Player(360, 550);
    for (int i = 0; i < 10; i++) {
      for (int j = 0; j < 3; j++) {
        //ENEMY_SPACING is the space between each enemy, ENEMY_PAD is the space between the left side and the first enemy, ENTITY_SIZE is the size of an enemy, all in pixels.
        enemies.add(new Enemy(Enemy.ENEMY_SPACING*i + Enemy.ENTITY_SIZE*i + Enemy.ENEMY_PAD, Enemy.ENEMY_SPACING*j + Enemy.ENTITY_SIZE*j + Enemy.ENEMY_PAD, 0, j*10 + i));
      }
    }
  }
}


void draw() {
  background(0);

  //Update and draw the friendly shots
  synchronized(friendlyShots) {
    for (Object o : friendlyShots) {
      //Cast
      Projectile p = (Projectile) o;
      //Update, this does collision detection, see Projectile.update() and collidingWith(boolean enemy, Entity projectile) below.
      p.update();
      //Draw
      render(p.getShape(), p.getX(), p.getY());
      println("Rendered Projectile");
    }
  }

  //Update and draw the enemies.
  synchronized(enemies) {
    for (Object o : enemies) {
      //Cast
      Enemy e = (Enemy) o;
      //Update, this is empty for now.
      e.update();
      //Draw
      render(e.getShape(), e.getX(), e.getY());
      println("Rendered Enemy");
    }
  }

  //Update the player
  player.update();
  //Render the player
  render(player.getShape(), player.getX(), player.getY());

  //If the mouse is too far to the right of the player, move the player right, move the player left if the inverse is true, otherwise don't move the player.
  if (mouseX > player.getX() + 5) {
    player.goRight();
  } else if (mouseX < player.getX() - 5) {
    player.goLeft();
  } else {
    player.stop();
  }
}

void mousePressed() {
  //This should be handled right after draw...but just in case let's synchronize.
  synchronized(friendlyShots) {
    //Add new projectile to the projectile array.
    friendlyShots.add(new Projectile(player.getX(), player.getY(), Projectile.FRIENDLY));
  }
}

//When this is not commented I get a concurrent modification exception from code here...but it should be synchronized right?
//This is called by the projectile update function.

boolean collidingWith(boolean enemy, Entity projectile) {
  float x = projectile.getX();
  float y = projectile.getY();
  boolean returnVal = false;

  if (enemy) {
    synchronized(enemies) {
      for (Object o: enemies) {
        println("Looking through enemies");
        Enemy e = (Enemy) o;
        if (abs(x - e.getX()) < Enemy.ENTITY_SIZE/2&&abs(y - e.getY()) < Enemy.ENTITY_SIZE) {
          enemies.remove(e.getID());
          returnVal = true;
        }
      }
    }
  }
  return returnVal;
  //TODO: Put player collision detection.
}

//Renders a shape
void render(PShape s, float x, float y) {
  pushMatrix();
  translate(x, y);
  shape(s);
  popMatrix();
}

Entity:

/* This is the base class for all the moving things on the screen, player, projectile and enemy.*/

public abstract class Entity {
  public static final byte STOP = 0;
  public static final byte NORTH = 1;
  public static final byte SOUTH = 2;
  public static final byte EAST = 3;
  public static final byte WEST = 4;

  private float x, y;
  private float speed;

  protected byte movement;

  protected PShape s;

  protected Entity (float x, float y, float speed) {
    this.x = x;
    this.y = y;
    this.speed = speed;
    movement = STOP;
  }

  public abstract void update();

  protected Entity () {
    this(0, 0, 0);
  }

  public abstract PShape getShape();

  public void setSpeed(float speed) {
    this.speed = speed;
  }

  public float getX() {
    return x;
  }

  public float getY() {
    return y;
  }

  public void setX(float x) {
    this.x = x;
  }

  public void setY(float y) {
    this.y = y;
  }

  public void incXBy(float dx) {
    x += dx;
  }

  public void incXBySpeed(boolean invert) {
    if(invert){
      incXBy(-speed);
    }
    else {
      incXBy(speed);
    }
  }

  public void incYBy(float dy) {
    y += dy;
  }

  public void incYBySpeed(boolean invert) {
    if(invert){
      incYBy(-speed);
    }
    else {
      incYBy(speed);
    }
  }
}

Enemy:

public class Enemy extends Entity {
  public static final int TYPE_0 = 0;
  public static final int TYPE_1 = 1;
  public static final int TYPE_2 = 2;

  public static final int ENEMY_SPACING = 40;
  public static final int ENTITY_SIZE = 30;
  public static final int ENEMY_PAD = 45;

  private int type;
  private int id;

  public Enemy(float x, float y, int type, int id) {
    super(x, y, 1);
    this.type = type;
    this.id = id;

    s = createShape();
    s.beginShape();
    s.noStroke();
    s.fill(255);
    s.vertex(-15, 0);
    s.vertex(15, 0);
    s.vertex(15, 30);
    s.vertex(7, 30);
    s.vertex(7, 20);
    s.vertex(-7, 20);
    s.vertex(-7, 30);
    s.vertex(-15, 30);
    s.endShape(CLOSE);
  }

  public int getID() {
    return id;
  }

  @Override
  public void update(){}

  public PShape getShape() {
    return s;
  }
}

Projectile:

public class Projectile extends Entity {
  public static final int FRIENDLY = 0;
  public static final int ENEMY = 1;

  private int side;

  public Projectile(float x, float y, int side) {
    super(x, y, 10);
    this.side = side;
    if (side != FRIENDLY) {
      super.setSpeed(5);
    }

    s = createShape();
    s.beginShape();
    s.noStroke();
    s.fill(255);
    s.vertex(-5, 0);
    s.vertex(5, 0);
    s.vertex(5, 20);
    s.vertex(-5, 20);
    s.endShape(CLOSE);
  }

  @Override
  public void update() {
    //If this is a friendly projectile...
      if (side == FRIENDLY) {
        //...make it go up...
        super.incYBySpeed(true);
        //...and check if it's coliding with enemies.
        if (collidingWith(true, this)) {
          //if it collides remove it...still should be synchronized since it's called from draw.
          friendlyShots.remove(this);
        }
      }
      else {
        super.incYBySpeed(false);
      }
  }

  @Override
  public PShape getShape() {
    return s;
  }
}

And if you need it, Player:

public class Player extends Entity {

  public Player(float x, float y) {
    super(x, y, 5);
    s = createShape();
    s.beginShape();
    s.fill(255);
    s.noStroke();
    s.vertex(-25, 50);
    s.vertex(25, 50);
    s.vertex(0, 0);
    s.endShape(CLOSE);
  }

  @Override
  public void update(){
    switch(movement) {
      case Entity.EAST:
        incXBySpeed(false);
        break;
      case Entity.WEST:
        incXBySpeed(true);
        break;
      case Entity.STOP:
        break;
      default:
        println("The player shouldn't be moving any other direction than east or west!!! What's going on?");
    }
  }

  @Override
  public PShape getShape() {
    return s;
  }

  public void goRight() {
    movement = Entity.EAST;
  }

  public void goLeft() {
    movement = Entity.WEST;
  }

  public void stop() {
    movement = Entity.STOP;
  }
}

And finally, the stack trace:

java.lang.RuntimeException: java.util.ConcurrentModificationException
    at com.jogamp.common.util.awt.AWTEDTExecutor.invoke(AWTEDTExecutor.java:58)
    at jogamp.opengl.awt.AWTThreadingPlugin.invokeOnOpenGLThread(AWTThreadingPlugin.java:103)
    at jogamp.opengl.ThreadingImpl.invokeOnOpenGLThread(ThreadingImpl.java:206)
    at javax.media.opengl.Threading.invokeOnOpenGLThread(Threading.java:172)
    at javax.media.opengl.Threading.invoke(Threading.java:191)
    at javax.media.opengl.awt.GLCanvas.display(GLCanvas.java:541)
    at processing.opengl.PJOGL.requestDraw(PJOGL.java:688)
    at processing.opengl.PGraphicsOpenGL.requestDraw(PGraphicsOpenGL.java:1651)
    at processing.core.PApplet.run(PApplet.java:2256)
    at java.lang.Thread.run(Unknown Source)
Caused by: java.util.ConcurrentModificationException
    at java.util.ArrayList$Itr.checkForComodification(Unknown Source)
    at java.util.ArrayList$Itr.next(Unknown Source)
    at SpaceInvaders.collidingWith(SpaceInvaders.java:105)
    at SpaceInvaders$Projectile.update(SpaceInvaders.java:325)
    at SpaceInvaders.draw(SpaceInvaders.java:52)
    at processing.core.PApplet.handleDraw(PApplet.java:2386)
    at processing.opengl.PJOGL$PGLListener.display(PJOGL.java:862)
    at jogamp.opengl.GLDrawableHelper.displayImpl(GLDrawableHelper.java:665)
    at jogamp.opengl.GLDrawableHelper.display(GLDrawableHelper.java:649)
    at javax.media.opengl.awt.GLCanvas$10.run(GLCanvas.java:1289)
    at jogamp.opengl.GLDrawableHelper.invokeGLImpl(GLDrawableHelper.java:1119)
    at jogamp.opengl.GLDrawableHelper.invokeGL(GLDrawableHelper.java:994)
    at javax.media.opengl.awt.GLCanvas$11.run(GLCanvas.java:1300)
    at java.awt.event.InvocationEvent.dispatch(Unknown Source)
    at java.awt.EventQueue.dispatchEventImpl(Unknown Source)
    at java.awt.EventQueue.access$200(Unknown Source)
    at java.awt.EventQueue$3.run(Unknown Source)
    at java.awt.EventQueue$3.run(Unknown Source)
    at java.security.AccessController.doPrivileged(Native Method)
    at java.security.ProtectionDomain$1.doIntersectionPrivilege(Unknown Source)
    at java.awt.EventQueue.dispatchEvent(Unknown Source)
    at java.awt.EventDispatchThread.pumpOneEventForFilters(Unknown Source)
    at java.awt.EventDispatchThread.pumpEventsForFilter(Unknown Source)
    at java.awt.EventDispatchThread.pumpEventsForHierarchy(Unknown Source)
    at java.awt.EventDispatchThread.pumpEvents(Unknown Source)
    at java.awt.EventDispatchThread.pumpEvents(Unknown Source)
    at java.awt.EventDispatchThread.run(Unknown Source)

If there's anything else needed or the formatting is wrong (I'm new here) I can edit my post, thanks for any help!

GoToLoop

I didn't dig your code that much, but I fail to see any Thread in it! :-?
Thence you should remove all that synchronized cruft! It only makes everything slower! /:)

ConcurrentModificationException has a # of diff. reasons to happen:
http://docs.oracle.com/javase/8/docs/api/java/util/ConcurrentModificationException.html

But the most common 1 is use remove() while traversing a List as you do in #93: enemies.remove(e.getID());
http://docs.oracle.com/javase/8/docs/api/java/util/List.html#remove-int-

Look for backwards loop tag for remove() techniques:
http://forum.processing.org/two/discussions/tagged/backwards

P.S.: You should use generics in order to specify which datatype reference a Collection gonna hold:

import java.util.List;

List<Enemy> enemies = new ArrayList();
List<Projectile> friendlyShots = new ArrayList();

This way, you avoid those (cast) boilerplates in your code! :>

GoToLoop

If there's anything else needed or the formatting is wrong...

Well, you can insert a space between @ and Override. ;;)
And for future posts, separate "http://" + "Processing.org" for the same buggy reason!

Drountian

Thanks for your help, I know it's rough looking through bad code. I changed the removal to a marking system and that worked, thought ConcurrentModificationExceptions were only triggered by modification from more than one thread! Learn something every day! :)>-