Iterate over collection in multithread environment: ConcurrentModificationException

Question

I'm getting a ConcurrentModificationException since this code is reached by several threads at the same time:

public void flush(Audit... audits) {
   // Copy first them on memory
   this.pendingAudits.addAll(Arrays.asList(audits));

   for (Iterator<Audit> it = this.pendingAudits.iterator(); it.hasNext();) {
       // Do something
       it.remove();
   }
}

I'm getting an iterator over pendingAudits and I'm removing each element at the same time other threads can add some other audits.

How to solve it elegantly?

Sounds like you use a list like a queue - have you looked into using an actual queue data structure instead? Perhaps [`ArrayBlockingQueue`](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/concurrent/ArrayBlockingQueue.html) or [`LinkedBlockingDeque`](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/concurrent/LinkedBlockingDeque.html) would be better suited for you use case. — Hulk, Dec 10 '18 at 10:35
You might want to consider using `Collections.synchronizedList()`: https://stackoverflow.com/questions/11360401/java-synchronized-list — Mushif Ali Nawaz, Dec 10 '18 at 10:36
@Hulk Yes, you are right, It's like an queue. Is there any specific queue-like structure which support iterator.remove at the same time of an add operation? — Jordi, Dec 10 '18 at 10:39
@HadiJ As far I know, `CopyOnWriteArrayList` doesn't support `iterator.remove`. — Jordi, Dec 10 '18 at 10:41
@Jordi well, typically queues are designed to allow concurrent removal on one end and adding at the other end. See [Queue](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/Queue.html). i.e. multiple threads can `offer` and/or `add` elements while multiple other threads `peek()` or `remove()` elements. There is a whole family of queues with lots of specialized variants for all kinds of use cases. — Hulk, Dec 10 '18 at 10:44
How about this `CopyOnWriteArrayList list = new CopyOnWriteArrayList<>(this.pendingAudits);` and `for (Audit audit:list){ list.remove(audit); }` — Hadi J, Dec 10 '18 at 10:48
@HadiJ Could you provide an slightly helping code in an answer, please? It could help me to figure it out... — Jordi, Dec 10 '18 at 11:09
@Jordi see my previous comment. `CopyOnWriteArrayList list = new CopyOnWriteArrayList<>(Arrays.asList(audits));` and `for (Audit audit:list){ //some condition list.remove(audit); }` — Hadi J, Dec 10 '18 at 11:17
@Hulk Should I need to create a `new CopyOnWriteArrayList` each time `flush method` is reached? Right now, `pendingAudits` is a class property... — Jordi, Dec 11 '18 at 08:14
@Jordi I never suggested a `CopyOnWriteArrayList`. But no, that doesn't sound right. I still haven't really understood what you are trying to achieve. What is the "Do something" - part? Storing or sending the data somewhere? I still feel that it might be better to redesign you workflow. It feels strange that there are multiple threads that concurrently fill some list/queue but also do the "something" immediately, removing items from the list. The purpose of such constructs usually is to do the filling in some producer-threads, and the "something" in other, dedicated consumer-threads. — Hulk, Dec 11 '18 at 08:34

score 2 · Answer 1 · answered Dec 10 '18 at 10:33

A quick solution is to wrap every access to this.pendingAudits in synchronized:

sychronized(this.pendingAudits) {
   this.pendingAudits.addAll(Arrays.asList(audits));

   for (Iterator<Audit> it = this.pendingAudits.iterator(); it.hasNext();) {
       // Do something
       it.remove();
   }
}

The net effect will be that at any given time only one thread can be executing the synchronized block. That can become a bottleneck if the code is executed frequently.

Iterate over collection in multithread environment: ConcurrentModificationException

1 Answers1