15

I am getting an exception when I try to remove elements from CopyOnWriteArrayList using an iterator. I have noticed that it is documented

Element-changing operations on iterators themselves (remove, set, and add) are not supported. These methods throw UnsupportedOperationException.

(from http://download.oracle.com/javase/6/docs/api/java/util/concurrent/CopyOnWriteArrayList.html)

Now, surprisingly i can iterate it with foreach and use the remove() function . But then I get the famous bug - when trying to remove an item from a list using a for loop - you skip the element next to the removed element. any suggestions then?

Bick
  • 17,833
  • 52
  • 146
  • 251

9 Answers9

22

Iterate over the collection choosing all the elements you want to delete and putting those in a temporary collection. After you finish iteration remove all found elements from the original collection using method removeAll.

Would that work out for you? I mean, not sure if deletion logic is more complicated than that in your algorithm.

Edwin Dalorzo
  • 76,803
  • 25
  • 144
  • 205
  • I was going to suggest that, but I decided not to as it's more verbose and less efficient. – Robin Green Apr 10 '11 at 14:47
  • 1
    Verbosity should not be an issue provided that it is right. Why do you say it is less efficient? All the contrary, in this case you force a single change in the "copy on write" collection". Don't you think so? – Edwin Dalorzo Apr 10 '11 at 14:49
  • 2
    It is more efficient in this case as every remove() operation creates a temporal array, and removeAll() does it only once. – Vladimir Dyuzhev Apr 10 '11 at 14:51
  • Vladimir. you are correct. though I was hoping to find a way to iterate back on the list. it would have been nicer - only one loop. and one memory instance. – Bick Apr 10 '11 at 14:54
  • 1
    Would for( int i=arr.size()-1; i>=0; i-- ) work for you? Still, temp array is a way better solution. – Vladimir Dyuzhev Apr 10 '11 at 15:29
7

EDIT: I'm an idiot. I missed the fact that this is a copy-on-write list so every removal means a new copy. So my suggestions below are likely to be suboptimal if there's more than one removal.

Same as for any other list whose iterator doesn't support remove, or anything where you're not using an iterator. There are three basic techniques that come to mind to avoid this bug:

  1. Decrement the index after removing something (being careful not to do anything with the index until the next iteration). For this you'll obviously have to use a for(int i=0; i < ... style of for loop, so that you can manipulate the index.

  2. Somehow repeat what the inside of the loop is doing, without literally going back to the top of the loop. Bit of a hack - I would avoid this technique.

  3. Iterate over the list in reverse (from end to start, instead of from start to end). I prefer this approach as it's the simplest.

Robin Green
  • 32,079
  • 16
  • 104
  • 187
  • number three was my lucky winner too. But how do I iterate back . the list doesn'.t supply has previous. – Bick Apr 10 '11 at 14:50
  • I now think my answer is a bad answer (see my answer edit just now). But if you really want to do it, I guess you'd have to use an old-style for loop and start at list.length()-1. – Robin Green Apr 10 '11 at 15:00
  • Dude. dont be so hard on yourself. At least you were the first :-). – Bick Apr 10 '11 at 15:04
  • 1
    Lists have a list iterator which has a hasPrevious() and previous() methods. You can set the initial position of the list iterator like this: myList.listIterator(myList.size()). Then iterate backwards. So, it is possible, but as you suggested not a good idea in this case d(^_^)b – Edwin Dalorzo Apr 10 '11 at 15:18
6

Since this is a CopyOnWriteArrayList it is totally safe to remove elements while iterating with forEach. No need for fancy algorithms.

list.forEach(e -> {
    if (shouldRemove(e))
        list.remove(e);
});

EDIT: Well of course that works if you want to delete elements by reference, not by position.

RamenChef
  • 5,557
  • 11
  • 31
  • 43
2

Ususlly you would iterate first gathering elemenet to be deleted in a separate list then delete them outside the for each loop (which is disguised iterator based loop anyway)

Tomasz Stanczak
  • 12,796
  • 1
  • 30
  • 32
1

You could use Queue instead of List.

private Queue<Something> queue = new ConcurrentLinkedQueue<Something>();

It's thread safe and supports iterator.remove(). Be aware of the thread-safe behavior of Queue iterators, though (check the javadoc).

Nacho Coloma
  • 7,070
  • 2
  • 40
  • 43
1

Something like this:

int pos = 0;
while(pos < lst.size() ) {
  Foo foo = lst.get(pos);
  if( hasToBeRemoved(foo) ) {
    lst.remove(pos);
    // do not move position
  } else {
    pos++;
  }
}
Vladimir Dyuzhev
  • 18,130
  • 10
  • 48
  • 62
  • This is nice but will get us calculating list.size a lot. it is possible though. – Bick Apr 10 '11 at 14:52
  • Calculating size happens during the removing of the element, and the removing is the real performance hit. Better practice, as it was said, to collect all elements to remove into a temporal array and remove them at once. – Vladimir Dyuzhev Apr 10 '11 at 15:26
1

If you want to delete all use just clear(). If you want to keep elements put them in a temporary ArrayList and get them back from there.

List<Object> tKeepThese= new ArrayList<>();
for(ListIterator<Object> tIter = theCopyOnWriteArrayList; tIter.hasNext();)
{
    tObject = tIter.next();
    if(condition to keep element)
        tKeepThese.add(tObject);
}
theCopyOnWriteArrayList.clear();
theCopyOnWriteArrayList.addAll(tKeepThese);
The incredible Jan
  • 741
  • 10
  • 17
0

the shortest and most efficient way:

List<String> list = new CopyOnWriteArrayList<>();
list.removeIf(s -> s.length() < 1);

internally it creates an temporary array with the same length and copies all elements where the predicate returns true.

keep in mind that if you use this method to actually iterate over the elements to perform some action, these actions cannot be performed in paralell anymore since the removeIf-call is atomic and will lock the traversal for other threads

benez
  • 1,856
  • 22
  • 28
0

Below works fine with CopyOnWriteArrayList

for(String key : list) {
    if (<some condition>) {
        list.remove(key);
    }
}
omar
  • 1