Java concurrency: some assistance

Question

I have a class like this:

package crossRoadConcurency;

import java.util.List;

public class TourGuide 
{
    private volatile boolean isGuiding;
    private volatile boolean isInShop;
    private final Object lockObject = new Object();
    private final int id;

    public TourGuide(int id)
    {
        this.isGuiding=false;
        this.isInShop=false;
        this.id=id;
    }

    public synchronized boolean isFree()
    {
        return !isGuiding && !isInShop;
    }

    public void guide(final Tourist[] tourists)
{
    new Thread(new Runnable() 
    {   
        @Override
        public void run() 
        {
            synchronized (lockObject) 
            {
                while(!isFree()) 
                {
                    try 
                    {
                        System.out.println("Guide "+id+" is bussy. Waiting... ");
                        lockObject.wait();
                    } 
                    catch (InterruptedException e) 
                    {
                        e.printStackTrace();
                    }
                }
            }
            isGuiding=true;
            System.out.println("Guide "+id+" is guiding "+tourists.length+" tourists");
            try 
            {
                Thread.sleep(4000);//lets not wait one hour, shall we?
                for (Tourist tourist : tourists) 
                {
                    tourist.exit();
                }
                System.out.println("All tourists exited for guide "+id+". Going to shop");
                isInShop=true;
                isGuiding=false;//if we invert the way we give these values bad thing may happen
                Thread.sleep(4000);
                isInShop=false;
                System.out.println("Guide "+id+" is free");
                synchronized (lockObject) 
                {
                    lockObject.notifyAll();
                }
            } 
            catch (InterruptedException e) 
            {
                e.printStackTrace();
            }   

        }
    }).start();
}
}

And another class:

package crossRoadConcurency;

import java.util.Random;

public class Tourist 
{
    public void exit() throws InterruptedException
    {
        Random random = new Random();
        Thread.sleep(random.nextInt(1000));// max one second to exit
    }
}

Which I use like this:

package crossRoadConcurency;

import java.util.List;

public class Main 
{

    public static void main(String[] args) throws InterruptedException
    {
        Tourist[] tourists = new Tourist[20];
        for (int i=0;i<tourists.length;i++) 
        {
            tourists[i]=new Tourist();
        }
        TourGuide guide = new TourGuide(0);
        guide.guide(tourists);
            Thread.sleep(500);
        guide.guide(tourists);

    }

}

The problem is I get this output:

Guide 0 is guiding 20 tourists
All tourists exited for guide 0. Going to shop
Guide 0 is free
Exception in thread "Thread-0" Guide 0 is guiding 20 tourists
java.lang.IllegalMonitorStateException
    at java.lang.Object.notifyAll(Native Method)
    at crossRoadConcurency.TourGide$1.run(TourGide.java:60)
    at java.lang.Thread.run(Unknown Source)
All tourists exited for guide 0. Going to shop
Guide 0 is free
Exception in thread "Thread-1" java.lang.IllegalMonitorStateException
    at java.lang.Object.notifyAll(Native Method)
    at crossRoadConcurency.TourGide$1.run(TourGide.java:60)
    at java.lang.Thread.run(Unknown Source)

First question: Why am I not seeing the "waiting output"
Second question: Why am I getting an exception
Third question: Is there a better way to do this, because I am fairly certain this concept is mixed up

Answer 1

1. You don't see "wait" because your guide is free by default. !isGuide && !isInShop returns true while you only print when !isFree().

2. You should call lockObject.notifyAll(). A direct call to notifyAll() by default invoke on this object and you certainly didn't hold the monitor of the anoymous object implementing Runnable interface because you never did by calling synchroized(this). That's why you got the exception.

3. Yes. You should use an Executor e.g. SingleThreadExecutor together with a concurrent queue. That's the robust and easy way to implement concurrency unless you goes on high performance computing. That package also provide excellent functinoalities and supporting utilitiy classes. Check out java.util.concurrent.

BTW, your package name contains Upper letter which isn't recommended by java programming specification.

Answer 2

when we synchronize any block or method, we should synchronize on class lock or object lock.

but here you have locked on some object which is no where related to caller of that method

so change this from synchronized (lockObject) to like below and run

synchronized (this)