Simple thread problem in java

Works except for when I free the crawler:

public void setCrawlerFree(WebCrawler w)
    {
        synchronized(myFreeCrawlers)
        {
            synchronized(numToGo)
            {
      开发者_如何学C          myFreeCrawlers.add(w);
                myFreeCrawlers.notifyAll();
                numToGo--;
                numToGo.notify();
            }
        }
    }

When the crawler is done, I can add it back on the list. I also want to subtract 1 from the number of things I still need to do. I have one main thread waiting until numToGo is at 0. I'm getting an IllegalMonitorStateException on numToGo.notify() but since its inside of the synchronization block, doesn't that mean I own it?

Consider rewriting it to ExecutorService.

ThreadPoolExecutor executor = new ThreadPoolExecutor(corePoolSize, 
     maximumPoolSize, keepAliveTime, timeUnit, 
     new LinkedBlockingQueue<Runnable>());
executor.submit(new Callable<...>() { ... });

It would greatly simplify your code and eliminate thread synchronization issues.

So I thought I needed to call wait and notify on the object that all the threads have in common, but that's not correct either.

Yes, it is. But:

public class IllegalMonitorStateException
extends RuntimeException

Thrown to indicate that a thread has attempted to wait on an object's monitor or to notify other threads waiting on an object's monitor without owning the specified monitor.

You need to synchronize on an object before calling wait() or notify() on it.

Is your numToGo field is a primitive type which is being wrapped ? (int to Integer, long to Long, etc). Remember these wrappers are immutable and will cause you to have different object every time the boxing/unboxing happens. It's always recommended to use final objects when synchronization is needed.

Instead of using and integer create your own object to maintain the value and synchronization.

class Counter {
    private int value ;
    private final Object lock = new Object() ;

    public ExecutionStatus() { }

    public void increment() {
        synchronized(lock) {
            value ++ ;
        }
    }

    public void decrease() {
        synchronized(lock) {
            value-- ;
        }
    }

    // Read dirty
    public int count() {
        return value ;
    }

    public int safeCount() {
        synchronize(lock) {
            return count() ;
        }
    }
}

Never the less, you can add the line private final Object lock = new Object() to your current code and use that to control the synchronization of the numToGo variable.

Hope this helps.

you are synchronising on a non-final member variable. Your sync(numToGo) syncs on some value of numToGo and then you change the reference: numToGo--. You now have a different instance on which you call notify, hence the exception.

Some good posts there, there are plenty of alternatives but I imagine this is some kind of academic exercise? As people have pointed out, you'd probably wouldn't use wait/notify/notifyAll when there are more modern alternatives that makes things easier. The wait/notify thing though is interesting and is well worth understanding as a basis for concurrency work.

I'm assuming this is some kind of consumer/producer thing? One thread is trapping a crawler, the other setting it free? If that's the case, you might want to wait for the trap to have occupants before setting free? it might look something like this...

  private final List<Object> trap = new ArrayList<Object>();

    public class BugCatcher {
        public void trapCrawler(Object crawler) {
            synchronized (trap) {
                trap.add(crawler);
                System.out.println("caught bug number " + trap.size() + "!");
                trap.notifyAll();
            }
        }
    }

    public class Hippy {
        public void setCrawlerFree(Object crawler) throws InterruptedException {
            synchronized (trap) {
                trap.wait();
                trap.clear();
                System.out.println("set bugs free! time to hug a tree");
            }
        }
    }

If the BugCatcher can catch bugs quicker than the hippy releases them, the hippy waits for the trap to have something in it before attempting to release the bugs (hence the wait call).

If you leave out the wait/notify part, things will rely just on the synchronized keyword, only one thread will access the trap at a time and its a race as to which gets there first (the hippy might try an empty an already empty trap).

In order to co-ordinate the wait and notify, the VM will use an object monitor. A thread acquires the object's monitor when it enters a synchronized block. An object has just a single monitor which acts as a mutually exclusivity lock (mutex). If you try and wait or notify without first getting the object's monitor (without executing wait or notify within a synchronized block), the VM can't set things up and so throws the IllegalMonitorException. It's saying "I can't allow this because if, for example, I wait, when will I know that I can progress when somebody calls notify? what/who are they notifying?". It uses the monitor to coordinate and so forces you to acquire the monitor.

So, the error you get is because numToGo isn't syncrhonised in the other thread (as Michael said previously).

I can't quite see why you need the numToGo, if it is producer/consumer, do you want to stop after a certain number? After the bug catcher catches 10 bugs and the hippy releases 10? Doesn't sound like that's what you're trying to do (as they could both have unrelated internal counters), so I'm not sure what you trying to do there. It'd be good to outline what you're trying to do in case I've gone off on completely the wrong tangent!