CODEWITHSUNDEEP
javamultithreading
Uprooted
Uprooted

Reputation: 971

java, synchronized in two separate methods?

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.

Upvotes: 0

Views: 122

Answers (1)

Turing85
Turing85

Reputation: 20185

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(...) notifies one wait()ing thread (if any), so it can fight for the lock as soon as the lock is released by the current owner. Notice that a call to notify() does not release the intrinsic lock. It just wakes up a wait()ing thread. This can be made observable with the following code snippet:

class Ideone {
  private static final Object lock = new Object();

  public static void main(String[] args) {
    printWithThreadNamePrefix("Start");
    Thread waiter = new Thread(Ideone::waiter);
    waiter.start();

    // Give waiter some time to a) start and b) acquire the intrinsic lock
    try {
      Thread.sleep(500);
    } catch (InterruptedException e) {
    }

    final Thread notifier = new Thread(Ideone::notifier);
    notifier.start();

    while (true) {
      try {
        waiter.join();
        break;
      } catch (InterruptedException e) {
      }
    }

    printWithThreadNamePrefix("End");
  }

  private static void waiter() {
    synchronized (lock) {
      printWithThreadNamePrefix("Waiting...");
      while (true) {
        try {
          lock.wait();
          break;
        } catch (InterruptedException e) {
        }
      }
      printWithThreadNamePrefix("... done waiting");
    }
  }

  private static void printWithThreadNamePrefix(String msg) {
    System.out.println(String.format(
        "%s: %s",
        Thread.currentThread().getName(),
        msg));
  }

  private static void notifier() {
    synchronized (lock) {
      printWithThreadNamePrefix("notifying");
      lock.notify();
      while (true) {
      }
    }
  }
}

Ideone demo

The program will never terminate. Although thread two calls notify(), it then enters an endless loop, never actually releasing the intrinsic lock. Thus, one never has a chance to acquire the intrinsic lock, and the program "hangs" (it is neither a deadlock, nor a livelock, it simply cannot proceed).

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)

Upvotes: 6

Related Questions