How to synchronize multithreaded app in one class, without worker threads and controller?

Question

I have multiple threads which calculate the sum of a given array roughly speaking. I can't use separate worker threads and controller rather the whole program must be written in one class. The point of the program is to print the result of the calculation such that thread id 0 is the first to print the results, then thread id 1, then thread id 2, and so on in that order.

I wrote two synchronized methods in the class: finished - notify all threads once a particular thread finished the calculation and waitForThread - waits if threads with id's lesser than its own still haven't finished.

Because I can't use a separate controller class, I decided to create a static array doneArr in which each cell symbolizes a thread id and once a thread finished its job it updates the corresponding cell in doneArr. This way waitForThread knows when to wait.

I can't seem to get this to work, can you please point me to the right direction?

EDIT: the problem can be easily solved by using join() but I'm looking for a solution that uses synchronization.

This is the code:

public class WorkThread extends Thread {
    private int[] vec;
    private int id;
    private int result;
    private static int[] doneArr;
    private static int progress = 0;

    public WorkThread(int[] vec, int id) {
        this.vec = vec;
        this.id = id;
        doneArr = new int[vec.length];
        for(int i = 0; i < doneArr.length; i++) {
            doneArr[i] = -1; //default value
        }
    }
    private boolean finishedUpToMe() {
        for(int i = 0; i < id; i++) {
            if(doneArr[i] == -1)
                return false;
        }
        return true;
    }
    private synchronized void waitForThread() throws InterruptedException {
        while(!finishedUpToMe() && id > 0)
            this.wait();
        progress++;
    }
    private synchronized void finished() throws IllegalMonitorStateException {
        this.notifyAll();
    }
    public int process(int[] vec, int id) {
        int result = 0;
        System.out.format("id = %d\n", this.id);
        for(int i = 0; i < vec.length; i++) {
            vec[i] = vec[i] + 1;
            result = result + vec[i];
        }
        return result;
    }
    public void run() {
        try {
            this.waitForThread();
        }catch (InterruptedException e) {
            System.out.println("waitForThread exception");
        }
        result = process(vec, id);
        doneArr[id] = id;
        System.out.format("id = %d, result = %d\n", id, result);
        try{
            this.finished();
        }catch (IllegalMonitorStateException e) {
            System.out.format("finished exception\n");
        }
    }
    public static void main(String[] args) {
        int[] vec = {1,2,3,4};
        WorkThread[] workers = new WorkThread[3];
        for(int i = 0; i < workers.length; i++) {
            workers[i] = new WorkThread(vec, i);
        }
        for(int i = 0; i < workers.length; i++) {
            workers[i].start();
        }
        System.out.format("main\n");
    }
}

Answer 1

You repeatedly initialize the same static member, which while harmless here is a fairly bad practice. Instead, use a static initialization block to initialize static variables (not the constructor).

private static final int[] doneArr;
static {
    for(...) {...}
}

But of course, in this case you run into the problem that its length is variable. I also notice that doneArr contains ints, but you simply use it as a flag (ie -1 or !-1). So instead, use a Map<Integer, Boolean>.

private static final Map<Integer, Boolean> threadIsDone = new HashMap<>();

public WorkThread(int[] vec, int id) {
    threadIsDone.put(id, false);
    ...
}

Once you do this, while not strictly necessary in this particular case it is still good practice to protect against missing values when you are examining it.

private boolean finishedUpToMe() {
    for(int i = 0; i < id; i++) {
        final Boolean threadDone = threadIsDone.get(i);
        if (threadDone != null && !threadDone)
            return false;
    }
    return true;
}

As for your threading issue, it seems that you used methods that are declared as synchronized. That's great for methods on a shared object, but that's not actually what you have - you have multiple unique worker objects. Because of this, you aren't actually synchronizing on the same object. This means:

• The JVM doesn't have to make writes visible to other threads.
• If writes do happen to become visible, you have a race condition.
• The call to notifyAll() doesn't actually accomplish anything.

Instead, use a lock object declared static final.

private static final Object lockObj = new Object();

private void waitForThread() {
    synchronized(lockObj) {...}
}

private void finished() {
    synchronized(lockObj) {...}
}

Edit: Other Thoughts

• Your code would be more readable if you declared variables closer to where they were actually used (ie immediately above the respective method).
• Your process(...) method is an instance method, but takes as arguments references to the instance variables that it already has access to.
• You should declare variables final if you aren't intending to modify them, for a variety of reasons.