CODEWITHSUNDEEP
javaconcurrentmodification
Harlizonian
Harlizonian

Reputation: 61

I can't find the cause of my java.util.ConcurrentModificationException

I have code that enters the for loop within my Main method.

for (List<Point2D> points : output) {
    currentPath = pathDistance(points);
    if (shortest == 0){
        shortest = currentPath;
    } else if (currentPath < shortest) {
        best = points;
        shortest = currentPath;
    }
}

where pathDistance is defined as

public static Double pathDistance(List<Point2D> path){
    double distance = 0;
    int count = path.size()-1;

    for (int i = 0; i < count; i++) {
        distance = distance + path.get(i).distance(path.get(i+1));
    }

    distance = distance + path.get(0).distance(path.get(count));
    return distance;
}

But I keep getting the error

Exception in thread "main" java.util.ConcurrentModificationException
   at java.util.SubList.checkForComodification(Unknown Source)
   at java.util.SubList.size(Unknown Source)
   at java.util.Collections$SynchronizedCollection.size(Unknown Source)
   at TSMain.pathDistance(TSMain.java:76)
   at TSMain.main(TSMain.java:203)

I know this is supposed to mean that I am altering an object while an iteration depends on it, but I can't for the life of me figure out where that might be happening. Any help would be appreciated.

Upvotes: 6

Views: 863

Answers (1)

vsminkov
vsminkov

Reputation: 11270

Your stacktrace shows that somewhere in your code subList is passed to Collections.synchronizedCollection (directly or indirectly). Like this

Set<List<Point2D>> output = Collections.singleton(
    Collections.synchronizedCollection(data.subList(start, end)));

However it does not copy data list. And points subList is still pointing to a range in data list. And original list is modified at the momet path.size() call occurs.

You can easily fix your problem by doing explicit list copy before passing it to pathDistance

for(List<Point2D> points : output){
    List<Point2D> pointsCopy = new ArrayList<>(points)
    currentPath = pathDistance(pointsCopy);
    // rest of code using pointsCopy
}

I also should notice that it looks like there is a problem with synchronization in your code. Wrapping sublists in synchronized collection is a bad idea because original list could be modified in unsafe manner w/o proper synchronization.

You can learn more about list modification checking by looking into AbstractList#modCount sources.

Upvotes: 2

Related Questions