Why are my threads not synchronizing?

Question

I am trying to get a grasp on synchronizing threads, but I don't understand the problem I'm encountering.

Can someone please help me diagnose this or, even better, explain how I can diagnose this for myself?

import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.CyclicBarrier;

public class Controller {

public static void main(String[] args) {        
    int numThreads = 0;
    List<Thread> threads = new ArrayList<>();

    if (args.length > 0) {
        numThreads = Integer.parseInt(args[0]);
    } 
    else {
        System.out.println("No arguments");
        System.exit(1);
    }

    CyclicBarrier barrier = new CyclicBarrier(numThreads);
    int arr[][] = new int[10][10];

    for (int i = 0; i < numThreads; i++) {
        Thread newThread = new Thread(new ThreadableClass(barrier, arr));
        threads.add(newThread);
    }

    for (Thread thread : threads) {
        thread.start();
    }
  }
}

There is a main method (above) which accepts the number of threads I want as a command line argument. And there is a work-flow (below) which I am aiming to have increment all elements in a 2D array and print the array before the next thread has its chance to do the same:

import java.util.concurrent.BrokenBarrierException;
import java.util.concurrent.CyclicBarrier;

public class ThreadableClass implements Runnable {

private CyclicBarrier barrier;
private int arr[][];

public ThreadableClass(CyclicBarrier barrier, int[][] arr) {
    this.barrier = barrier;
    this.arr = arr;
}

@Override
public void run() {
    long threadId = Thread.currentThread().getId();
    System.out.println(threadId + " Starting");

    for (int i = 0; i < 10; i++) {
        changeArray();
        try {
            barrier.await();
        } catch (InterruptedException | BrokenBarrierException e) {
            e.printStackTrace();
        }
    }
}

private synchronized void changeArray() {
    for (int i = 0; i < arr.length; i++) {
        for (int j = 0; j < arr.length; j++) {
            arr[i][j]++;
        }
    }
    printArray();
}

private synchronized void printArray() {
    System.out.println(Thread.currentThread().getId() + " is printing: ");
    for (int i = 0; i < arr.length; i++) {
        for (int j = 0; j < arr.length; j++) {
            System.out.print(arr[i][j] + " ");
        }
        System.out.println();
    }
}
}

Imagining the size of the array is 2x2, the expected output would look something like this:

1 1
1 1
2 2
2 2
3 3
3 3
4 4
4 4 
...
...
(10 * numThreads)-1 (10 * numThreads)-1
(10 * numThreads)-1 (10 * numThreads)-1
(10 * numThreads) (10 * numThreads)
(10 * numThreads) (10 * numThreads)

Instead, all threads increment the array, and begin printing over one another.

Answer 1

Because you are providing new instance for each theard using new ThreadableClass(barrier, arr), basically, all the theadrs are using different ThreadableClass objects, so your code synchronized methods run parallely, so you need to use a single ThreadableClass object as shown below:

ThreadableClass threadableClass= new ThreadableClass(barrier, arr);
for (int i = 0; i < numThreads; i++) {
     Thread newThread = new Thread(threadableClass);
     threads.add(newThread);
}

The important point is synchronization is all about providing access (i.e., key) to an object for a single thread at a time. If you are using a different object for each thread, threads don't wait for the key because each thread has got its own key (like in your example).

Answer 2

There is nothing surprising about the result. You create n threads. You tell all threads to start. Each threads run() starts with:

long threadId = Thread.currentThread().getId();
System.out.println(threadId + " Starting");
...changeArray();

going to change that shared array. After writing to the array, you try to sync (on that barrier). Its too late then!

The point is: you have 10 different ThreadableClass instances. Each one is operating on its own! The synchronized key word ... simply doesn't provide any protection here!

Because: synchronized prevents two different threads calling the same method on the same object. But when you have multiple objects, and your threads are calling that method on those different objects, than there is no locking! What your code does boils down to:

threadA to call changeArray() .. on itself
threadB to call changeArray() .. on itself
threadC to call changeArray() .. on itself

...

In other words: you give n threads access to that shared array. But then you allow those n threads to enter changeArray() at the same time.

One simple fix; change

private synchronized void changeArray() {

to

private void changeArray() {
  synchronized(arr) {

In other words: make sure that the n threads have to lock on the same monitor; in that case the shared array.

Alternatively: instead of making changeArray() a method in that ThreadableClass ... create a class

ArrayUpdater {
  int arr[] to update

  synchronized changeArray() ...

Then create one instance of that class; and give that same instance to each of your threads. Now the sync'ed method will prevent multiple threads to enter!