Java IllegalMonitorStateException on tryLock

I was wondering if someone could help me. I'm new to concurrent programming and I've got the following code which at times is giving me an IllegalMonitorStateException as a Thread that does not currently own the lock is trying to unlock.

I was told on the OTN forum that to prevent this and deadlock from occurring that I should make sure that the lowest indexes are locked first but I can't figure out how to do this. could one of you please point me in the right direction.

Many thanks.

         import java.util.concurrent.locks.*;
         class ShuffleBase {

         ReentrantLock [] locker;
         int [] data;
         int x;
         ReentrantLock lock1;
         ReentrantLock lock2;

         public ShuffleBase(int [] d){
         x=0;
        data=d;
        locker = new ReentrantLock[d.length];
        while (x!=data.length)
        {
         locker[x]=new ReentrantLock();   
          x++;
        }
        }


     /*
      *  Boolean method to test if both indexes are locked
      *  Returning their status for use in the method swap
      *  If locked the swap happens else the locks are 
      *  released
      */
      public boolean lockedIndex(int a, int b){

      Boolean lockA = false;

      Boolean lockB = false;

      try{

      lockA = lock1.tryLock();

      lockB = lock2.tryLock(); 
       }finally{

          if (!(lockA && lockB))
      {

       if(lockA){
         lock1.unlock();       
        }
       if (lockB){
        lock2.unlock();
         }  
           }// end of IF ! lockA & lockB

      } // End of finally
      return lockA && lockB;
    }// End of lockedIndex


     public void swap(int a, int b){

      int temp;

      lock1 = locker[a];
      lock2=locker[b];



      //If a & b aren't the same index swap
       if(a!=b)
        {   
        if(lockedIndex(a,b))
        {
         try{
          temp=data[b];

         data[b]=data[a];

         data[a]=temp;

          }finally{
            lock1.unlock(); 
                       lock2.unlock();

          }
         }
         else{System.out.println("Couldn't lock");}   

        }
       else{return;}

    }//EOF Method

    public void display(){
    System.out.println("The array when swapped");
    for(int j=0; j<data.length;j++)
    {
     System.out.print(data[j]+", ");
    }
    }// End of Display


     }// EOC

Thank you all for your answers, yes I am doing this as part of a self learning/homework. I've changed the code to the following as I believe that this is what Rodion suggested, I hope I interpreted your suggestion correctly. Mtraut I've also made the changes you suggested and I can't believe that I made these mistakes :-) Thanks for spotting them.

import java.util.concurrent.locks.*;
class ShuffleBase {

ReentrantLock [] locker;
int [] data;
int x=0;


public ShuffleBase(int [] d){
  data=d;
  locker = new ReentrantLock[d.length];
  while (x!=data.length)
   {
     locker[x]=new ReentrantLock();   
      x++;
   }
}


 /*
  * Boolean method to test if both indexes are locked
  *  Returning their status for use in the method swap
  *  If locked the swap happens else the locks are 
  *  released
  */
  private boolean lockedIndex(int a, int b){

   Boolean lockA = false;

   Boolean lockB = false;

    try{

        lockA = locker[a].tryLock();

        lockB = locker[b].tryLock(); 
        }finally{

         if (!(lockA && lockB))
             {

                 if(lockA){
                 locker[a].unlock();       
                 }

                 if (lockB){
                   locker[b].unlock();
                 }  
             }// end of IF ! lockA & lockB

       } // End of finally
           return lockA && lockB;
    }// End of lockedIndex


public void swap(int a, int b){

 int temp; 

 //If a & b aren't the same index swap
   if(a!=b)
          {   
            if(lockedIndex(a,b))
              {
                 try{
                      temp=data[b];
                      data[b]=data[a];
                      data[a]=temp;
                     }finally{
                              locker[a].unlock(); 
                              locker[b].unlock();
                              }
               }
             else{System.out.println("Couldn't lock");}   
           }
           else{System.out.println(return;}

       }//EOF Method

   public void display(){
       System.out.println("The array when swapped");
       for(int j=0; j<data.length;j++)
           {
              System.out.print(data[j]+", ");
           }
       }// End of Display

  }// EOC

Thanks again for all of your answers and apologies for my late reply.

Here is a quick fix for your program:

1. Get rid of lock1 and lock2 fields, they are the source of your problem.
2. Replace all references to lock1 with locker[a] and lock2 with locker[b].

Your program does not have any deadlock problems because you are using tryLock() so you don't really need to worry about that. If you used lock() then you need to think of the order in which you acquire locks or else you may dead lock (see other answers).

Now the reason your current program is flawed is because lock1 and lock2 fields are shared across threads. So for example, thread-1 sets lock1 to locker[0], then instantly another thread-2 overwrites lock1 by setting it to locker[99], so when thread-1 tries to unlock lock1 it happens to reference locker[99] lock which was acquired by thread-2, hence the IllegalStateMonitorException.

Hope this clears it up.

I think your problem is that you call swap(.., ..) from multiple threads and the swap (and lockedIndex) method is not thread safe since the member (lock1 and lock2) variables could be set by another thread - and you would unlock some other index that you did not lock.

Using tryLock could be tricky, what would you do if you cannot get the lock?

I would go for the solution the guys over at the forum described - locking the indexes in a perticular order for more information about the problem you have run into see Dining philosophers problem.

Here is your code re-written to use lock() instead:

class ShuffleBase {

    private int[] data;
    private ReentrantLock[] locker;

    public ShuffleBase(int[] data) {

        this.data = data;

        this.locker = new ReentrantLock[data.length];
        for (int x = 0; x < data.length; x++)
            this.locker[x] = new ReentrantLock();
    }

    public void swap(int a, int b) {

        // check if we have to do sothing
        if (a == b) 
            return; 

        // set the order (we want to lock lower indexes first!)
        if (b < a) {
            int tmp = b;
            b = a;
            a = tmp;
        }

        // lock them in sequence (first the lower, then the higher index)
        locker[a].lock();
        locker[b].lock();

        // do the swap
        int tmp = data[b];
        data[b] = data[a];
        data[a] = tmp;

        // unlock
        locker[b].unlock();
        locker[a].unlock();
    }

    public void display() {
        for (int i = 0; i < locker.length; i++) locker[i].lock();
        System.out.println("Swapped array: " + Arrays.toString(data));
        for (int i = 0; i < locker.length; i++) locker[i].unlock();
    }
}

I assume you are doing this as part of some studying or homework to learn about concurrency? If not, please consider redesign...

That said, i further assume that that task to accomplish is to call "swap" from multiple threads randomly. Then your program will most probably immediately fail because you hold "lock1" and "lock2" in instance variables. These are not thread local - when it comes to "lockedIndex" some other thread is likely to have overwritten these values, so that it is unpredictable who finally unlocks them. Remove this variables and access always via "locker[]" This should do for the IllegalMonitorState...

After this you will encounter deadlocks (as said in other forums). This stems from the fact that two threads may simultaneously acquire locks in opposite order for example a swap(1,2) and swap(2,1). Now thread 1 holds locker[1] and waits for lockar[2] and thread 2 the other wa round. To avoid this you should put an ordering to your lock acquisition, for example always in ascending order.

Minor flaws: - do not make x an instance variable - do not make lockedIndex public (unless really called from outside)

An outstanding book on the topic of concurrency, even when a little outdated:

http://java.sun.com/docs/books/cp/