Can't get critical section to be atomic

Question

I'm currently learning about critical section & semaphores and I'm stuck atm with this part. I hope you guys can give me an insight.

I have these 3 types of threads: one will do pop() on a stack, another does push() on the same stack and the last one prints the values of that stack. At the moment, I've put wait() and signal() on what I assumed as critical sections.

class Example{

  public static void main(){

    //create thread objects
    StackPop p1 = new StackPop();
    StackPop p2 = new StackPop();
    StackPop p3 = new StackPop();

    StackPush ps1 = new StackPush();
    StackPush ps2 = new StackPush();
    StackPush ps3 = new StackPush();

    StackValues s1 = new StackValues();
    StackValues s2 = new StackValues();
    StackValues s3 = new StackValues();

    //then we start these threads in mix orders

    p1.start();
    s3.start();
    ps2.start();
    // etc
  }
}

class StackPop extends Thread{

  public void run(){

    mutex.wait();
    pop();
    SOP("value popped is " + get.popVal); 
    mutex.signal();

  }
}

class StackPush extends Thread{

  public void run(){

    mutex.wait();
    push();
    SOP("value inserted is " + get.pushVal);
    mutex.signal();

  }
}

class StackValues extends Thread{

  public void run(){

    mutex.wait();

    for(int i = 0; i<stack.size(); i++)
        S.O.P.("["+getVal(i)+"]");   //where getVal() will retrieve value at that index

    mutex.signal();
  }
}

The above code is a simplified version, but the idea is the same. My problem is even though I used wait() and signal(), I still get a really weird output. For example part of my output will show "[1][2][3] value inserted is 3 [5][@][@]" (where @ means no number inside). I realize this is because the processor is letting another thread run while the for loop is getting the value. I was assuming wait() and signal() will make it atomic automatically but maybe I missed something.

Answer 1

You can use locks or "synchronized" on your critical section. You can see more in http://docs.oracle.com/javase/tutorial/essential/concurrency/syncmeth.html You can use the same idea to pop() push() ect..

With mutex;

try {
     mutex.acquire();
     try
     { 
     // work
     } finally {
     mutex.release();
  }
} catch(InterruptedException ie) {// ...}

source -> http://java.dzone.com/articles/dnp-java-concurrency-–-part-4

"I have these 3 types of threads: one will do pop() on a stack, another does push() on the same stack and the last one prints the values of that stack"

You are using 3 different stack on for each Thread, thats why you have a unexpected. You make pop on one stack, you push from different stack and finally you print from a another stack.

You should do something like :

public class Teste {

    Thread t1;
    Thread t2;
    Thread t3;

    Stack<Integer> stack = new Stack <Integer>();
    private final Semaphore mutex = new Semaphore(1);

    public Teste ()
    {
        Runnable r1 = new Runnable()                                                    
        {                                   
            public void run()
            {
            pop();
            } // run()
        }; // runnable


        Runnable r2 = new Runnable()                                                    
        {                   
            public void run()
            {
                for(int x = 0; x < 3; x++)
                   push(x);
            } // run()
        }; // runnable

        Runnable r3 = new Runnable()                                                    
        {                   
            public void run()
            {
                for(int x = 0; x < 3; x++)
                   print();
            } // run()
        }; // runnable

        this.t1 = new Thread(r1); // Create the thread associating the runnable
        this.t2 = new Thread(r2);
        this.t3 = new Thread(r3);
    }

    public void pop()
    {
        try {
            mutex.acquire();
        } catch (InterruptedException e) {

            e.printStackTrace();
        }
        stack.pop();
        mutex.release();
    }

    // Similar to pop
    public void push(int x){ //..}

    public void print(){
        try {
            mutex.acquire();
        } catch (InterruptedException e) {e.printStackTrace();}

        for(int i = 0; i<stack.size(); i++)
            System.out.println("["+stack.get(i)+"]");

        mutex.release();
    }

}

public static void main(String argv[])
  {

    Teste t = new Teste();
    t.t2.start();
    t.t1.start();
    t.t3.start();
  }

Answer 2

Be careful, wait is inherited from Object, its opposite is notify - you are likely to be calling the wrong methods.

Also, Java has a native synchronization mechanism, the synchronized keyword, you may want to investigate that.