why synchronized method is not working for multithread

Question

I have written one program where two threads are running that's why i have used synchronized keyword. My code is below-

public class TestThreadAnother {
    public synchronized void decrement(){
        try {
            for (int i = 4; i > 0; i--) {
                System.out.println("Thread " + i);
            }
        } catch (Exception e) {
            e.printStackTrace();
        }
    }
}


public class TestThreadClass extends Thread {
    private String threadName;
    TestThreadAnother obj1;

    public TestThreadClass(String threadName, TestThreadAnother Obj1) {
        this.threadName = threadName;
        obj1 = Obj1;
        System.out.println("Creating " + threadName);
    }

    @Override
    public void run() {
        System.out.println("Running " + threadName);
        obj1.decrement();
        System.out.println("End of " + threadName);
    }    
}

public class TestThreadMain {

    public static void main(String[] args) {
        TestThreadAnother obj1 = new TestThreadAnother();

        TestThreadClass t1 = new TestThreadClass("Thread 1", obj1);
        TestThreadClass t2 = new TestThreadClass("Thread 2", obj1);

        t1.start();
        t2.start();
    }
}

The output shoud be -

Creating Thread 1
Creating Thread 2
Running Thread 1
Thread 4
Thread 3
Thread 2
Thread 1
End of Thread 1
Running Thread 2
Thread 4
Thread 3
Thread 2
Thread 1
End of Thread 2

But I am getting the following result-

Creating Thread 1
Creating Thread 2
Running Thread 2
Thread 4
Thread 3
Running Thread 1
Thread 2
Thread 1
Thread 4
Thread 3
Thread 2
Thread 1
End of Thread 1
End of Thread 2

but my question is as i am using synchronized method then why this is happening? As know if i use synchronized then at first 1st thread will execute and then 2nd one will execute.But in my case sometimes this is not happening. Java Experts need your help regarding this problem.

Emileemilee

only after thread2 completed the decrement method , thread1 executed the same method- because only the method is synchronized – Incomputable

What are you expecting the try/catch in decrement() to catch? – Kaplan

Answer 1

The output is correct. Only your decrement() method is synchronized. This means that the decrement() method of this object can't be called by more than one thread at a time. But that doesn't prevent other threads from doing something else while one thread is executing decrement().

Your output shows that while thread 2 is executing decrement(), thread 1 is executing the first System.out.println() in the run() method, then possibly waits for thread 2 to have completed its call to decrement(), then executes decrement().

Charkha

But if i want my output just like the first one then what should i do? – Emileemilee

You would put the two System.out.println() calls of the run() method in the decrement() method. Or you would share a single Runnable between your two threads, and have its run() method synchronized. Or you would explicitely synchronize the whole body of the run() method on a shared lock object. That would still not guarantee that thread1 starts before thread2, though. – Charkha

@Emileemilee Try it the way I mentioned in my answer. – Laparotomy

Answer 2

The important thing that you must understand is. Synchronization only provides the guarantee that two threads will not hold the lock on the same object at the same time. In your case, one thread (either thread2 or thread1) will get the lock and will call decrement() method. Until, this thread finishes, the other threda will be blocked.

Now, analyzing your output :

Creating Thread 1
Creating Thread 2
Running Thread 2 // Thread -2 has the lock now.
Thread 4         // thrad-2's decrement()
Thread 3
Running Thread 1  // thread-1 is blocked. So, only this line is printed
Thread 2         // still thread2
Thread 1      // still thread2
Thread 4        // now thread1 decrement() starts
Thread 3
Thread 2
Thread 1
End of Thread 1
End of Thread 2

print the thread.getName() in your decrement method to get better idea.

Grussing

Answer 3

You should do it this way,

@Override
public void run() {
    synchronized(obj1){
          System.out.println("Running " + threadName);
          obj1.decrement();
          System.out.println("End of " + threadName);
    }

}    

And method can be non-synchronized

public void decrement(){
    try {
        for (int i = 4; i > 0; i--) {
            System.out.println("Thread " + i);
        }
    } catch (Exception e) {
        e.printStackTrace();
    }
}

Laparotomy

If i synchronized obj1 then still it is not sure that every time thread 1 will execute first and then thread 2.because in the o/p sometime thread 1 is executing first and then thread 2 and sometimes vice versa. – Emileemilee

@Emileemilee - yes.. you are right.. there is no guarantee.What you could do, is use thread#join. – Grussing

Answer 4

This is because following two lines are not in synchronized block:

System.out.println("Running " + threadName);

System.out.println("End of " + threadName);

So after and before decrement() thread can be switched. If you want expected observations you can modify decrement method like below:

public synchronized void decrement(String threadName ){
        System.out.println("Decrement: " + threadName);
        try {
            for (int i = 4; i > 0; i--) {

                System.out.println("Thread " + i);
            }
        } catch (Exception e) {
            e.printStackTrace();
        }
        System.out.println("End Decrement: " + threadName);
    }

change run method like below,

public void run() {
    obj1.decrement(threadName);

}   

Landsman

Still it is not guarantee that my thread 1 will execute first and then thread 2,because sometimes i am getting -Creating Thread 1 Creating Thread 2 Running Thread 2 Thread 4 Thread 3 Thread 2 Thread 1 End Decrement: Thread 2 Running Thread 1 Thread 4 Thread 3 Thread 2 Thread 1 End Decrement: Thread 1 – Emileemilee

Yes, This is never guaranteed. Thread Scheduler is free to choose any runnable Thread from pool of threads first. If you want to execute thread t1 first you may have to put Thread.sleep after you start Thread t1. – Landsman

Answer 5

The other answers have already explained why synchronized wasn't working as you expected. This is more of a follow-up on the comment:

Still it is not guarantee that my thread 1 will execute first and then thread 2

You can handle this externally of the threads using join on the first one:

t1.start(); // Start t1
t1.join();  // Wait for t1 to complete
t2.start(); // Start t2

If you need the main thread to keep executing while the threads run one at a time, you can create a manager thread to execute them. Fortunately, someone already did this for us:

ExecutorService executor = Executors.newSingleThreadExecutor();
executor.execute(t1);
executor.execute(t2);

public static ExecutorService newSingleThreadExecutor()

Creates an Executor that uses a single worker thread operating off an unbounded queue. (Note however that if this single thread terminates due to a failure during execution prior to shutdown, a new one will take its place if needed to execute subsequent tasks.) Tasks are guaranteed to execute sequentially, and no more than one task will be active at any given time.

(emphasis mine)

Backbreaker

Answer 6

Well since System.out.println of "Running ..." and "End of Running..." and decrement() method are not in an atomic operation

If you want to stick with the synchronized method:

just put the

System.out.println("Running " + threadName); 

and

System.out.println("End of " + threadName);

inside your decrement() method, then i think you will be able to obtain your desired output.

Cashmere