Synchronized threads not working correctly

Question

I am trying to understand threads so I made a simple program. I read numbers from a file, I put them into an array and then for every element in the array I multiply it by itself and then, when the whole array was traversed I write it to a file. I use n threads for this program. The problem is that not everytime I get the answer I expect. For example:

Numbers: 2 3 4 1 1 4 Output: 4 9 16 1 1 16 - correct

Sometimes I get this for the same Numbers: Output: 16 81 256 1 1 256 - which clearly is not what I want.

Here is the source code:

class MyThread2 which makes the multiplying:

package thr;

import java.util.ArrayList;

public class MyThread2 extends Thread{

    private ArrayList<Integer> array;

    public MyThread2(ArrayList<Integer> a){
        array = a;
    }

    public void run(){
        for(int i=0; i < array.size(); i++){
            synchronized (this) {
                array.set(i, array.get(i) * array.get(i));
            }
        }
    }
};

class App - which is the application

package thr;

import java.io.BufferedReader;
import java.io.BufferedWriter;
import java.io.FileReader;
import java.io.FileWriter;
import java.io.IOException;
import java.util.ArrayList;

public class App {

    public ArrayList<Integer> readFromFile(String str){
        ArrayList<Integer> a = new ArrayList<Integer>();
        try{
            BufferedReader reader = new BufferedReader(new FileReader(str));
            String message = null;
            while((message = reader.readLine()) != null){
                try{
                    int nr = Integer.parseInt(message);
                    a.add(nr);
                }catch(NumberFormatException e){
                    System.out.println(e.getMessage());
                }
            }
            reader.close();
        }catch(IOException e){
            System.out.println(e.getMessage());
        }

        return a;
    }

    public void writeToFile(ArrayList<Integer> a){
        try{
            BufferedWriter writer = new BufferedWriter(new FileWriter("output.txt"));
            for(int i=0; i < a.size(); i++){
                writer.write(a.get(i).toString());
                writer.newLine();
            }
            writer.close();
        }catch(IOException e){
            System.out.println(e.getMessage());
        }
    }


    public ArrayList<Integer> runThreads(ArrayList<Integer> a, int nrThreads) {

        ArrayList<MyThread2> thread = new ArrayList<MyThread2>();
        int i;


        for(i=0; i < nrThreads; i++){
            thread.add(new MyThread2(a));
        }


        for (i = 0; i < thread.size(); i++) {
            thread.get(i).start();
        }

        System.out.println("in fctie");
        print(a);
        System.out.println("in fctie");

        return a;
    }

    public void print(ArrayList<Integer> array){
        for(int i=0; i < array.size(); i++){
            System.out.println(array.get(i));
        }
    }

    public static void main(String[] args){
        App app = new App();
        ArrayList<Integer> array;

        array = app.readFromFile("numbers.txt");
        app.print(array);
        app.runThreads(array, 5);
        app.print(array);
        app.writeToFile(array);
    }

}

Does anyone know what I'm doing wrong?

Answer 1

Multiple threads are executing the same operation simultaneously. Therefore, you will of course get different answers each time you run it, based on which Threads run at which times (which is not deterministic).

Note that your synchronization is inside your for loop, which means that each thread will loop independently of the synchronization. Also, you are synchronizing on this which means that each instance of the Thread is synchronizing only with itself i.e. there is no synchronization at all. You could synchronize outside the loop on a common monitor Object. But that of course, would basically cause each Thread to do the operation serially. But at least your result will be deterministic.

If you want to do the multiplication only once for each element in the array, in a multi-threaded manner, you should initialize each Thread with a specific slice of the array to process.

Answer 2

There are many problems in your code.

First: your main method starts N threads, and then prints and saves the contents of the list that all these threads are modifying. You're not waiting for the threads to finish, so the main threads and all the MyThread2 threads are accessing the list concurrently, in an unpredictable way.

Second: there is 0 synchronization. The only place where you have a synchronization is in

synchronized (this) {
    array.set(i, array.get(i) * array.get(i));
}

So each thread synchronizes on itself. All threads use a separate lock, and thus access the thread-unsafe list concurrently. That's a bit like if you had a safe lock with 10 doors and 10 keys: in such a design, 10 people can open the lock concurrently. What you need is a single door and a single key.