java.util.concurrent (JDK 1.5) problem

[Ake L]

Someone knows what's wrong with this code? When I run it, sometimes all
threads finishes, sometimes not. Why?

When all is OK I get the following output:

Main thread enters sleep...
pool-1-thread-1 enters await...
pool-1-thread-4 enters await...
pool-1-thread-5 enters await...
pool-1-thread-3 enters await...
pool-1-thread-2 enters await...
Main thread woke up from sleep
Signalling all...
pool-1-thread-1 woke up from await
pool-1-thread-1 releases the lock
pool-1-thread-4 woke up from await
pool-1-thread-4 releases the lock
pool-1-thread-5 woke up from await
pool-1-thread-5 releases the lock
pool-1-thread-3 woke up from await
pool-1-thread-3 releases the lock
pool-1-thread-2 woke up from await
pool-1-thread-2 releases the lock
Terminated.

When it goes wrong, the code could hang after for instance
"pool-1-thread-4 releases the lock".

Here's the code:

------------------------
public class TestMain {

public static void main(String[] args) throws InterruptedException
{
ExecutorService executor = Executors.newCachedThreadPool();
CompletionService<Object> completionService = new
ExecutorCompletionService<Object>(executor);

for (int i = 0; i < 5; i++) {
completionService.submit(new TestThread());
}

System.out.println("Main thread enters sleep...");
Thread.sleep(1000);
System.out.println("Main thread woke up from sleep");

ALockingClass.getInstance().signal();

for (int i = 0; i < 5; i++) {
completionService.take();
}
executor.shutdown();
System.out.println("Terminated.");
}
}
------------------------
public class TestThread implements Callable<Object> {

public Object call() throws Exception {
ALockingClass.getInstance().waitForCondition();
return null;
}
}
------------------------
public class ALockingClass {

private static ALockingClass instance = null;

private final Lock lock = new ReentrantLock();
private final Condition aCondition = lock.newCondition();

public static ALockingClass getInstance() {
if (instance == null) {
instance = new ALockingClass();
}
return instance;
}

public void waitForCondition() throws InterruptedException {
lock.lock();
try {
System.out.println(Thread.currentThread().getName() + "
enters await...");
aCondition.await();
System.out.println(Thread.currentThread().getName() + "
woke up from await");
}
finally {
System.out.println(Thread.currentThread().getName() + "
releases the lock");
lock.unlock();
}
}

public void signal() {
lock.lock();
try {
System.out.println("Signalling all...");
aCondition.signalAll();
}
finally {
lock.unlock();
}
}
}
------------------------

 

[hiwa]

I just glanced through your code, didn't compile it nor run.
I am rather unfamiliar with new Java concurrency gadgets details.
Your locking class:
Why it should be a singleton?
Why it's factory is not concurrency aware?

 

[Ake L]

Why it should be a singleton?
Just to make sure all threads calls the same "waitForCondition" method
and references the same lock.
My idea was that since it is actually only of interest the first time
someone accesses (and therefor creates a new object) it, I didn't
regarded it important.
But it actually turned out that this was the problem! If two threads
get different instances then my code won't work (see the previous
comment).
So thanks for the comment. Problem solved!

 

[Stefan Schulz]

Just to make sure all threads calls the same "waitForCondition" method
and references the same lock.

My idea was that since it is actually only of interest the first time
someone accesses (and therefor creates a new object) it, I didn't
regarded it important.
But it actually turned out that this was the problem! If two threads
get different instances then my code won't work (see the previous
comment).
So thanks for the comment. Problem solved!

This is why i tend to initialize singletons statically. This way, this
nasty bit of unpleasantness can not happen, and you need not synchronize
in the getInstance method.

 

[Fiz]

I guess your main thread calls the executor.shutdown() before even the
callable tasks added to the executorservice completes. Try using a
barrier (java.util.concurrent.CyclicBarrier) to wait for all the
submitted tasks to complete before calling the shutdown.

 

[Ake L]

Very good point, I never thought of that.

 

[Ake L]

I meant Stefan's comment about initializing singletons statically.

 

[Ake L]

I guess your main thread calls the executor.shutdown() before even the

callable tasks added to the executorservice completes. Try using a
barrier (java.util.concurrent.CyclicBarrier) to wait for all the
submitted tasks to complete before calling the shutdown.

I thought that was what the CompletionService.take() does?

From the API docs:

"Retrieves and removes the Future representing the next completed task,
waiting if none are yet present."