Implementing a cyclicbarrier in java using semapho

Question

The question is as follows, since the barrier is only called using down() so that it would wait for the n threads to arrive and then execute all n threads together in the critical region now how do I inform the threads calling on barrier.down that it can move on now. I tried adding notifyAll() after phase2() and that doesn't work. Help? :)

public class cyclicBarrier {
    private int n;
    private int count;
    private semaphore mutex;
    private semaphore turnstile;
    private semaphore turnstile2;

    public cyclicBarrier(int n){
        this.n = n;
        this.count = 0;
        this.mutex = new semaphore(1);
        this.turnstile = new semaphore(0);
        this.turnstile2 = new semaphore(0);
    }

    public synchronized void down() throws InterruptedException{
        this.phase1(); //waits for n threads to arrive
        this.phase2(); //waits for n threads to execute
    }

    private synchronized void phase1() throws InterruptedException {
        this.mutex.down();
        this.count++;
        if(this.count == this.n){
            for(int i = 0; i < this.n; i++){
                this.turnstile.signal(); //when n threads received then move on to phase 2
            }
        }
        this.mutex.signal();
        this.turnstile.down(); //keeps waiting till I get n threads
    }

    private synchronized void phase2() throws InterruptedException {
        this.mutex.down();
        this.count--;
        if(this.count == 0){
            for(int i = 0; i < this.n; i++){
                this.turnstile2.signal(); //reset the barrier for reuse 
            }
        }
        this.mutex.signal();
        this.turnstile2.down(); //keeps waiting till n threads get executed
    }
}


public class semaphore {
    private int counter;

    public semaphore(int number){
        if (number > 0) {
            this.counter = number;
        }
    }

    public synchronized void signal(){
        this.counter++;
        notifyAll();
    }

    public synchronized void down() throws InterruptedException{
        while (this.counter <= 0){
            wait();
        }
        this.counter--;
    }
}

Answer 1

I see you're using the solution from The Little Book of Semaphores. One main point of the book is that you can solve many coordination problems using semaphores as the only coordination primitive. It is perfectly fine to use synchronized to implement a semaphore, since that is necessary to do it correctly. It misses the point, however, to use synchronized in the methods which solve a puzzle that is supposed to be solved with semaphores.

Also, I think it doesn't work in your case: don't you get a deadlock at this.turnstile.down()? You block on a semaphore which holding an exclusive lock (through synchronized) on the object and method which would allow that semaphore to get released.

Addressing the question as stated: you signal to threads that they can proceed by returning from barrier.down(). You ensure that you don't return too soon by doing turnstile.down().

Aside: Semaphore implementation

Your semaphore implementation looks correct, except that you only allow non-negative initial values, which is at least non-standard. Is there some motivation for doing this that I can't see? If you think negative initial values are wrong, why not throw an error instead of silently doing something else?

Aside: Other synchronization primitives

Note that the java constructs synchronized, .wait() and .notify() correspond to the Monitor coordination primitive. It may be instructive to solve the puzzles with monitors (or other coordination primitives) instead of semaphores, but I would recommend keeping those efforts separate. I've had a bit of fun trying to solve a puzzle using Haskell's Software Transactional Memory.

Aside: On runnability

You say you have tried things, which indicates that you have some code that allows you to run the code in the question. It would have been helpful if you had included that code, so we could easily run it too. I probably would have checked that my hypothesized deadlock actually occurs.