4

I'm trying to figure out the best way to have multiple threads working from the same list of strings. For example, say I have a list of words, and I want multiple threads to work on printing out each word on this list.

Here is what I came up with. The thread uses a while loop, and while the iterator has next, it prints out and removes it from the list.

import java.util.*;
public class ThreadsExample {

    static Iterator it;

    public static void main(String[] args) throws Exception {

        ArrayList<String> list = new ArrayList<>();

        list.add("comet");
        list.add("planet");
        list.add("moon");
        list.add("star");
        list.add("asteroid");
        list.add("rocket");
        list.add("spaceship");
        list.add("solar");
        list.add("quasar");
        list.add("blackhole");


        it = list.iterator();

        //launch three threads
        RunIt rit = new RunIt();

        rit.runit();
        rit.runit();
        rit.runit();

    }
}

class RunIt implements Runnable {

    public void run()
    {
        while (ThreadsExample.it.hasNext()) {
            //Print out and remove string from the list
            System.out.println(ThreadsExample.it.next());

            ThreadsExample.it.remove();
        }
    }

    public void runit() {
        Thread thread = new Thread(new RunIt());
        thread.start();
    }
}

This seems to work, although I get some Exception in thread "Thread-2" Exception in thread "Thread-0" java.lang.IllegalStateException errors during the run:

Exception in thread "Thread-1" Exception in thread "Thread-0"
java.lang.IllegalStateException at
java.util.ArrayList$Itr.remove(ArrayList.java:864) at
RunIt.run(ThreadsExample.java:44) at
java.lang.Thread.run(Thread.java:745) java.lang.IllegalStateException
at java.util.ArrayList$Itr.remove(ArrayList.java:864) at
RunIt.run(ThreadsExample.java:44) at
java.lang.Thread.run(Thread.java:745)

Am I doing this correctly or is there a better way to have multiple threads working on the same pool of strings?

Gnoupi
  • 4,715
  • 5
  • 34
  • 50
Andrio
  • 1,852
  • 2
  • 25
  • 54
  • making a runnable that starts a thread is unnecessarily convoluted. – Nathan Hughes Apr 16 '15 at 13:46
  • Note that using Iterator.remove() was a good point to begin with (it prevents the usual [problem of removing items in a loop](http://stackoverflow.com/questions/223918/iterating-through-a-list-avoiding-concurrentmodificationexception-when-removing)). It's not concurrent access safe, though. – Gnoupi Apr 16 '15 at 13:48
  • using the same iterator in different threads looks, umm, strange. Common approach is to use queues. – Alexei Kaigorodov Apr 16 '15 at 21:39

3 Answers3

6

A better way to do this is to use a concurrent queue. The Queue interface is designed to hold elements in a structure prior to processing them.

    final Queue<String> queue = new ConcurrentLinkedQueue<String>();
    queue.offer("asteroid");

    ExecutorService executorService = Executors.newFixedThreadPool(4);

    executorService.execute(new Runnable() {
        public void run() {
            System.out.println(queue.poll());
        }
    });

    executorService.shutdown();
2

Try creating the list as a synchronized list using List.synchronizedList

Update your code like this:

ArrayList<String> list = Collections.synchronizedList(new ArrayList<>());
uldall
  • 2,458
  • 1
  • 17
  • 31
  • Are you sure that synchronized list iterator is also synchronized? – Alex Salauyou Apr 16 '15 at 13:44
  • You can extend ArrayList and overwrite all functions adding a `synchronized{}` wrap to every `return super.something();`, which, i believe is not as practical. – Felype Apr 16 '15 at 13:45
  • @Sasha True I am not sure that is the case. – uldall Apr 16 '15 at 13:46
  • `Collections.synchronizedList()` yields a `List` for which the individual methods are synchronized, but that's not enough to fix the problems in the original code. Each (`hasNext()`, `next()`, `remove()`) triple must be performed as an atomic group, else unwanted behavior is possible (e.g. `IllegalStateException` or `NoSuchElementException`). – John Bollinger Apr 16 '15 at 13:51
2

Am I doing this correctly or is there a better way to have multiple threads working on the same pool of strings?

You are not doing it correctly. Your code is not properly synchronized, and therefore its behavior is not well defined. There are a great number of ways you could approach the general problem you present, but one way the issues in your particular code could be fixed would be to change RunIt.run() to properly synchronize:

    public void run()
    {
        while (true) {
            synchronized(ThreadsExample.it) {
                if (ThreadsExample.it.hasNext()) {
                    //Print out and remove string from the list
                    System.out.println(ThreadsExample.it.next());

                    ThreadsExample.it.remove();
                } else {
                    break;
                }
            }
        }
    }

Note here that the hasNext() check, retrieval of the next element, and removal of that element are all handled within the same synchronized block to ensure mutual consistency of these operations. On the other hand, the scope of that block is contained within the loop, so that different threads executing the loop concurrently each get a chance to execute.

Note, too, that although in this case all the threads synchronize on the Iterator object, that's basically just a convenience (for me). As long as they all synchronize on the same object, it doesn't matter so much which object that is.

John Bollinger
  • 160,171
  • 8
  • 81
  • 157