Removing collection items without a ConcurrentModificationException using nested Iterators

Question

I'm enduring this exception and after reading around I understand you can usually deal with it by using an Iterator. But having tried that I find it doesn't work in this scenario, perhaps because I have two nested loops? All the code is running on the main thread (Android).

 private boolean removeRoutesWithTimestampFromTeams(long time) {
    boolean routesRemoved = false;
    Iterator<WhizzyTeam> teamIterator = assignedTeams.iterator();
    while (teamIterator.hasNext()) {
        WhizzyTeam team = teamIterator.next();  // EXCEPTION HERE
        Iterator<Route> routeIterator = team.routes.iterator();
        while (teamIterator.hasNext()) {
            Route route = routeIterator.next();
            if (route.whizzy_assignment_time_in_millis == time) {
                team.removeRoute(route);
                routesRemoved = true;
            }
        }
        if (team.routes == null || team.routes.size() == 0) {
            assignedTeams.remove(team);
            unassignedTeams.add(team);
        }
    }
    return routesRemoved;

I'm relying on object references to identify objects, so copying collections is a very unattractive option. Is there any way to do what I need to do without resorting to copies?

===== WORKING CODE =====

I actually made three mistakes that are fixed here. 1. I used the same iterator in both loops (fixed in the code above too) 2. I should have used Iterator.remove() 3. Within removeRoute() there was another (unnecessary) for loop looping through routes to delete similar siblings

private boolean removeRoutesWithTimestampFromTeams(long time) {
    boolean routesRemoved = false;
    Iterator<WhizzyTeam> teamIterator = assignedTeams.iterator();
    while (teamIterator.hasNext()) {
        WhizzyTeam team = teamIterator.next();
        Iterator<Route> routeIterator = team.routes.iterator();
        while (routeIterator.hasNext()) {
            Route route = routeIterator.next();
            if (route.whizzy_assignment_time_in_millis == time) {
                routeIterator.remove();
                routesRemoved = true;
            }
        }
        if (team.routes == null || team.routes.size() == 0) {
            teamIterator.remove();
            unassignedTeams.add(team);
        }
    }
    return routesRemoved;
}

you are using two times teamIterator, in the first and in the second loop, and you should use the iterator itself to remove the object from the collection — Blackbelt
– Blackbelt, Commented Dec 19, 2014 at 17:05
@Blackbelt You are dead right there. I changed the inner loop to use the correct iterator, but I am still seeing the same exception, but in the first call to next(). — Ollie C
– Ollie C, Commented Dec 19, 2014 at 17:12

Blackbelt · Accepted Answer · 2014-12-19 17:33:29Z

I see two issues with the snippet you posted. The first one you are using the outer iterator also int inner loop

  while (teamIterator.hasNext()) {
        WhizzyTeam team = teamIterator.next();  // EXCEPTION HERE
        Iterator<Route> routeIterator = team.routes.iterator();
        while (teamIterator.hasNext()) {

I do think you should use routeIterator, in place of teamIterator in the second loop.

Second you are not using the remove method from the Iterator interface to remove the item from your collection. From the doc

The iterators returned by this class's iterator and listIterator methods are fail-fast: if the list is structurally modified at any time after the iterator is created, in any way except through the iterator's own remove or add methods, the iterator will throw a ConcurrentModificationException. Thus, in the face of concurrent modification, the iterator fails quickly and cleanly, rather than risking arbitrary, non-deterministic behavior at an undetermined time in the future.

wolfcastle · Accepted Answer · 2014-12-19 17:51:58Z

As you can tell, using nested iterators can lead to mixups in using the wrong one, etc. Another approach to doing this without iterators is to keep track of the ones you want to remove while iterating, then remove them after. This approach does require temporary collections, but allows you to use the foreach loop and prevent errors with using the wrong iterators.

private boolean removeRoutesWithTimestampFromTeams(long time) {
    boolean routesRemoved = false;
    List<WhizzyTeam> teamsToUnassign = new ArrayList<>();
    for( WhizzyTeam team : assignedTeams ) {
        List<Route> routesToRemove = new ArrayList<>();
        for( Route route : team.routes ) {
            if (route.whizzy_assignment_time_in_millis == time) {
                routesToRemove.add( route );
            }
        }
        if( routesToRemove.size() > 0 ) {
            team.routes.removeAll( routesToRemove );
            if( team.routes.isEmpty() ) {
                teamsToUnassign.add( team );
            }
            routesRemoved = true;
        }
    }
    assignedTeams.removeAll( teamsToUnassign );
    unassignedTeams.addAll( teamsToUnassign );

    return routesRemoved;
}

Collectives™ on Stack Overflow

Removing collection items without a ConcurrentModificationException using nested Iterators

2 Answers 2

Comments

Comments

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

Comments

Related