MultiThreading in Java not Working as Expected

Question

The first code does not always show the sum to be 1000, so I figured a way around to fix the problem but why does not the first code work? The results are highly inconsistent and I know that using synchronized does not serve any purpose in this case, but I was just trying.

class Thread1 extends Thread{
    int[] count;
    int[] event;
    Thread1(int[] event, int[] count){
        this.event=event;
        this.count=count;
    }
    public void run(){
        for(int i=0; i<500; i++){

            int x = event[i];
            synchronized (count){
                count[x]++;
            }
        }
    }
}

class Thread2 extends Thread{
    int[] count;
    int[] event;
    Thread2(int[] event, int[] count){
        this.event=event;
        this.count=count;
    }
    public void run(){
        for(int i=500; i<1000; i++){

            int x = event[i];
            synchronized (count){
                count[x]++;
            }

        }
    }
}

public class Learning {

    public static void main(String[] args) {
        Random random = new Random();
        int[] event = new int[1000];

        for(int i=0; i<event.length; i++){
            event[i] = random.nextInt(3);
        }

        Thread1 a = new Thread1(event, new int[3]);
        Thread2 b = new Thread2(event, new int[3]);

        a.start();
        b.start();

        int second = a.count[1]+b.count[1];
        int third = a.count[2]+b.count[2];
        int first = a.count[0]+b.count[0];

        System.out.println(first);
        System.out.println(second);
        System.out.println(third);

        System.out.println("SUM--> "+(first+second+third));
    }
}

WORKS HERE:

DOES NOT WORK HERE

The code sometimes shows a total of 1000 entries, sometimes doesn't. I don't feel there is any need to synchronize as no common resource is being accesed.

Answer 1

The Thread1 and Thread2 classes use their respective count objects to synchronize.

The problem is that you instantiate them like this:

    Thread1 a = new Thread1(event, new int[3]);
    Thread2 b = new Thread2(event, new int[3]);

See?

You are passing different arrays to the two threads. If your two threads use different objects as their count, you do not get mutual exclusion or proper memory visibility.

---

On further examination, it looks like the synchronized block is probably unnecessary anyway. (You don't need mutual exclusion, and you get certain guarantees that the child threads will see properly initialized arrays because of the start() happens-before.)

However, it is clear that it is necessary to join the two child threads in the main thread for a couple of reasons:

1. If you don't join(), you cannot be sure that the child threads have completed before the main thread looks at the results.

2. If you don't join(), there are potential memory anomalies ... even if the child threads have both terminated before the main thread looks at the counts. (The join() call imposes a happens-before relationship between the child threads and the main thread.)

---

Your attempted solution using stop() is bogus for the following reasons:

1. The stop() method is deprecated because it is dangerous. It shouldn't be used.

2. The stop() method doesn't have a specified synchronizing effect.

3. Based on the documented semantics (such as they are) there is no logical reason that calling stop() should fix the problem.

As a general rule "randomly trying thing" is not a sound strategy for fixing concurrency bugs. There is a good chance that a random chance will not fix a bug, but turn it from a bug that occurs frequently to one that occurs rarely ... or only on a different Java platform to the one you test on.

Why does it appear to work?

It looks like the child threads terminated before they stopped. But it is just luck that that is happening. It is unlikely to happen if you scale up the amount of work that the child threads do.

Answer 2

adding - a.stop(); b.stop(); after a.start(); b.start(); fixes the problem.

But I don't understand why.