CODEWITHSUNDEEP
javamultithreading
Vahid Hashemi
Vahid Hashemi

Reputation: 5240

Synchronized Arraylist between two threads doesn't return the same value

I have following code :

import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;

class MyThread implements Runnable{

    private List<Integer> myList;

    public MyThread(List<Integer> list){
            this.myList = list;
    }
    private void updateList(int i){
           synchronized (myList) {
               myList.add(i);
           }
    }
    @Override
    public void run() {
        for( int i = 0; i < 1000000;i++){
                updateList(i);
        }
        System.out.println("end: " + myList.size());
    }
}
public class MyExecutor {

    private List<Integer> taskList = new ArrayList<>();
    private void launch(){

        ExecutorService executorService= Executors.newFixedThreadPool(10000);
        executorService.execute(new MyThread(taskList));
        executorService.execute(new MyThread(taskList));
        executorService.shutdown();
    }
    public static void main(String[] args) {
        MyExecutor test = new MyExecutor();
        test.launch();
    }
}

the output should be : 2000000

I will get different result which means these two threads are replacing each other's value.

I can't figure out where is the problem, tried several modifications on this code but none of them has fixed the problem. (replaced with Vector / added synchronize in constructor / added volatile)

Why doesn't this code work correctly?

Edit

At both thread I expect to get 1000000

Upvotes: 1

Views: 99

Answers (2)

Andrew
Andrew

Reputation: 49606

You are getting

end: 1065878
end: 2000000

The first line is from the thread that has finished its job first. It shouldn't be exactly 1M, because several threads are working. It's reasonable to assume that by the time one first thread finishes adding its 1M, the other has added at least one.

The second line is always 2M (as you expected ) due to the synchronised method.

I guess the first thread should execute for the exact number I wanted, no more no less.

Things happened in parallel. The threads were running. Each was trying to invoke updateList: one entered, the others waited. There was no priority on who should be next, so the control over the method was being passed among all the workers in a rather random manner.

I bet you are still thinking of the sequential execution :) One thread runs the whole run method, prints 1M, the other takes a 1M-sized list and adds its portion.

To understand it better, add a print statement

private void updateList(int i) {
    synchronized (myList) {
        myList.add(i);
        System.out.println(Thread.currentThread().getName() + " added " + i);
    }
}

and reduce the number of elements to add by a task to, let's say, 10.

pool-1-thread-1 added 0
pool-1-thread-1 added 1
pool-1-thread-1 added 2
pool-1-thread-1 added 3
pool-1-thread-2 added 0
pool-1-thread-2 added 1
pool-1-thread-2 added 2
pool-1-thread-2 added 3
pool-1-thread-1 added 4
pool-1-thread-1 added 5
pool-1-thread-1 added 6
pool-1-thread-1 added 7
pool-1-thread-1 added 8
pool-1-thread-1 added 9
end: 14
pool-1-thread-2 added 4
pool-1-thread-2 added 5
pool-1-thread-2 added 6
pool-1-thread-2 added 7
pool-1-thread-2 added 8
pool-1-thread-2 added 9
end: 20

Upvotes: 2

Andy Turner
Andy Turner

Reputation: 140309

the output should be : 2000000

No, for three reasons:

  1. You are printing two things, so the output won't be a single number.
  2. It prints the size when each thread happens to have added 1000000 things; you know nothing about how much the other thread has done at this point.
  3. You are not accessing the size in a synchronized way, so you are liable to get a non-up to date value.

Upvotes: 3

Related Questions