Java thread program not working using wait() and notifyAll()

Question

Below is my program. Always the thread 0 gets the printer, other threads do not get it. There is one printer object, and i want multiple job threads to use the printer. How to make this program work so that all jobs get the printer. For me the code flow seems to be fine. Am synchronizing on a single printer object. Please help.

    package classesTesting;

    public class PrinterQueue {

        final static Printer printer = new Printer();;

        public static void main(String[] args) {
            // TODO Auto-generated method stub
            System.out.println("In Main");

            for (int i = 0; i < 5; i++) {
                new Thread(new Jobs(), "Thread - " + i).start();
                System.out.println("started " + i + " thread");
            }

        }

    }

    class Printer {
        private boolean isUsed;

        Printer() {
            this.isUsed = false;
        }

        public void setUsed(boolean used) {
            this.isUsed = used;
        }

        public boolean isUsed() {

            return this.isUsed;
        }
    }

    class Jobs implements Runnable {

        String name;
        boolean isDataAvailble;

        Jobs() {        
            this.isDataAvailble = true;
        }

        public void setNoData(boolean noData) {
            this.isDataAvailble = false;
        }

        @Override
        public void run() {

            while (isDataAvailble) {

                if (PrinterQueue.printer.isUsed()) {
                    try {
                        System.out.println(Thread.currentThread()
                                + "WAITING FOR PRINTER");
                        synchronized (PrinterQueue.printer) {
                            PrinterQueue.printer.wait();
                        }
                    } catch (InterruptedException e) {
                        e.printStackTrace();
                    }

                } else {
                    synchronized (PrinterQueue.printer) {
                        System.out.println(Thread.currentThread() + "GOT PRINTER");
                        PrinterQueue.printer.setUsed(true);
                        try {
                            Thread.sleep(3000);
                        } catch (InterruptedException e) {
                            e.printStackTrace();
                        }
                        PrinterQueue.printer.setUsed(false);
                        PrinterQueue.printer.notify();
                    }
                }
            }

            try {
                Thread.sleep(3000);
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        }

    }

Hi, I have revised my program for getting lock first then condition checking. Even then the thread 0 always gets the printer. Other threads starve.

Revised program:

    package classesTesting;

    public class PrinterQueue {

        static Printer printer;

        public static void main(String[] args) {
            // TODO Auto-generated method stub
            System.out.println("In Main");

            printer = new Printer();

            for (int i = 0; i < 5; i++) {
                Jobs j1 = new Jobs();
                j1.setPrinter(printer);

                Thread t1 = new Thread(j1, "Thread - " + i);
                t1.start();

                System.out.println("started " + i + " thread");
            }

        }

    }

    class Printer {
        private boolean isUsed;

        Printer() {
            this.isUsed = false;
        }

        public void setUsed(boolean used) {
            this.isUsed = used;
        }

        public boolean isUsed() {

            return this.isUsed;
        }
    }

    class Jobs implements Runnable {

        String name;
        Printer printer;

        public Printer getPrinter() {
            return printer;
        }

        public void setPrinter(Printer printer) {
            this.printer = printer;
        }

        boolean isDataAvailble;

        Jobs() {
            this.isDataAvailble = true;
        }

        public void setNoData(boolean noData) {
            this.isDataAvailble = false;
        }

        @Override
        public void run() {

            while (isDataAvailble) {
                synchronized (PrinterQueue.printer) {
                    if (this.printer.isUsed()) {
                        try {
                            System.out.println(Thread.currentThread()
                                    + "WAITING FOR PRINTER");

                            PrinterQueue.printer.wait();

                        } catch (InterruptedException e) {
                            e.printStackTrace();
                        }
                    }

                    else {

                        System.out.println(Thread.currentThread() + "GOT PRINTER");

                        PrinterQueue.printer.setUsed(true);

                        try {
                            Thread.sleep(3000);
                        } catch (InterruptedException e) {
                            e.printStackTrace();
                        }

                        PrinterQueue.printer.setUsed(false);
                        PrinterQueue.printer.notify();
                    }
                }
            }

        }

    }

Answer 1

It should look like this:

1. Acquire the printer:

   synchronized (PrinterQueue.printer) {
       while (PrinterQueue.printer.isUsed()) {
           try {
               System.out.println(Thread.currentThread()
                               + "WAITING FOR PRINTER");
               PrinterQueue.printer.wait();
           } catch (InterruptedException e) {
               e.printStackTrace();
           }
       }
       System.out.println(Thread.currentThread() + "GOT PRINTER");
       PrinterQueue.printer.setUsed(true);
   }
   

2. Use the printer, dummied as per your code by Thread.sleep():

   try {
       Thread.sleep(3000);
   } catch (InterruptedException e) {
       e.printStackTrace();
   }
   

3. Release the printer:

   synchronized (PrinterQueue.printer) {
       PrinterQueue.printer.setUsed(false);
       PrinterQueue.printer.notifyAll();
   }
   

You need to use while rather than if, and you need to test the same object you're synchronized on. And use notifyAll() rather than notify().

But it isn't clear to me that you need any of this, just a synchronized block.

Answer 2

I think what you are looking for is a Condition. You first need to obtain a lock, then you can check a condition. While that condition hold the thread will sleep. When the condition no longer holds the sleeping thread (or next sleeping thread) is woken up to check the condition again.

You can read more about the Condition object here: http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/locks/Condition.html

Answer 3

If you want the resource to be available for all the threads in fair manner, it's much better to use ReentrantLock with fair = true parameter. Also never rely on non-volatile variables changed in concurrent way. Here's the fixed code:

import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;

public class PrinterQueue {
    static Printer printer;

    public static void main(String[] args) {
        System.out.println("In Main");
        printer = new Printer();
        for (int i = 0; i < 5; i++) {
            // I added printer constructor parameter to pass the same printer
            // to all the Jobs
            new Thread(new Jobs(printer), "Thread - " + i).start();
            System.out.println("started " + i + " thread");
        }
    }
}

class Printer {
    // internally printer holds a fair ReentrantLock
    Lock lock = new ReentrantLock(true);

    // call this to get the printer
    public void acquire() {
        lock.lock();
    }

    // call this to release the printer, so it's available for other threads
    public void release() {
        lock.unlock();
    }
}

class Jobs implements Runnable {
    // Declare isDataAvailble as volatile as you're going to change it from another thread
    volatile boolean isDataAvailble;
    private final Printer printer;

    // constructor now takes the printer argument
    Jobs(Printer printer) {
        this.isDataAvailble = true;
        this.printer = printer;
    }

    @Override
    public void run() {
        try {
            while (isDataAvailble) {
                System.out.println(Thread.currentThread()
                        + "Trying to get the printer");
                // get the printer
                this.printer.acquire();
                try {
                    System.out.println(Thread.currentThread()
                            + "Printer acquired!");
                    // use it
                    Thread.sleep(3000);
                } finally {
                    // Release the printer. Better to do it in finally block
                    // so you will release it even if some unexpected exception occurs
                    this.printer.release();
                }
            }

            Thread.sleep(3000);
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
    }
}