1

I made a thread pool based on the example on this page. In the worker thread we have the infinite loop that never lets the thread die and the wait() method call that pauses the thread when there is no work to do:

while (true) {
    synchronized(queue) {
         loop:while (queue.isEmpty()) { // labled the loop so we can return here
             try
             {
                 queue.wait();
                 if(queue.isEmpty())    // check the condition predicate again
                     continue loop;
             }
             catch (InterruptedException ignored)
             {
             }
         }    
         r = (Runnable) queue.removeFirst();
 }

// If we don't catch RuntimeException, 
// the pool could leak threads
try {
    r.run();
}
catch (RuntimeException e) {
    // You might want to log something here
}

The fact is that r = (Runnable) queue.removeFirst(); can throw a NoSuchElementException which is a RuntimeException if the queue is empty. And when such an exception is thrown on that line, the current thread holding the mutex dies and the pool leaks the thread. The mutex seems to be released when the thread dies.

However, if you instead of using the default synchronized keyword to synchronize the queue, use the ReentrantLock to lock and Condition for signaling and awaiting, the current thread that holds the mutex does not seem to release the lock when it interrupts unexpectedly.

So, in my case, when I checked with JVisualVM under the Threads tab I could see that the AWT-EventQueue-0 thread was wating for Thread-1 to release the mutex. But the Thread-1 died upon its way to run the task and was unexpectedly terminated (RuntumeException) and the mutex did not seem to be released.

My questions:

1) Does not ReentrantLocks being released if the thread that holds it terminates unexpectedly?

2) Is there any difference between while (queue.isEmpty()) { and if (queue.isEmpty()) { in the code snippet above? I cannot see any difference since the thread will wait in both cases. But I think the behaviour it different when using if (like if more than one thread can affect the queue).

EDIT
Java Concurrency in Practice states:

For all these reasons, when you wake up from wait you must test the condition predicate again, and go back to waiting (or fail) if it is not yet true. Since you can wake up repeatedly without your condition predicate being true, you must therefore always call wait from within a loop, testing thecondition predicate in each iteration.

Look at my edit in the code above, now the code should be correctly as stated in Java Concurrency in Practice.

Rox
  • 2,647
  • 15
  • 50
  • 85
  • notes about code and ignoring InterruptedException. How do you plan stoping you application? Do you use System.exit()? – Taky Feb 08 '13 at 06:45
  • second check is Queue empty in if statement is unnecessary. You will check the same condition before next while iteration – Taky Feb 09 '13 at 20:50
  • Notes: if you want your thread can be interrupted use lockInterruptibly()( http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/locks/Lock.html#lockInterruptibly() ) instead of normal lock(). – Taky Feb 09 '13 at 20:51

3 Answers3

4

1) Does not ReentrantLocks being released if the thread that holds it terminates expectedly?

Lock release only when you call Lock#unlock() explicitly. That why recommend to invoke Lock#unlock() in finally block to prevent deadlocks in your application.

2) Is there any difference between while (queue.isEmpty()) { and if (queue.isEmpty()) { in the code snippet above? I cannot see any difference since the thread will wait in both cases. But I think the behaviour it different when using if (like if more than one thread can affect the queue).

There, no big differences in particular situation. But using while you guarantee assert in your application, you will not invoke removeFirst() when Queue is empty. Also, PROS for using while instead of if is spurious wakeups.

notes: If you are implementing this schema not only for education, consider using BlockingQueue. java.util.concurrent library resolved many multithread problems and in most cases you can build application based on the high level abstractions of java.util.concurrent instead of using low-level techniques such as wait()/notify().

Community
  • 1
  • 1
Taky
  • 5,284
  • 1
  • 20
  • 29
  • I see, but it seems that a thread, when it interrupts unexpectedly, automatically releases its lock when using `synchronized` keyword, but not when using ReentrantLock. – Rox Feb 09 '13 at 11:48
  • I have never heard of spurious wakeups. Thanks for notifying me of that. Look at my edit in the code snippet above and the quote from Java Concurrency in Practice in the end of my post. Any comments on that edit? – Rox Feb 09 '13 at 12:16
1

Your code seems too complicated - I would simply write:

while (true) {
    synchronized(queue) {
         while (queue.isEmpty()) {
             try {
                 queue.wait();
             } catch (InterruptedException ignored) {
                 //don't ignore me please
                 //you probably should exit the loop and return here...
             }
         }
         r = queue.removeFirst(); //Why use a cast? Use generics instead.
     }
 }

The only situation where queue.removeFirst() could throw a NoSuchElementException is if it is modified concurrently, which is not possible if all accesses to the queue are made in synchronized blocks.

So find the place where you access the queue without holding the lock on the monitor and you will solve your problem.

The reason why you must call wait within a loop is that wait might wake spuriously (i.e. because wait wakes up does not mean your condition has become true so you need to test it again).

As a side note, if you used a BlockingQueue you would not have to worry about those low level details and you could simply write:

while(true) {
    Runnable r = queue.take(); //blocks until queue is not empty
}
assylias
  • 321,522
  • 82
  • 660
  • 783
  • But if wait() can wake up spuriously, shouldn't I have to check the condition just after the wait() as in my code example above? Is not it what you are saying yourself? – Rox Feb 10 '13 at 17:48
  • I have seen that threads in the pool in my application have tried to do queue.removeFirst() when the queue has been empty (and the queue is just used twice in my code, within synchrnoized blocks), so I assume it has been spuriously wakeups. And then I have to check the condition after the wait() again, right? – Rox Feb 10 '13 at 17:53
  • @Rox yes you can add an if + continue, but since it is strictly equivalent to a simple while, I don't see any reasons for the added clutter. – assylias Feb 10 '13 at 18:02
  • Also your code has unbalanced braces - I assumed that `r = queue.removeFirst();` was within the synchronized block. – assylias Feb 10 '13 at 18:03
  • Oh, ok! But if I used an if statement instead of the while loop, then I would have another if statement checking the condition after the wait() wakeup, right? – Rox Feb 10 '13 at 18:39
  • @Rox And if the second if is false (condition not true), how do you go back to the initial if to start waiting again? – assylias Feb 10 '13 at 18:41
0

Stuff in java.util.concurrent give you more flexibility and things like timed waits and try-lock/acquire methods. Also, synchronized gives you code block, unlike lock.lock()/unlock() pairs. It also tends to be more efficient when there is no contention. Anyway, when using concurrency, one should definitely look into java.util.concurrent since many problems are solved there already.

Ralf H
  • 1,392
  • 1
  • 9
  • 17