CODEWITHSUNDEEP
javalistmultithreading
Shekhar
Shekhar

Reputation: 5931

I am confused -- Will this code always work?

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

Upvotes: 0

Views: 186

Answers (5)

Prine
Prine

Reputation: 12528

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>());

Upvotes: 1

MartinStettner
MartinStettner

Reputation: 29164

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 ...)

Upvotes: 2

aioobe
aioobe

Reputation: 421020

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

Upvotes: 0

Tom Hawtin - tackline
Tom Hawtin - tackline

Reputation: 147164

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

Upvotes: 1

b_erb
b_erb

Reputation: 21241

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.

Upvotes: 5

Related Questions