Controlling thread using wait() and notify()

Question

(Problem solved, solution below)
I have 2 classes: Equip and Command. The equip is an equipment that run commands, but I need it to be able to run only 1 command at the same time. A command is a thread, that executes on the run() function, while Equip is a normal class that don't extend anything. Currently I have the following setup to run the commands:

Command class:

@Override
public void run() {
    boolean execute = equip.queueCommand(this);
    if (!execute) {
        // if this command is the only one on the queue, execute it, or wait.
        esperar();
    }
    // executes the command.....
    equip.executeNextCommand();
}


synchronized public void esperar() {
    try {
        this.wait();
    } catch (Exception ex) {
        Log.logErro(ex);
    }
}

synchronized public void continue() {
    this.notifyAll();
}

Equip class:

public boolean queueCommand(Command cmd) {
    // commandQueue is a LinkedList
    commandQueue.addLast(cmd);
    return (commandQueue.size() == 1);
}

public void executeNextCommand() {
    if (commandQueue.size() >= 1) {
        Command cmd = commandQueue.pollFirst();
        cmd.continue();
    }
}

However, this is not working. Basically, the notify() isn't waking the command thread, so it'll never execute. I searched about the wait and notify protocol, but I couldn't find anything wrong with the code. I also tried calling the wait() directly from the queueCommand() method, but then the execution of the queueCommand stopped, and it also didn't do what it was supposed to do. Is this approach correct and I'm missing something or this is completely wrong and I should implement a Monitor class to manipulate the concurrent threads?

EDIT: I solved the problem using another completely different approach, using Executors, thanks to @Gray.

Here's the final code, it might help someone someday:

Equip class:

private ExecutorCompletionService commandQueue = new ExecutorCompletionService(Executors.newFixedThreadPool(1));

public void executeCommand(Command cmd, boolean waitCompletion) {
    commandQueue.submit(cmd, null);
    if (waitCompletion) {
        try {
            commandQueue.take();
        } catch (Exception ex) {
        }
    }
}

In the Command class I just have a method to encapsulate the equip's execute method. The boolean waitCompletion is used when I need the result of the command at the same time, and instead of calling a new thread to execute it, I just execute and wait, pretending that it's executing on the same thread. This question contains a good discussion on this matter: When would you call java's thread.run() instead of thread.start()?. And yes, this is a case where it's useful to call .run() instead of .start().

Answer 1

There are a large number of race conditions that exist in your code if Command.run() is called from multiple threads. Unless this is some sort of homework question where you have to implement the code yourself, I would highly recommend using one of the Java Executors which were added in 1.6. In this case the Executors.newSingleThreadExecutor() is what you need to limit the number of running background tasks to 1. This will allow an unlimited number of tasks to be submitted to the ExecutorService, but only one of those tasks will be executing at any one time.

If you need the thread that is submitting the tasks to block when another task is already running then you would use something like the following. This sets up a pool of a maximum of 1 thread and uses a SynchronousQueue which blocks until the worker thread consumes the job:

final ExecutorService executorServer =
    new ThreadPoolExecutor(0, 1, 60L, TimeUnit.SECONDS,
         new SynchronousQueue<Runnable>());

But if that was the case then you would just call the task directly inside of a synchronized block and you wouldn't need the ExecutorService.

Lastly, for any new concurrency programmer (of any language) I would recommend that you take the time to read some documentation on the subject. Until you start recognizing the concurrent pitfalls inherent in threading even the simplest set of classes, it will be a frustrating process to get your code to work. Doug Lea's book is one of the bible's on the subject. My apologies if I have underestimated your experience in this area.

Answer 2

I think you should not have "synchronized" on the esperar method. That will block using the object instances as the locking object. Any other thread that attempts to wait will block AT ENTRY TO THE METHOD, not on the wait. So, the notifyAll will release the one thread that got into the method first. Of the remaining callers, only one will proceed with a call to esperar, which will then block on the wait(). Rinse and repeat.

Answer 3

ExectutorService is the way to go. But if you want to do-it-yourself, or need to do something fancier, I offer the following.

I gather than this whole thing is driven by Equip's queueCommand, which might be callled from any thread anywhere at any time. For starters, the two methods in Equip should by synchronized so commandQueue does not get trashed. (You might use ConcurrentLinkedQueue, but be careful with your counts.) Better still, put the code in each method in a block synchronized by queueCommand.

But further, I think your two classes work better combined. Switching Command to a simple Runnable, I'd try something like this:

class Equip  {
    private Object  queueLock = new Object();  // Better than "this". 
    private LinkedList<Runnable>  commandQueue = new LinkedList<Runnable>();

    private void run() {
        for (;;)  {
            Runnable  cmd = equip.getNextCommand();
        if (cmd == null)  {
                // Nothing to do.
                synchronized (queueLock)  { queueLock.wait(); }
            }
            else
                cmd.run();
        }
    }
    // Adds commands to run.
    public boolean queueCommand( Runnable cmd )  {
        synchronized (queueCommand)  { commandQueue.addLast( cmd ); }
        synchronized (queueLock)  {
            // Lets "run" know queue has something in it if it
            // is in a wait state.
            queueLock.notifyAll();
        }
    }
    private Runnable getNextCommand()  {
        synchronized (queueCommand)  { return commandQueue.pollFirst(); }
    }
}

You'll need to catch some exceptions, and figure out how to start things up and shut them down, but this should give an idea of how the wait and notify work. (I'd look for some way to know when "run" was not waiting so I could skip synching on queueLock in queueCommand, but walk before you run.)