Why don't these blocks of code give the same result?

Question

So I'm new to this Thread stuff and I wrote a simple program to test avoiding Race Conditions. My first attempt was with Named Inner classes :

/* App1.java */

package ehsan;

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

public class App1{
    private final int poolSize = 10;
    private final int numLoop = 5;
    private int lastThread = 0;

    public App1() {
        ExecutorService taskList = Executors.newFixedThreadPool(poolSize);
        for (int i = 0;i < poolSize;i++) {
            taskList.execute(new Counter());
        }
        taskList.shutdown();
    }

    private class Counter implements Runnable{

        @Override
        public void run() {
            synchronized (this) {
                int currentThread = lastThread;
                System.out.println("Current thread : "+currentThread);
                lastThread = lastThread + 1;
            }
            System.out.println("Thread was executed");
        }
    }

}

and App1Test.java :

package ehsan;

import java.io.IOException;

public class Test {
    public static void main(String[] args) throws IOException {
        new App1();
    }
}

So as a result it showed :

Current thread : 0
Thread was executed
Current thread : 1
Thread was executed
Current thread : 1
Thread was executed
Current thread : 3
Thread was executed
Current thread : 4
Thread was executed
Current thread : 5
Thread was executed
Current thread : 6
Thread was executed
Current thread : 7
Thread was executed
Current thread : 6
Current thread : 8
Thread was executed
Thread was executed

And whole things got mixed up and I'm facing Race conditions here even when I've use synchronized there.

But my second attempt worked! :

package ehsan;

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

public class App1 implements Runnable{
    private final int poolSize = 10;
    private final int numLoop = 5;
    private int lastThread = 0;

    public App1() {
        ExecutorService taskList = Executors.newFixedThreadPool(poolSize);
        for (int i = 0;i < poolSize;i++) {
            taskList.execute(this);
        }
        taskList.shutdown();
    }

    @Override
    public void run() {
        synchronized (this) {
            int currentThread = lastThread;
            System.out.println("Current thread : "+currentThread);
            lastThread = lastThread + 1;
            System.out.println("Thread was executed");
        }
    }
}

And the result was as I expected :

Current thread : 0
Thread was executed
Current thread : 1
Thread was executed
Current thread : 2
Thread was executed
Current thread : 3
Thread was executed
Current thread : 4
Thread was executed
Current thread : 5
Thread was executed
Current thread : 6
Thread was executed
Current thread : 7
Thread was executed
Current thread : 8
Thread was executed
Current thread : 9
Thread was executed

So my question is why my first attempt didn't work and the second one worked greatly? Thanks for helping me, I'm a beginner in Multi-Threaded programming!

Answer 1

In the first program, you create a different Counter instance as the Runnable whose run() method is executed by each thread, so synchronized (this) uses a different lock for each thread, and therefore the code is not thread safe. If you use the same Counter instance instead of creating a new one for each thread, this program will also behave as you expected.

    Counter counter = new Counter();
    for (int i = 0;i < poolSize;i++) {
        taskList.execute(counter);
    }

In the second program, you use the same App1 instance as the Runnable whose run() method is executed by all the threads, so synchronized (this) uses the same locks for all the threads.

Answer 2

With a great deal of research I found few ways to solve this.

Please Note : Some of these solutions were pointed out by Eran, Thomas and etc. and I thank them for helping me but I just wanted to collect all the possible solutions in a single answer so the future visitors of this post find the answers easily.

• For Named Inner Classes :

Instead of using this as the locking object for synchronization we can use the OutterClass instance :

 synchronized (App1.this) {
        int currentThread = lastThread;
        System.out.println("Current thread : "+currentThread);
        lastThread = lastThread + 1;
 }

• For Separated Classes :

Solution 1

---

Synchronizing on an object we are receiving from the Caller Class( The class containing the task list and ... ). Here is an example :

public App1 implements Runnable {

    private final Integer shared;/* Not spending time on auto-boxing */

    public App1(Integer sharedNum) {
        shared = sharedNum;
    }

    @Override
    public void run() {
        synchronization(shared){
            //code here
        }
    }
}

public App1Test {
    private final int forSharing = 14;
    public static void main(String[] args) {
        ExecutorService taskList = Executors.newFixedThreadPool(poolSize);
        taskList.execute(new App1(forSharing));
        // and lob lob
    }
}

Solution 2

---

Synchronization on class object :

synchronized (App1.class) { /* As the fields and methods of class App1 are the same for all objects */
    int currentThread = lastThread;
    System.out.println("Current thread : "+currentThread);
    lastThread = lastThread + 1;
}

Solution 3

---

Synchronizing on an static field ( Which I loved it because its really innovative ) :

/* Add this field to class definition */
private static Object sharedObject = new Object();

/* Now in `run` method  use the object like this : */
synchronized(sharedObject) {
    //TODO : write your code here :)
}

So these are the solutions to this sneaky problem which is a bit hard to debug :)

I hope this helps fellows who have faced the same problem :)

Cheers, Ehsan

Answer 3

synchronized (this) in Counter synchronizes on the instance, so if you pass a new instance to each thread (taskList.execute(new Counter());) there won't be any synchronization at all (since no 2 threads use the same instance to synchronize on). Thus either use synchronized(Counter.class) or some other shared monitor object (since Counter is an inner class of App1 you could use synchronize(App1.this)).

Your second approach works since you pass the same instance to each thread: taskList.execute(this);.