CODEWITHSUNDEEP
javaandroidconcurrentmodification
Ollie C
Ollie C

Reputation: 28509

Removing collection items without a ConcurrentModificationException using nested Iterators

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;
}

Upvotes: 0

Views: 712

Answers (2)

wolfcastle
wolfcastle

Reputation: 5930

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;
}

Upvotes: 1

Blackbelt
Blackbelt

Reputation: 157447

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.

Upvotes: 1

Related Questions