notify() and wait() not working in Java

Question

I have 2 threads which I want to synchronize with wait() and notify(). However when I notify the thread which waits never resumes. This are my pieces of code. In Lib60870 i start both threads, and thread HandShake is synchronized with SerialReader.

public Lib60870(){ //Here I start threads
    try {
        myConnection=new Connection(LOCALHOST,port);
    } catch (IOException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
    }

    mySerialReader.start();
    myHandshake.start();}
}

Class SerialReader

public class SerialReader extends Thread {

private static boolean isPaused=true;

@Override
public void run() {
    synchronized(this){
        if(Lib60870.myConnection!=null){
            while(true){
                if(!isPaused){
                    byte inByte=Lib60870.myConnection.getByte();
                    if(inByte==0x68){
                        ...
                        }
                        notify();
                    }
                    else if(inByte==0x10){
                        ...         
                        }
                        notify();
                    }
                }
            }
        }
    }
}

public void setPause(boolean pause){
    isPaused=pause;
}

Class Handshake

public class HandShake extends Thread {


public void run() {
    synchronized(Lib60870.mySerialReader){
        Lib60870.mySerialReader.setPause(false);
        ...
        try {
            Lib60870.mySerialReader.wait();
        } catch (InterruptedException e1) {
            // TODO Auto-generated catch block
            e1.printStackTrace();
        }
        Lib60870.mySerialReader.setPause(true);
        ...
        Lib60870.mySerialReader.setPause(false);
        try {
            Lib60870.mySerialReader.wait();
        } catch (InterruptedException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        }
    }
}

}

Thanks in advance

