I'm trying to create thread safe queue in java. I've come across this example:

class ProducerConsumer<T> {
   private static final int BUFFER_MAX_SIZE = 42;
   private List<T> buffer = new LinkedList<>();

   synchronized void produce(T value) throws InterruptedException {
      while (buffer.size() == BUFFER_MAX_SIZE) {
         wait();
      }
      buffer.add(value);
      notify();
   }

   synchronized T consume() throws InterruptedException {
      while (buffer.size() == 0) {
         wait();
      }
      T result = buffer.remove(0);
      notify();
      return result;
   }
}

I'm new to java. In my understanding those two 'synchronized' keywords would prevent contention inside each method, but not when both methods are called simultaneously. E.g. thread P calls produce, locks method, thread C calls consume, locks other method, then one tries to extract element from list, another tries to insert element, thread exception arises.

My question: Is this example broken?

Or maybe I'm missing something and it's ok.

CodePudding user response：

JLS, §17.1 is quite explicit about the mechanism:

...

A synchronized method (§8.4.3.6) automatically performs a lock action when it is invoked; its body is not executed until the lock action has successfully completed. If the method is an instance method, it locks the monitor associated with the instance for which it was invoked (that is, the object that will be known as this during execution of the body of the method). If the method is static, it locks the monitor associated with the Class object that represents the class in which the method is defined. If execution of the method's body is ever completed, either normally or abruptly, an unlock action is automatically performed on that same monitor.

...

Thus, it is guaranteed that at one point in time on one object at most one thread is executing either produce(...) or consume(). It is not possible that, at one point in time, one thread executes produce(...) on an object while another thread executes consume() on the same object.

The call to wait() in consume() releases the intrinsic lock and blocks execution. The call to notify() in produce(...) releases the intrinsic lock and notifies one wait()ing thread (if any), so it can lock the intrinsic lock and continue execution.

The things I recommend to change are:

• declare private List<T> buffer additionally as final
• call notifyAll() instead of notify() in order to wake all waiting threads (they will still execute sequentially, for details see this question by Sergey Mikhanov and its answers)