I am confused -- Will this code always work?

Question

Hello,

I have written this piece of code

public class Test{
public static void main(String[] args) {

    List<Integer> list = new ArrayList<Integer>();

    for(int i = 1;i<= 4;i++){
        new Thread(new TestTask(i, list)).start();
    }

    while(list.size() != 4){
        // this while loop required so that all threads complete their work
    }

    System.out.println("List "+list);
}

}

class TestTask implements Runnable{

private int sequence;
private List<Integer> list;

public TestTask(int sequence, List<Integer> list) {
    this.sequence = sequence;
    this.list = list;
}

@Override
public void run() {
    list.add(sequence);
}
}

This code works and prints all the four elements of list on my machine.

My question is that will this code always work. I think there might be a issue in this code when two/or more threads add element to this list at the same point. In that case it while loop will never end and code will fail.

Can anybody suggest a better way to do this? I am not very good at multithreading and don't know which concurrent collection i can use?

Thanks, Shekhar

Answer 1

Afair, Lists are not thread-safe in Java, so you might get anything from working to crashing. Use synchronized access to the list in order to get a well-defined behaviour:

@Override
public void run() {
    synchronized(list) {
        list.add(sequence);
    }
}

This way, access to the list is only possible for a single thread at a time.

Also you I'd use Thread.join() to wait for the threads to finish (you have to keep them in a separate list for doing that ...)

Answer 2

Use this in order to get a real thread-safe list: List<Integer> list = Collections.synchronizedList(new ArrayList<Integer>());

Depending on your usage, also a CopyOnWriteArrayList could be interesting for you. Precisly, when traversal operations vastly outnumber mutations on that list.

Answer 3

No, there are no guarantees. The simplest solution would be to join with each thread.

Answer 4

Read up on wait() and notify() instead of having a busy-waiting while-loop.

Answer 5

I think you need to do two things. First of all you need to join the threads. Because atm the other loop will sometimes run even if the threads are not completed.

You have to do it like this:

Threads threads[4] = new Thread[4];

for(int i = 1;i<= 4;i++){
    threads[i] = new Thread(new TestTask(i, list));
    threads[i].start();
}

// to wait that all threads finish..
for(int i = 1;i<= 4;i++){
    threads[i].join();
 }


 while(list.size() != 4){
     // this while loop required so that all threads complete their work
 }

and you can make your ArrayList thread safe with packing it into:

List<Integer> list = Collections.synchronizedList(new ArrayList<Integer>());