ConcurrentModificationException without modifying object

Question

I've got the following piece of code that is causing a ConcurrentModificationException. I'm not modifying the products object at all,

List<Product> products = Product.findActiveByFilter(filters);
Set<Long> temp = new HashSet<Long>();
List<Product> resultSetTemp = new ArrayList<Product>();
for (Product product : products) { // << Exception points to this
    if(!temp.contains(product.getId())){
        temp.add(product.getId());
        resultSetTemp.add(product);
    }
}
products.clear();
products.addAll(resultSetTemp);

I've seen this exception pop-up several times, but I cannot reproduce it (it happens randomly).

Product.findActiveByFilter is a method that returns a new instance of a List<Product> that has been build from a cached List<Product>.

Edit: I've found a way to reproduce the error. The code is called when a client wants the products (its a webshop), and the website loads more items when the client scrolls down. This triggers the exception (as the server is not-yet done responding with the products, and gets another call for it). Race conditions, fun!

@serejja Wouldn't that be weird? The `List products` is a local variable — Diamondo25, Mar 20 '14 at 10:29
P.S. why not use a `Map` to store your results rather than keeping two collections in sync? — Boris the Spider, Mar 20 '14 at 10:33
@chris No can do, it's a huge function with many checks (for every filter type it checks if it has already filtered something before, if so, filter the filtered. Else, filter the cached list) — Diamondo25, Mar 20 '14 at 10:38
@BoristheSpider I haven't made this code from scratch; it's from a different developer. Shouldn't create a conflict tho. — Diamondo25, Mar 20 '14 at 10:40
Okay. Reason I asked is, are you 100% sure it's returning a *copy* of the cached list and not the cached list itself? Because if it's the cached list itself, and your code gets run in two different places simultaneously (=concurrently), you will get the exception you're having — BadIdeaException, Mar 20 '14 at 10:43
Try to create a new List and work with it. `List tmpProducts = new ArrayList(products);` — Alexander_Winter, Mar 20 '14 at 10:44
@chris There could be a case of that; changed the code of `findActiveByFilter` to return a new instance of `List(products)` — Diamondo25, Mar 20 '14 at 10:58

score 1 · Accepted Answer · answered Mar 20 '14 at 11:07

As some said already, it was caused by a seperate tread 'modifying' products (as it was a cached instance). I've changed the implementation of Product.findActiveByFilter to return a new ArrayList<Product>(products); instead of a reference to the cached value, when no filters are applied (thus no filtered result is given back).

public static List<Product> findActiveByFilter(ArrayList<FilterPair> filters) {
    List<Product> products = getCachedAllProductsByFirstSupplier();

    if (products == null) {
        products = new ArrayList<Product>();
    }

    if(filters.size() > 0) {
        List<Product> productsFiltered = new ArrayList<Product>(products);
        // ... MANY checks here for the filters ...

        return productsFiltered;
    }

    return new ArrayList<Product>(products); // Do not give cached copy, was 'return products;'
}

There were 2 calls to the findActiveByFilter, called by the website. The first one did include a filter, but the second one did not (so the first one was still busy, while the second one returned directly).

Thanks for posting the answer back. Glad it worked! Seems like @serejja was right :) — Keerthivasan, Mar 20 '14 at 11:10

score 0 · Answer 2 · answered Mar 20 '14 at 11:04

Have you tried using an iterator?

List<Product> products = Product.findActiveByFilter(filters);
Set<Long> temp = new HashSet<Long>();
List<Product> resultSetTemp = new ArrayList<Product>();

Iterator ite = products.iterator();

while (ite.hasNext()) {

    Product product = ite.next();

    if(!temp.contains(product.getId())){

        temp.add(product.getId());
        resultSetTemp.add(product);
    }
}
products.clear();
products.addAll(resultSetTemp);

EDIT: Just saw your update. In light of that, this probably won't solve your problem.

I don't think that will resolve the issue. When we do foreach loop an iterator is used. — Hunsu, Mar 20 '14 at 11:06

ConcurrentModificationException without modifying object

2 Answers2