Differences between synchronized keyword and ReentrantLock [duplicate]

Question

This question already has answers here:

What's the difference between Synchronized and Lock in my example? (2 answers)

What's the difference in using ReentrentLock and Synchronized(object)? [duplicate] (1 answer)

Closed 4 years ago.

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.

Answer 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
}

Answer 2

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().

Answer 3

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.