Please post a [Minimal, Complete, and Verifiable example](http://stackoverflow.com/help/MCVE). — Jiri Tousek, Dec 15 '15 at 14:19

score 1 · Answer 1 · edited Dec 15 '15 at 14:53

There are many problems with your approach:

Extending Thread is considered bad practice.
Using wait/notify for something that can be done with something in java.util.concurrent is not a good idea.
Falling into a tightly spinning loop is not pausing.

Here is a pauseable thread class. Write yourself a Stepper object and use one of these to execute the step() method continuously while not paused. Use its pause/resume methods to pause it cleanly.

/**
 * PauseableThread is a Thread with pause/resume and cancel methods.
 *
 * The meat of the process must implement `step`.
 *
 * You can either extend this and implement `step` or use the factory.
 *
 * I cannot extend Thread because my resume will clash.
 *
 */
public abstract class PauseableThread implements Runnable {

    // The lock.
    private final ReadWriteLock pause = new ReentrantReadWriteLock();
    private final Lock readLock = pause.readLock();
    private final Lock writeLock = pause.writeLock();
    // Flag to cancel the whole process.
    private volatile boolean cancelled = false;
    // The exception that cause it to finish.
    private Exception thrown = null;
    // The thread that is me.
    private Thread me = null;

    @Override
    // The core run mechanism.
    public void run() {
        // Track my current thread.
        me = Thread.currentThread();
        try {
            while (!finished()) {
                // Block here if we're paused.
                blockIfPaused();
                // Don't do any more work if we've been asked to stop.
                if (!finished()) {
                    // Do my work.
                    step();
                }
            }
        } catch (Exception ex) {
            // Just fall out when exception is thrown.
            thrown = ex;
        }
    }

    // Have we finished yet?
    private boolean finished() {
        return cancelled || me.isInterrupted();
    }

    // Block if pause has been called without a matching resume.
    private void blockIfPaused() throws InterruptedException {
        try {
            // Grab a write lock. Will block if a read lock has been taken - i.e. we've been paused.
            writeLock.lockInterruptibly();
        } finally {
            // Release the lock immediately to avoid blocking when pause is called.
            writeLock.unlock();
        }
    }

    // Pause the work. NB: MUST be balanced by a resume.
    public void pause() {
        // We can wait for a lock here.
        readLock.lock();
    }

    // Resume the work. NB: MUST be balanced by a pause.
    public void resume() {
        // Release the lock.
        readLock.unlock();
    }

    // Stop.
    public void cancel() {
        // Stop everything.
        cancelled = true;
    }

    // Stop immediately (if param is true).
    public void cancel(boolean interrupt) {
        if (interrupt) {
            // Interrupt me.
            me.interrupt();
        } else {
            // Or cancel me.
            cancel();
        }
    }

    // Wait for completion.
    public void await() throws InterruptedException {
        // Wait 'till we've finished. NB: Will wait forever if you haven't instigated a cancel of some kind.
        while (me.isAlive()) {
            Thread.sleep(0);
        }
    }

    // Start - like a thread.
    public void start() {
        // Wrap me in a thread and fire the sucker up!
        new Thread(this).start();
    }

    // Get the exception that was thrown to stop the thread or null if the thread was cancelled.
    public Exception getThrown() {
        return thrown;
    }

    // Expose my Thread.
    public Thread getThread() {
        return me;
    }

    // Create this method to do stuff.
    // Calls to this method will stop when pause is called.
    // Any thrown exception stops the whole process.
    public abstract void step() throws Exception;

    // Factory to wrap a Stepper in a PauseableThread
    public static PauseableThread make(Stepper stepper) {
        StepperThread pauseableStepper = new StepperThread(stepper);
        // That's the thread they can pause/resume.
        return pauseableStepper;
    }

    // One of these must be used.
    public interface Stepper {

        // A Stepper has a step method.
        // Any exception thrown causes the enclosing thread to stop.
        public void step() throws Exception;
    }

    // Holder for a Stepper.
    private static class StepperThread extends PauseableThread {

        // The actual stepper I am proxying.
        private final Stepper stepper;

        StepperThread(Stepper stepper) {
            this.stepper = stepper;
        }

        @Override
        public void step() throws Exception {
            stepper.step();
        }
    }

    // !!!! Testing only below !!!!
    // My test counter.
    static int n = 0;

    // Test/demo.
    public static void main(String[] args) throws InterruptedException {

        try {
            // Simple stepper that just increments n.
            Stepper s = () -> {
                n += 1;
                Thread.sleep(1);
            };
            PauseableThread pt = PauseableThread.make(s);
            // Start it up.
            pt.start();
            Thread.sleep(1000);
            pt.pause();
            System.out.println("Paused: " + n);
            Thread.sleep(1000);
            System.out.println("Resuminng: " + n);
            pt.resume();
            Thread.sleep(1000);
            pt.cancel();
            pt.await();
            System.out.println("Finished: " + n);

            // Start again to test agressive cancelling.
            n = 0;
            pt = PauseableThread.make(s);
            // Start it up.
            pt.start();
            Thread.sleep(1000);
            pt.pause();
            System.out.println("Paused: " + n);
            Thread.sleep(1000);
            System.out.println("Resuminng: " + n);
            pt.resume();
            Thread.sleep(1000);
            // Cancel aggressively.
            pt.cancel(true);
            pt.await();
            System.out.println("Finished: " + n);
            System.out.println("thrown: " + pt.getThrown());

        } catch (InterruptedException e) {
        }
    }
}

"Extending Thread is considered bad practice." ...instead, you should `implement Runnable` and create a new Thread from there. Or use something that creates new threads for you, such as an `ExecutorService`. — Powerlord, Dec 15 '15 at 16:25
Your solution is over complicated. Simple CountdownLatch shared between two threads will solve the problem of wait and notify. — Gaskoin, Dec 16 '15 at 08:14

score 0 · Accepted Answer · answered Dec 15 '15 at 16:12

The main problem you have is that only one thread can hold a lock. This means while your notify() thread holds the lock, no other thread can be running in a block of code which holds that lock.

Move the synchronized block inside the if (isPaused) block so another thread can run in between.

Another problem you have is that your isPaused boolean is not volatile so it can be inlined, i.e. it might never stop. It shouldn't be static even if you know you will never have more than one of these, it's bad practice to use a static field for an instance variable.

When paused you should cause the CPU to sleep.

Whenever you notify() or notifyAll() this should cause a state changes your wait() should always check for that state change.

notify() and wait() not working in Java

2 Answers2