Programming Languages Bug Report

Java ConcurrentModificationException removing from an ArrayList in for-each

Our webhook dispatcher threw ConcurrentModificationException a few times a day and silently skipped deliveries the rest of the time. Two bugs shared one list: removal inside a for-each loop and an admin thread editing it concurrently.

Daniel Tran Daniel Tran @fun_dev · May 13, 2025 · 8 min read · 5 Views
Banner: a Java list being edited by one hand while another hand iterates through it

Our platform lets customers register webhook endpoints. A singleton WebhookDispatcher in a Spring Boot service picks up events every second and posts them to each subscriber. Two symptoms showed up in the same week. First, a handful of errors per day in the logs:

2025-05-06T09:14:02.381Z ERROR 1 --- [dispatch-1] c.e.webhooks.WebhookDispatcher : dispatch failed
java.util.ConcurrentModificationException: null
	at java.base/java.util.ArrayList$Itr.checkForComodification(ArrayList.java:1095)
	at java.base/java.util.ArrayList$Itr.next(ArrayList.java:1049)
	at com.example.webhooks.WebhookDispatcher.dispatch(WebhookDispatcher.java:58)
	at com.example.webhooks.WebhookDispatcher.tick(WebhookDispatcher.java:41)

Second, and more worrying, two customers reported missing events even on days with no exceptions in the logs. Delivery records showed that their endpoints had simply never been called for certain events.

Reproducing it

The dispatch loop removed subscribers whose endpoint answered 410 Gone, which is how customers signal that an endpoint was retired:

for (Subscription sub : subscribers) {
    HttpStatusCode status = client.post(sub.url(), event);
    if (status.value() == 410) {
        subscribers.remove(sub);
    }
}

In a unit test with five subscribers, making the second one return 410 threw the exception every time. Making the fourth one, the second-to-last, return 410 threw nothing at all, and the fifth subscriber was never called. That was the silent skip.

What we ruled out

  • The HTTP client mutating the list. It has no reference to it.
  • Spring creating multiple dispatcher instances. It is a singleton bean; we confirmed with a heap dump that there was one instance and one list.
  • Retries masking the missed deliveries. Retries only apply to failed calls. Skipped subscribers had no call at all, so nothing was retried.

Root cause

There were two bugs on the same ArrayList. Inside the loop, subscribers.remove(sub) modified the list behind the back of the for-each iterator. Depending on the position of the removed element, the iterator either detected it on the next next() call and threw, or the loop ended early without noticing, skipping the remaining element. Separately, the admin API (on a Tomcat request thread) called subscribers.remove(...) when a customer deleted an endpoint, while the dispatcher thread was iterating. That cross-thread modification explains the exceptions on days when no endpoint returned 410, and it is a data race on a non-thread-safe collection.

What is happening under the hood

A for-each loop over a List is compiled to an Iterator. ArrayList keeps a counter called modCount that increases on every structural change (add, remove, clear). The iterator records the value when it is created and compares it in next(). If they differ, it throws ConcurrentModificationException. The name is misleading: no second thread is needed.

The silent skip comes from hasNext(), which only checks cursor != size. Suppose the list has five elements and we remove the fourth while the cursor points past it, at index 4. The size drops to 4, hasNext() returns false, the loop ends, and next() is never called, so the modification check never runs. The fifth element shifted into index 3 and was never visited.

With two threads, things are worse. ArrayList is not thread-safe. The Javadoc describes the fail-fast behaviour as best-effort: the iterator may throw, may skip or repeat elements, or may see an inconsistent array. An exception is the lucky outcome.

Environment

Component Version / value
OS Ubuntu 24.04 LTS container
Runtime Eclipse Temurin 21.0.7
Framework Spring Boot 3.4.5
Scheduler @Scheduled(fixedDelay = 1000) on a dedicated thread pool
Collection java.util.ArrayList<Subscription> field in a singleton bean
Subscribers about 2,300 active endpoints

Minimal reproduction

import java.util.ArrayList;
import java.util.List;

public class SkipDemo {
    public static void main(String[] args) {
        List<String> subs = new ArrayList<>(List.of("a", "b", "c", "d", "e"));
        List<String> called = new ArrayList<>();
        for (String s : subs) {
            called.add(s);
            if (s.equals("d")) subs.remove(s); // second-to-last: no exception
        }
        System.out.println(called); // [a, b, c, d]  -> "e" never visited
    }
}

Change "d" to "b" and it throws ConcurrentModificationException instead.

Who is affected

All Java versions and every fail-fast collection in java.util: ArrayList, LinkedList, HashMap and its views, HashSet, TreeMap. The same pattern exists in Kotlin with MutableList and in C# as InvalidOperationException: Collection was modified.

The fix

We split the problem in two. Removal during iteration is a single-thread concern; shared access between the dispatcher and the admin API is a concurrency concern.

For the dispatcher, we stopped removing inside the loop. It now collects gone endpoints and removes them once, after the iteration:

