2

Lets say I have a large number of worker threads all actively processing, and a supervisor thread that waits for them all to complete. Traditionally I could do something like: 

for(Worker w:staff){
    w.start();
}
for(Worker w:staff){
    w.join();
}

..and all would be well. However in this case the size of my worker list (ArrayList staff) is variable and may change at arbitrary times. My best knowledge tells me something like this should work:

synchronized(this){
    for(Worker w:staff){
        w.start();
    }
}
while(true){ //this loop will keep the supervisor running until all workers have stopped
                  //interrupts will occur when changes to the size of staff list occur
    try{
        Iterator<Worker> it;
        synchronized(this){
            it = staff.iterator();
        }
        while(it.hasNext()){
            Worker w = it.next();
            w.join();
        }
        return;
    }catch(InterruptedException ie){continue;}
}

This however, still results in a conncurrentModificationException on the line 

Worker w = it.next();

...which to me seems strange, because I thought that once you retrieved the iterator it was separate from the list itself. My next idea is to clone the iterator or staff list so that it IS separate from the original changing list, but I thought I would give you experts a shot at it first.

Andrew
  • 617
  • 2
  • 6
  • 19
  • unless you're writing some concurrency framework there's hardly any point in using low-level java idiosynchratic stuff like *join()*. I've got a 250 KLOC or so codebase which is one heck of a multi-threaded application (doing number crunching and spreading the load on all the cores) here and we're using low-level things like *join()* exactly, well... **zero** time. But higher-level constructs like the ones detailed in *Java Concurrency in Practice*, that, we use all the time. And it's full of 'producers' and 'consumers' but *join()*, not once. – SyntaxT3rr0r Jun 06 '11 at 21:45
  • Agreed that you should not use `join`. There's a problem with your code: your never `start` your newer threads added to the list. If you start them somewhere else, it would be important to know because it needs to be synchronized too. – toto2 Jun 06 '11 at 23:10

2 Answers2

4

An Iterator instance is invalidated if any changes are made to the underlying collection outside of the Iterator instance.

Why don't you consider using an ExecutorService instead? Make Worker implement Callable, then use one of the service's invokeAll() methods.

CyclicBarrier or CountDownLatch are lower-level tools that can be used to build similar function.

erickson
  • 265,237
  • 58
  • 395
  • 493
  • I think this would have been the correct way to do things. However the code is written as is now. I will just duplicate the list this time. Next time I will bother to learn higher-up concurrency primitives. – Andrew Jun 06 '11 at 22:17
  • @Andrew - don't wait till next time. Fix it this time, and reduce the technical debt that your successor has to deal with. – Stephen C Jun 07 '11 at 00:39
2

You get a ConcurrentModificationException when you try to modify a list while an Iterator is partially complete however in your code samples there is no code which adds or removes from the list therefore the problem is elsewhere in your code.

Basically don't remove or add from the list while you are using an Interator, you can remove items using Iterator.remove() instead of List.remove() if you have to remove.

3urdoch
  • 7,192
  • 8
  • 42
  • 58
  • As stated in the question: "the size of my worker list is variable and may change at arbitrary times", so simply avoiding changing the list is not an option. – Andrew Jun 06 '11 at 22:12