How can I use synchronized block well?

Question

I want to print "ping" "pong", which are in same class but different method, 5 times using synchronized block.

The problem is that it stops after print ping pong once.

How can I print ping pong 5 times?

I think I put notifyAll() and wait() in right place.

print result

ping
pong

here is my main class

public class ThreadTest2 {

    public static void main(String[] args) throws {
        Thread thread1 = new Thread(() -> {
            forLoop("a");
        });
        Thread thread2 = new Thread(() -> {
            forLoop(null);
        });

        thread1.setPriority(10);
        thread2.setPriority(1);
        thread1.start();
        thread2.start();
    }

    static void forLoop(String target) {
        AA aa = new AA();
        try {
            for(int i=0; i<5; i++){
                if(target != null){
                    aa.ping();
                }
                else{
                    aa.pong();
                }
            }
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
    }
}

here is my ping pong class

public class AA {

    Thread thread;
    public void ping() throws InterruptedException {
        synchronized (this) {
            System.out.println("ping");
            wait();   
            notifyAll();     
        }
    }

    public void pong() throws InterruptedException {
        synchronized (this) {
                System.out.println("pong");
                notifyAll();
                wait();
        }
    }
}

Thank you!

ping
pong
ping
pong
ping
pong
ping
pong
ping
pong

Answer 1

The problem is that it stops after print ping pong once.

I really dislike these sorts of academic questions. Having two threads lock-step with each other is exactly what you don't want to do with threads which are designed to run asynchronously and coordinate less often.

There are a number of things wrong with your code:

• Each call to the forloop() method creates a new instance of AA. Since you call it in each thread, each thread will be locking and waiting on different instances of AA so they won't see the other thread's notifyAll() and will always deadlock. You need to create one AA and pass the same instance to both forloops.

• You need to call notifyAll() before wait() in both cases otherwise the ping thread might wait before waking up the pong thread causing a deadlock.

• There is no guarantees that the initial ping will run before pong. This is hard to solve right. One (ugly) solution would be a boolean firstPrinted field in AA and have it set to true after the println(...) and have pong do something like this:

    synchronized (this) {
        while (!first) {
            wait();
        }
    }
  

• Once one of the threads finishes after the 5 iterations, the other thread will still be waiting so will never exit. You need to somehow skip the last wait. One option here is to add a call to aa.oneMoreNotify() after the for 1 to 5 loop finishes:

    public void oneMoreNotify() {
        synchronized (this) {
            notifyAll();
        }
    }
  

Couple other comments:

• thread.setPriority(...) really does very little unless you have very CPU bound computational loops. These should be removed.
• When you catch (InterruptedException e) you should immediately call Thread.currentThread().interrupt() as a good pattern.
• Passing in String target as opposed to a boolean is a strange pattern. Maybe your real code uses target otherwise.
• You might consider passing in the message to be printed as an argument to the forloop. Then the ping and the pong can call the same method. Then the first wait logic should happen when message.equals("pong").
• The Thread field in AA is not used and will confuse.

Answer 2

• you can change code like this,Each thread control itself loop
• must execute notifyAll() first and then execute wait()
• if there are 3 Threads，need to control by a global variable

public class SynchronizedTest {
    public void ping(int count) throws InterruptedException {
        synchronized (this) {
            for (int i = 0; i <count ; i++) {
                System.out.println("ping");
                notifyAll();//
                wait();
            }
        }
    }

    public void pong(int count) throws InterruptedException {
        synchronized (this) {
            for (int i = 0; i <count ; i++) {
                System.out.println("pong");
                notifyAll();
                wait();
            }
        }
    }


    public static void main(String[] args) throws InterruptedException {
        SynchronizedTest aa = new SynchronizedTest();
        Thread thread1 = new Thread(() -> {
            try {
                aa.ping(5);
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        });

        Thread thread2 = new Thread(() -> {
            try {
                aa.pong(5);
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        });
        thread1.start();
        thread2.start();
    }
}