Adding and removing elements to a Collection

Question

I'm new to Java and I'm currently making this game where the player has to eat some cookies. These cookies are elements of an ArrayList. This ArrayList is modified by two threads : -one that iterates over it and removes the cookies that have been eaten, using Iterator.remove() -one that adds a cookie to the ArrayList every 5 seconds

Sometimes I get a ConcurrentModificationException, and I know it's because the behavior of Iterator.remove() is "unspecified if the underlying collection is modified in any other way while the iteration is in progress", as stated in the Java Tutorial from Sun. How should I proceed ?

EDIT : updated code with

List<Cupcake> cake = Collections.synchronizedList(new ArrayList<Cupcake>());

Here's the Spawner :

public class CupcakeSpawner extends Thread {
    private Background back;

    public CupcakeSpawner(Background back) {
        this.back = back;
    }

    public void run() {
        while(true) {
            if(back.getCake().size() < 15)
                back.getCake().add(new Cupcake());
            try {
                Thread.sleep(5000);
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        }
    }
}

My update method :

public void update() {
    List<Cupcake> cake = back.getCake();
    Iterator<Cupcake> itrC = cake.iterator();
    while(itrC.hasNext()) {
        Cupcake cupcake = (Cupcake)(itrC.next());
        checkCollisionCup(cupcake);
        if(cupcake.isEaten()) 
                itrC.remove();
        }
    }
}

you don't need to cast with Iterator#next: Cupcake cupcake = itrC.next(); — manzur, May 31 '15 at 11:54

Bohemian · Answer 1 · 2015-05-31T11:30:45.687

1

ArrayList is not a threadsafe implementation of List.

Use a CopyOnWriteArrayList instead - it will not cause an explosion if you add elements while iterating.

edited May 31 '15 at 11:30

answered May 31 '15 at 11:24

Bohemian

412,405
93
575
722

I think in the context of question using CopOnWriteArrayList can show already eaten cakes – manzur May 31 '15 at 11:46

Aalkhodiry · Answer 2 · 2015-05-31T14:32:43.613

0

As @Bohemian stated that ArrayList are not thread-safe.

It's better to use:

List list = Collections.synchronizedList(new ArrayList());

then you want to call Iterator.remove()

synchronized (list) {
  Iterator.remove() 
}

Hope this will help you.

See this

public class CupcakeSpawner extends Thread {
private Background back;

public CupcakeSpawner(Background back) {
    this.back = back;
}

public void run() {
    List cakes = back.getCake();
    while(true) {
        if(cakes.size() < 15){
            synchronized (cakes) {
                 cakes.add(new Cupcake());
            }
        }
        try {
            Thread.sleep(5000);
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
      }
   }
}

edited May 31 '15 at 14:32

answered May 31 '15 at 11:41

Aalkhodiry

2,178
30
25

What's the reason of using Collections#synchronizedList if you embed your call in synchronized block? – manzur May 31 '15 at 11:44
I tried to update my code with what you suggested. Is it correct ? – Quentin May 31 '15 at 12:15
@manzur ```List list = Collections.synchronizedList(new ArrayList());``` Returns a synchronized (thread-safe) list backed by the specified list. – Aalkhodiry May 31 '15 at 12:47
thank you for your answer. So I don't need to write the remove instruction in a synchronized block ? – Quentin May 31 '15 at 12:58
I still get the exception, but maybe the way I am testing my code is wrong. Since the exception occurred only once in a while, the only way I found to test the fix was to spawn one cupcake every 5 ms, with a limit size of 10 000 – Quentin May 31 '15 at 13:09
@Quentin you need to add ```synchronized (list) {}``` block for adding to the list also. The exception happens when 2 threads are trying to add elements at the same time – Aalkhodiry May 31 '15 at 14:26
@Aalkhodiry you don't need to call Collections#synchronizedList if you're using synchronized block. What Collections#synchronizedList does it creates a wrapper List which uses synchronized calls to the original list. So preferably you choose only one of these. – manzur May 31 '15 at 14:58
@manzur I don't know where he added ```Collections.synchronizedList()``` so the ```synchronized (list) { }``` is another solution – Aalkhodiry May 31 '15 at 15:45

score 0 · Answer 3 · answered May 31 '15 at 14:05

One mustn't use an Iterator of an ArrayList if the underlying array has changed:

public static void main(String[] args) {
    List<Object> l = new ArrayList<>();
    Iterator<Object> i = l.iterator();
    l.add(new Object());
    try {
        System.out.println("I've added " + i.next());
    } catch (ConcurrentModificationException e) {
        System.out.println("Nooo!!!");
    }
}

This is true for threads too of course.

So in your case you are checking for eaten cupcakes (to remove them) while also adding new ones to the same collection. What you are experiencing is a so called race condition.

One way to solve this is to use a CopyOnWriteArrayList as suggested by @Bohemian but I wouldn't recommend it because each modification cause the creation of a new array and this is a performance overhead (and memory issue if "old" arrays are still being referenced, e.g. due to Iterator instances).

Another way to solve this is to use a counting for-loop in you update method.

public void update() {
    List<Cupcake> cake = back.getCake();
    for (int i = 0; i < cake.size(); ++i) {
        Cupcake cupcake = cake.get(i);
        checkCollisionCup(cupcake);
        if (cupcake.isEaten()) 
            cake.remove(i);
    }
}

Important: With this counting for-loop you should use a synchronized list as suggested by @Aalkhodiry.

Fwiw: Yet there's more to threading than this...

Adding and removing elements to a Collection

3 Answers3