StringUtils concurrency exception

Question

I am getting the following error:

Caused by: java.util.ConcurrentModificationException
    at java.util.ArrayList$Itr.checkForComodification(ArrayList.java:901)
    at java.util.ArrayList$Itr.next(ArrayList.java:851)
    at org.apache.commons.lang3.StringUtils.join(StringUtils.java:3428)
    at org.apache.commons.lang3.StringUtils.join(StringUtils.java:3513)

For running StringUtils.join

The StringUtils documentation mentions: #ThreadSafe#

What is wrong? The code is located in a java class that implements Callable.

My full code:

public class MyApiCallable implements Callable<ResponseType> {

    final List<String> itemsId;

    MyApiCallable(List<String> itemsId) {
        this.itemsId = itemsId;
    }

    @Override
    public ResponseType call() throws Exception {
        Client client = ClientBuilder.newClient();
        WebTarget baseTarget = client.target("http://whatever.com").path("path").queryParam("ItemID", StringUtils.join(itemsId,",",));
        ResponseType rs = target.request().get(ResponseType.class);
        return rs;
    }
}

and here is the method that calls the callable:

private Future<ResponseType> addGetMultipleItems(ExecutorService executor, List<String> itemIds) {
        Callable<ResponseType> shoppingCallable = new MyApiCallable(itemIds);
        Future<ResponseType> shoppingResult = executor.submit(shoppingCallable);
        return shoppingResult;
    }

You are trying to add/remove from a collection through an interator. You aren't allowed to do that. Keep in mind that enhanced for loops use an iterator under the hood — Vince, Apr 07 '15 at 20:16
Threadsafe means that the function cannot break anything by itself, but it's still possible for it to manifest breakage happening elsewhere. You are modifying the passed-in list in another thread. — Karol S, Apr 07 '15 at 20:17
why not actually include the code in question in the question? — eis, Apr 07 '15 at 20:17
@VinceEmigh it's not me- it's a method that belongs to StringUtils — Dejell, Apr 07 '15 at 20:18
This may not be safe if the list is actually being concurrently modified, even though it's annotated thread-safe... — Louis Wasserman, Apr 07 '15 at 20:19
@Dejel the problem doesn't seem to be a method of StringUtils, but what you pass to it - but please, show us the code in question so we can see. — eis, Apr 07 '15 at 20:20
Some other thread modifies the `itemsId`list while `StringUtils.join()` is iterating on it. `StringUtils.join()` is thread-safe. But the list you pass to the method is not. — JB Nizet, Apr 07 '15 at 20:24
do u mean that I need to clone the itemIds for this.itemsId = itemsId;? — Dejell, Apr 07 '15 at 20:25
Cloning the list would do the same thing as StringUtils.join() does: it would iterate on the list to copy all its elements to a new list. You need to use a thread-safe list, or to synchronize all the accesses to the list properly, or to avoid sharing this list between threads, or avoid to modify it once it has been constructed and passed to threads. The design problem is in code you don't show. — JB Nizet, Apr 07 '15 at 20:27
Try cloning the collection in the constructor: Instead of `this.itemsId = itemsId;` use `this.itemsId = new ArrayList<>(itemsId);`. You'll see the problem disappears because some thread modifies your **itemsId** collection. — Robert, Apr 07 '15 at 20:30
@JBNizet is there a way inside MyApiCallable to make it not changed? — Dejell, Apr 07 '15 at 20:30
MyApiCallable only iterates through the list. It doesn't modify it. Some other code, executed by another thread, modifies it. — JB Nizet, Apr 07 '15 at 20:33
@JBNizet correct - however will new ArrayList(itemIds) solve it? — Dejell, Apr 07 '15 at 20:41
Creating a new list will solve the problem. Also please consider Guava's [`ImmutableList`](https://code.google.com/p/guava-libraries/wiki/ImmutableCollectionsExplained), which gives thread safety through immutability. — Giovanni Botta, Apr 07 '15 at 20:45
@RobertDanci can you pls write as an answer and I will accept it? — Dejell, Apr 07 '15 at 20:47
Creating a new list can solve the problem **if** the copy of the list is done in the same thread than the one modifying the list and causing the exception to be thrown. — JB Nizet, Apr 07 '15 at 21:03
Copying the list in the constructor won't necessariily fix the problem, since the list could be modified by another thread when it is being copied in the constructor. — fps, Apr 07 '15 at 21:30
@JBNizet Thanks - it's a callable class - a new thread that is opened just for it. I guess that it would help no? as that thread cannot modify the list — Dejell, Apr 07 '15 at 21:39
@Dejel JB Nizet is correct, usually _cloning_ happens by looping over the elements. Luckily cloning an ArrayList from an ArrayList copies the backing array so it won't throw a ConcurrentModificationException. But Magnamag is right, some thread could modify the list while you are constructing MyApiCallable so you can get unpredictable results. The solution would be to clone the list before **addGetMultipleItems** gets called or synchronize it. — Robert, Apr 09 '15 at 10:21

StringUtils concurrency exception

0 Answers0