Guarded Objects & NotifyAll()

Ok I must have missed sometime very obvious. I�m trying to create a guarded Thread() that will only execute when the Thread() counter is less than set max value.

The first three threads fall into the execute state and the next three threads will go into a wait state. This is what I wanted; the issue is on completion of an executing thread, I expected that the NotifyAll() would wake one of the waiting threads but it�s not?

thanxs

public class GuardedObject extends Thread {

private String name;
private static final int maxNumberOfActiveThreads = 3;
private static int currentNumberOfActiveThreads = 0;

GuardedObject(String name) {
this.name = name;
}

public void run() {
boolean loopControl = true;
while (loopControl) {
try {
if (currentNumberOfActiveThreads < maxNumberOfActiveThreads) {
currentNumberOfActiveThreads++;
System.out.println(this.name + ": Running!!");
for (int i = 0; i < 1000; i++) {
System.out.println(this.name + " :" + i);
}
synchronized (this) {
currentNumberOfActiveThreads--;
loopControl = false;
notifyAll();
System.out.println(this.name + ": Current number of active threads are: " + currentNumberOfActiveThreads);
}
} else {
synchronized (this) {
System.out.println(this.name + ": Too many active Threads");
wait();
}
}
} catch (InterruptedException e) {
System.out.println(this.name + ": I'm going to try again!");
}
}
}

public static void main(String[] args) {
GuardedObject a = new GuardedObject("a");
GuardedObject b = new GuardedObject("b");
GuardedObject c = new GuardedObject("c");
GuardedObject d = new GuardedObject("d");
GuardedObject e = new GuardedObject("e");
GuardedObject f = new GuardedObject("f");
a.start();
b.start();
c.start();
d.start();
e.start();
f.start();
}
}

I would say the problem is that notifyAll is being executed on a different object (a different instance of the GuardedObject class) from the objects on which wait calls are being executed. So if object "d" is waiting, via effectively a call to this.wait(), it needs a call to the notifyAll method of the same object, "d". A call to the notifyAll() method of object "a" will not do the trick.

I agree. Try adding another static variable, say a String called "lock". Synchronize on lock and then lock.notifyAll() when you're done. Since it's static, all instances will be synchronizing on the same object.

Going at another angle ... if your true objective is to never run more than three of something at a time, look into thread pooling with Executor in JDK 5. You set up an executor with three threads and put any number of commands into a queue. Each thread will run one command and then try to get another command from the queue. As an added advantage you set up and tear down only three threads, not six (or six thousand depending on your application). It's generally cheaper to create more commands and fewer threads.
[ December 30, 2004: Message edited by: Stan James ]

Thanxs for your suggestions! I have modified the code base on your comments and it not works great!

Here is the final version & again thanxs 4 the help..

public class GuardedObject extends Thread {

private String name;
private static final int maxNumberOfActiveThreads = 3;
private static int currentNumberOfActiveThreads = 0;
private static Object lock = new Object();

GuardedObject(String name) {
this.name = name;
}

public void run() {
boolean loopControl = true;
while (loopControl) {
try {
if (currentNumberOfActiveThreads < maxNumberOfActiveThreads) {
currentNumberOfActiveThreads++;
System.out.println(this.name + ": Running!!");
for (int i = 0; i < 1000; i++) {
System.out.println(this.name + " :" + i);
}
synchronized (lock) {
currentNumberOfActiveThreads--;
loopControl = false;
lock.notifyAll();
System.out.println(this.name + ": Current number of active threads are: " + currentNumberOfActiveThreads);
}
} else {
synchronized (lock) {
System.out.println(this.name + ": Too many active Threads");
lock.wait();
}
}
} catch (InterruptedException e) {
System.out.println(this.name + ": I'm going to try again!");
}
}

public static void main(String[] args) {
GuardedObject a = new GuardedObject("a");
GuardedObject b = new GuardedObject("b");
GuardedObject c = new GuardedObject("c");
GuardedObject d = new GuardedObject("d");
GuardedObject e = new GuardedObject("e");
GuardedObject f = new GuardedObject("f");
a.start();
b.start();
c.start();
d.start();
e.start();
f.start();
}

Thanxs for your suggestions! I have modified the code base on your comments and it not works great!

It may works great -- or not, but there are definitely problems... Take a look at this code snippet.

You are reading and changing the currentNumberOfActiveThreads variable, but you are not protecting it with the lock you created. The scope that you are using to grab the lock for the wait/notify method is too small. It either needs to be increased to encompass the changing variables -- or new locks needs to be created. (I would choose increasing the scope in this case)

Henry

Another point, there is no reason to use notifyAll(). The notify() method is good enough. An exiting thread only needs to wake up one thread to replace it.

Henry