-for (Subscription sub : subscribers) {
-    HttpStatusCode status = client.post(sub.url(), event);
-    if (status.value() == 410) {
-        subscribers.remove(sub);
-    }
-}
+List<Subscription> gone = new ArrayList<>();
+for (Subscription sub : subscribers) {
+    HttpStatusCode status = client.post(sub.url(), event);
+    if (status.value() == 410) {
+        gone.add(sub);
+    }
+}
+subscribers.removeAll(gone);

For the shared list, we chose CopyOnWriteArrayList. Subscribers change a few times per hour and are read thousands of times per minute, which is the workload it is designed for. Every write copies the backing array; every iterator works on an immutable snapshot and never throws:

-private final List<Subscription> subscribers = new ArrayList<>();
+private final List<Subscription> subscribers = new CopyOnWriteArrayList<>();

With a snapshot iterator, the admin API can remove an endpoint mid-dispatch safely. The dispatch in progress may still post to it once, which we accept and document: deletion takes effect from the next tick.

Alternatives and trade-offs

  1. Iterator.remove(). The only safe way to remove the current element while iterating a regular ArrayList explicitly. Note that CopyOnWriteArrayList iterators do not support it and throw UnsupportedOperationException.

    for (Iterator<Subscription> it = subs.iterator(); it.hasNext(); ) {
        if (isGone(it.next())) it.remove();
    }
    
  2. removeIf. The cleanest option when the condition does not have side effects, such as removing expired entries: subs.removeIf(Subscription::isExpired). We did not use it here because our condition performs an HTTP call, and side effects inside a predicate are hard to reason about.

  3. Collections.synchronizedList. Makes individual calls thread-safe, but you must still wrap the entire iteration in synchronized (list) { ... }, which blocks the admin API for the whole dispatch, including slow HTTP calls. Not a good fit here.

  4. An immutable snapshot in an AtomicReference. Readers call get() and iterate an immutable List.copyOf(...); writers use updateAndGet to build a new list. Same properties as CopyOnWriteArrayList, more explicit, and easy to extend to a map keyed by tenant.

  5. ConcurrentHashMap.newKeySet(). Weakly consistent iterators that tolerate concurrent changes without copying. Better for large, frequently written sets where order does not matter.

How to test the fix

A deterministic test for each position covers the silent-skip case:

@ParameterizedTest
@ValueSource(ints = {0, 1, 2, 3, 4})
void everySubscriberIsCalledWhenOneIsGone(int goneIndex) {
    var subs = fixtures(5);
    var client = new FakeClient().respond410For(subs.get(goneIndex).url());
    var dispatcher = new WebhookDispatcher(client, subs);

    dispatcher.dispatch(sampleEvent());

    assertThat(client.calledUrls()).hasSize(5);
    assertThat(dispatcher.subscribers()).hasSize(4);
}

And a stress test that removes subscribers from another thread while dispatching thousands of times; it failed within seconds on the old code:

./mvnw -Dtest='WebhookDispatcher*Test' test

Confirming in production

We added a counter of subscribers attempted per event and compared it to the number of active subscribers at the start of the tick. In the week before the fix, 0.4% of events had fewer attempts than subscribers. In the three weeks after, the gap was zero across roughly 41 million deliveries, and ConcurrentModificationException disappeared from the logs. We re-sent the missed events to the two customers using the delivery records.

Regression guard

  • The parameterised and stress tests run in CI.
  • Error Prone is configured in our build, and its ModifyCollectionInEnhancedForLoop check flags calls like list.remove(x) inside a for-each over the same list.
  • A code review note in our contributing guide: any mutable collection field in a singleton bean must be a concurrent collection or be explicitly guarded.

Security implications

Skipped deliveries are a reliability problem, but webhooks often drive security-relevant actions, such as revoking access when a subscription is cancelled. A silently missed subscription.cancelled event can leave access in place. With concurrent mutation of a non-thread-safe list, the dispatcher could in theory also deliver an event to the wrong subscriber if the backing array was observed mid-update. We found no evidence of that in the delivery records, but it is the reason we did not treat this as a mere logging issue.

Prevention checklist

  • Never call list.remove or list.add inside a for-each over the same list.
  • Use Iterator.remove, removeIf, or collect-then-remove for single-thread removal.
  • Pick a concurrent collection for any field shared between threads, based on the read/write ratio.
  • Test removal at every position, including second-to-last, to catch silent skips.
  • Enable Error Prone or an equivalent static check in the build.

References

Add a label

#concurrency #java #collections #spring-boot #arraylist
Written by
Daniel Tran @fun_dev

I have spent most of my career on the side of software that pages people at night: payment integrations, web platforms with real traffic, CI/CD pipelines, Kubernetes clusters and the databases underneath them. Somewhere along the way I started keeping a notebook of every bug that cost more than an hour — the exact error, what we ruled out, the real root cause and how we proved the fix. Hibug is where those notes live. Every post follows the same path: Investigate, Explain, Fix, Verify — with the environment spelled out, the fix tested, and the security angle checked.

Advertisement

Comments

0

No comments yet. Be the first to comment!

Sign In to post a comment.