Ad

Concurrent Modification Exception In Callable Class

- 1 answer

I'm trying to split a list of objects within smaller sublist and to process them separately on different threads. So I have following code:

        List<Instance> instances = xmlInstance.readInstancesFromXml();
        List<Future<List<Instance>>> futureList = new ArrayList<>();

        int nThreads = 4;

        ExecutorService executor = Executors.newFixedThreadPool(nThreads);

        final List<List<Instance>> instancesPerThread = split(instances, nThreads);

        for (List<Instance> instancesThread : instancesPerThread) {
            if (instancesThread.isEmpty()) {
                break;
            }

            Callable<List<Instance>> callable = new MyCallable(instancesThread);
            Future<List<Instance>> submit = executor.submit(callable);
            futureList.add(submit);
        }

        instances.clear();

        for (Future<List<Instance>> future : futureList) {
            try {
                final List<Instance> instancesFromFuture = future.get();
                instances.addAll(instancesFromFuture);
            } catch (InterruptedException | ExecutionException e) {
                e.printStackTrace();
            }
        }

        executor.shutdown();

        try {
            executor.awaitTermination(Long.MAX_VALUE, TimeUnit.NANOSECONDS);
        } catch (InterruptedException ie) {
            ie.printStackTrace();
        }

And the MyCallable class :

public class MyCallable implements Callable<List<Instance>> {

    private List<Instance> instances;

    public MyCallable (List<Instance> instances) {
        this.instances = Collections.synchronizedList(instances);
    }


    @Override
    public List<Instance> call() throws Exception {

        for (Instance instance : instances) {
            //process each object and changing some fields;
        }

        return instances;
    }

}

Split method(It's split an given list in given number of list, also trying to have almost same size on each sublist) :

public static List<List<Instance>> split(List<Instance> list, int nrOfThreads) {
        List<List<Instance>> parts = new ArrayList<>();
        final int nrOfItems = list.size();
        int minItemsPerThread = nrOfItems / nrOfThreads;
        int maxItemsPerThread = minItemsPerThread + 1;
        int threadsWithMaxItems = nrOfItems - nrOfThreads * minItemsPerThread;
        int start = 0;
        for (int i = 0; i < nrOfThreads; i++) {
            int itemsCount = (i < threadsWithMaxItems ? maxItemsPerThread : minItemsPerThread);
            int end = start + itemsCount;
            parts.add(list.subList(start, end));
            start = end;
        }

        return parts;
    }

So, when I'm trying to execute it, I'm getting java.util.ConcurrentModificationException on this line for (Instance instance : instances) {, can somebody give any ideas why it's happening?

Ad

Answer

public MyCallable (List<Instance> instances) {
    this.instances = Collections.synchronizedList(instances);
}

Using synchronizedList like this doesn't help you in the way you think it might.

It's only useful to wrap a list in a synchronizedList at the time you create it (e.g. Collections.synchronizedList(new ArrayList<>()). Otherwise, the underlying list is directly accessible, and thus accessible in an unsynchronized way.

Additionally, synchronizedList only synchronizes for the duration of individual method calls, not for the whole time while you are iterating over it.

The easiest fix here is to take a copy of the list in the constructor:

    this.instances = new ArrayList<>(instances);

Then, nobody else has access to that list, so they can't change it while you are iterating it.

This is different to taking a copy of the list in the call method, because the copy is done in a single-threaded part of the code: no other thread can be modifying it while you are taking that copy, so you won't get the ConcurrentModificationException (you can get a CME in single-threaded code, but not using this copy constructor). Doing the copy in the call method means the list is iterated, in exactly the same way as with the for loop you already have.

Ad
source: stackoverflow.com
Ad