JAVA synchronized block not working in a multithreaded environment

Question

I have the below code and trying to understand why the synchronization is not achieved here:

class Main {
    Thread t1 = new OpenServerSockets();
    t1.start();
}

public class OpenServerSockets extends Thread {
    @Override
    public void run() {
        while(true){
            Thread t = new ClientResponder(clientSocket, dis, dos);
            t.start();
        }
    }

public class ClientResponder extends Thread {
    @Override
    public void run(){
        synchronized(this) {
            // some other static method call in another class.
        }
    }
}

The synchronized block gets called by multiple threads at the same time. Why is it so? Isn't synchronized block used this way not supposed to ensure mutual exclusion execution of code?

you probably have more than one instance of `ClientResponder`, so `this` will vary — Andrew Tobilko, Oct 02 '19 at 07:41
Ok, I got the point but what is a static object and how do we use one? — Pankaj, Oct 02 '19 at 07:49
@Pankaj check posted the answer. An alternate approach is to pass lock object while creating the thread or pass ReentrantLock which provides the advanced features. — Govinda Sakhare, Oct 02 '19 at 07:51
Okay, so what I understood is that when we use `synchronized(ClassName.class)` , here ClassName.class is treated as a static object? — Pankaj, Oct 02 '19 at 07:57
@Pankaj Yes. I have updated the answer which uses `ReentrantLock`. — Govinda Sakhare, Oct 02 '19 at 08:01

Govinda Sakhare · Answer 1 · 2019-10-02T08:14:13.717

I can see the lock is on this.

Thread t1 = new ClientResponder();
Thread t2 = new ClientResponder();

Now for t1 and t2 this is referring to a different objects. You need a class level lock.

public class ClientResponder extends Thread {
    @Override
    public void run(){
        synchronized(ClientResponder.class) {
            // some other static method call in another class.
        }
    }
}

The better approach would be to use ReentrantLock. It provides advanced features which syncronized doesn't provide

class ClientResponder extends Thread {

   public static final ReentrantLock lock = new ReentrantLock ();

    @Override
    public void run() {
        lock.lock ();
        // task.
        lock.unlock ();
    }
}

A quick read for the reference: Java Synchronized Block for .class

Rentrant Lock: https://javarevisited.blogspot.com/2013/03/reentrantlock-example-in-java-synchronized-difference-vs-lock.html

Thanks @Govinda for the answer. Will be helpful if you can share some more insights for the Reentrant lock or a link to a good read. Thanks! — Pankaj, Oct 02 '19 at 08:11

score 0 · Answer 2 · answered Oct 02 '19 at 07:56

As has been covered in the comments, you're synchronizing on different objects, so there's no coordination between the threads.

Instead, you need to have the threads share an object that they synchronize on. You could create that object in Main or OpenServerSockets and then pass it into the constructor of ClientResponder, and then ClientResponder could synchronize on that object.

But there are other possible issues:

It's not clear why OpenServerSockets should be a Thread
It seems odd that OpenServerSockets constantly spawns new threads in a tight loop
You're wrapping your entire ClientResponder run method in a single synchronized block. That means that for the thread's entire useful life it's holding the lock. That seems unlikely to be what you need.
In general, it's best practice to implement Runnable rather than to extend Thread, and then pass an instance of the class implementing Runnable into new Thread.

Re #2: ClientResponder's run should instead probably look something like this:

public void run(){
    while (this.running) {
        synchronized (this.objectToSynchronizeOn) {
            // Do one unit of work that requires synchronization
        }
    }
}

That way, each of the threads gets a chance to do something in-between the work done by other threads.

Thanks for the answer Crowder. I have posted an edited snippet of the code just to ask where I had doubts. — Pankaj, Oct 02 '19 at 08:08
@Pankaj - Sure, of course, and that's good. A key thing when doing a [mcve], though, is to make sure it is representative of your actual code. If your actual code doesn't have the *entire* `run` method in the `synchronized` block, for instance, the MRE shouldn't either (even if it just has comments or something). — T.J. Crowder, Oct 02 '19 at 08:13
Thanks @Crowder, dint heard of the MRE concept before. It's a cool thing. — Pankaj, Oct 02 '19 at 23:27

JAVA synchronized block not working in a multithreaded environment

2 Answers2