IllegalMonitorStateException while implementing producer consumer using RentrantLock

Question

im trying to implement producer consumer using ReentrantLock in java

    Condition producerlock = lock.newCondition();
    Condition consumerlock = lock.newCondition();

it has two conditions one for producer and another for consumer .

Here we have a processor class with two methods producer consumer and one stack

   Stack<Integer> hellostrack = new Stack<>();



 public void produce() throws InterruptedException {
        lock.tryLock();
        System.out.println("inside producer method");
        while (true) {
            try {

                if (hellostrack.size() > 8) {
                    System.out.println("stack is full its time for me to go to sleep");
                    producerlock.await();
                }
                System.out.println("thread is alive and kicking");
                hellostrack.add(new Random().nextInt());
                consumerlock.signalAll();


            } finally {
                System.out.println("Exception occours in producer Thread");
                lock.unlock();
            }
        }
    }


public void consume() throws InterruptedException{
             System.out.println("inside consumer method");
             lock.tryLock();
          try {
              while (true) {
                  if (hellostrack.isEmpty()) {
                      System.out.println("stack is empty im going to sleep");
                      consumerlock.await();

                  } else {
                      System.out.println("poping elelmts from stock" + hellostrack.pop());
                      consumerlock.signalAll();


                  }

          } }finally {
              System.out.println("Exception occours at consumer");
              lock.unlock();
          }
     }

as you can see when stack reaches a certain limit producer will go to sleep , same for consumer when stack is empty

but when i run them in two thread

Processor p  = new Processor();
        Thread t1 = new Thread(new Runnable() {
            @Override
            public void run() {
                try {
                    p.consume();
                } catch (InterruptedException e) {
                    e.printStackTrace();
                }
            }
        });

        Thread t12 = new Thread(new Runnable() {
            @Override
            public void run() {
                try {
                    p.produce();
                } catch (InterruptedException e) {
                    e.printStackTrace();
                }
            }
        });

        t1.start();
        t12.start();



i get illegal state exception 


inside consumer method
stack is empty im going to sleep
inside producer method
thread is alive and kicking
Exception occours in producer Thread
thread is alive and kicking
Exception occours in producer Thread
Exception in thread "Thread-1" java.lang.IllegalMonitorStateException
    at java.base/java.util.concurrent.locks.ReentrantLock$Sync.tryRelease(ReentrantLock.java:149)
    at java.base/java.util.concurrent.locks.AbstractQueuedSynchronizer.release(AbstractQueuedSynchronizer.java:1302)
    at java.base/java.util.concurrent.locks.ReentrantLock.unlock(ReentrantLock.java:439)
    at Processor.produce(Processor.java:30)
    at Processor$2.run(Processor.java:76)
    at java.base/java.lang.Thread.run(Thread.java:834)
poping elelmts from stock891164354
poping elelmts from stock-1958956829
stack is empty im going to sleep

Answer 1

Notice that you are in a while(true) loop. You never leave the loop which means you unlock then never re-lock. After the first loop, you will then try unlock again but since you don't own the lock it will throw an exception.

Move the tryLock under the while(true) and will probably work better.

Answer 2

In addition to @JohnVint's answer, there are a few other issues with your code.

1. You are using Lock.tryLock():

   Acquires the lock only if it is free at the time of invocation.

   Acquires the lock if it is available and returns immediately with the value true. If the lock is not available then this method will return immediately with the value false.

   A typical usage idiom for this method would be:

   Lock lock = ...;
   if (lock.tryLock()) {
     try {
        // manipulate protected state
     } finally {
       lock.unlock();
     }
   } else {
     // perform alternative actions
   }
   

   This usage ensures that the lock is unlocked if it was acquired, and doesn't try to unlock if the lock was not acquired.

   Your code doesn't check the result of tryLock which means a thread has a chance of moving into the guarded code without holding the lock. This means there's a chance of calling await(), signalAll(), and unlock() without holding the lock—in addition to the improperly synchronized access.

   The method you want to call in this situation is Lock.lock():

   Acquires the lock.

   If the lock is not available then the current thread becomes disabled for thread scheduling purposes and lies dormant until the lock has been acquired.

   This means the thread will wait until it acquires the lock before moving on. However, since your method is already throwing InterruptedException you might as well use Lock.lockInterruptibly(). It's basically the same thing as lock() but the wait can be interrupted.

2. You're calling consumerlock.signalAll() inside your consume() method.

   Once you consume an element you want to notify the producers that there is more room available. You should be calling producerlock.signalAll().

3. You don't call await() inside a loop.

   It's good practice to call await() inside a loop that checks the condition. The reason is because a thread can wake up for whatever reason (rarely). If that happens, and there's a loop, the thread will recheck the condition and, if appropriate, will call await() again.

   Additionally, you're using signalAll(). That method notifies all waiting threads to wake up, try to acquire the lock, and move on. Since you don't use a loop all threads that wake up will simply move on to perform any modifications—probably leading to inconsistent/incorrect state. In contrast, having loop would mean if one of the woken up threads causes the wait condition to be true again any subsequent threads will go back to waiting.

   Using loops would look something like:

   while (hellostrack.size() > 8) { // should this be >= 8?
       producerlock.await();
   }
   
   // and
   
   while (hellostrack.isEmpty()) {
       consumerlock.await();
   }