Java: How to remove elements from a list while iterating over/adding to it

Question

Hi!

This question is a more special case of the problem described (and solved) in this question.

I have two methods, stopAndRemove(ServerObject server) and a close() method. The later should close all servers and remove them from the server list. The list is defined as

List<ServerObject> server.

I do not want to have almost the same code from stopAndRemove in closeCurrentlyOpen, so I want to do something like:

public void closeCurrentlyOpen() {
   for(ServerObject server : this.servers) {
       stopAndRemove(server)
   }
}

This won't work, as this will cause a ConcurrentModificationException. I tried to make a copy of the list

List<ServerObject> copyList = new ArrayList<ServerObject>(this.servers);

and use that as the list for the foreach-loop. But then it might be possible that an other thread appends a Server to the servers list while I am iterating over copyList but closeCurrentlyOpen is supposed to result in an emtpy list. As the addServerToList method is synchronized to the servers-list, doing this

public void closeCurrentlyOpen() {
   synchronized(this.servers) {
     for(ServerObject server : this.servers) {
        stopAndRemove(server)
     }
    }
}

will solve the problem with modifications. But then I can not synchronize the code in the stopAndRemove method which is necessary if it is directly called.

I seems to me that the design of this three methods probably needs a workover. Ideas anybody?

Answer 1

When I've done this before, I always used the "old school" LinkedList collection, an Iterator, and the Iterator.remove() method to remove the current item.

Answer 2

Refactor out all the ServerObject stopping code from stopAndRemove into a private stopServer method, and then do the removal separately in stopAndRemove and closeCurrentlyOpen. Then you can use a ListIterator to remove them (or just stop them all in a for loop and clear the list at the end).

Answer 3

Split off a method stop() from stopAndRemove(). Then write the loop with an explicit iterator, do the stop and then iterator.remove().

"and" in a method name is a code smell.

Answer 4

You should get an iterator and remove using it. You are getting the exception because iterators are fail-fast in java.

Answer 5

You might find this article about ConcurrentModificationException has some advice in this area.

Answer 6

Perhaps this is the wrong way to do it, but I always create a removal collection, which contains indexes or references to the objects that need to be removed. I then iterate over that collection and remove those indexes/objects from the original collection. Probably not the most efficient but it got the job done.

Instead of

for(Collection things : thing)  
    things.remove(thing)

I use

Collection toRemove = new LinkedList();
for(things : thing)
    toRemove.add(thing);

for(toRemove : thing)
    things.remove(thing)

Answer 7

Answering to the title of the question, not the specific details of the given example. In fact, this solution is not even appropriate in the given situation (refactoring is appropriate, as suggested by others).

However, it seems that many java programmers are not aware of CopyOnWriteArrayList (part of JDK since 1.5) and are trying to roll their own solutions to the same problem (copy list before iterating).