Unisex Bathroom problem with semaphore An office has a bathroom that can be used by both men and women, but not both at the same time. If a man is in the bathroom, other men may enter, but any women wishing to use the bathroom should wait for it to be empty. If a woman is in the bathroom, other women may enter, but any men wishing to use the bathroom should wait it to be empty. Each person (man or woman) will spend some time using the bathroom.



package basics.problems;

    //BATHROOM_SIZE - 5
    //2 types of threads - MEN and WOMEN
    import java.util.concurrent.Semaphore;
    import java.util.concurrent.atomic.AtomicInteger;

    public class UniSexBathroom {
        AtomicInteger currentSize;
        int capacity;
        Semaphore noOfEntriesSemaphore;
        Semaphore currentGenderSemaphore = new Semaphore(1);
        volatile Person currentGender;

        public UniSexBathroom(int capacity) {
            this.capacity = capacity;
            this.noOfEntriesSemaphore = new Semaphore(capacity);
            this.currentSize =  new AtomicInteger(0);
        }

        public void occupy(Person personSex) throws InterruptedException {
            acquireGenderSemaphore(personSex);
            useBathroomAndExit(personSex);
            releaseGenderSemaphore(personSex);
        }

        private void releaseGenderSemaphore(Person personSex) {
            if(currentSize.get()==0){
                currentGenderSemaphore.release();
                currentGender = null;
                System.out.println(personSex.type+ Thread.currentThread().getName()+" releasing gender semaphore");
            }
        }

        private void useBathroomAndExit(Person personSex) throws InterruptedException {
            noOfEntriesSemaphore.acquire();
            currentSize.incrementAndGet();
            System.out.println(personSex.type +  Thread.currentThread().getName()+" is stepping in, current capacity is : "+currentSize.get());
            Thread.sleep(100);
            noOfEntriesSemaphore.release();
            currentSize.decrementAndGet();
            System.out.println(personSex.type +  Thread.currentThread().getName()+" is stepping out, current capacity is : "+ currentSize.get() +"-----------------");
        }

        private void acquireGenderSemaphore(Person personSex) throws InterruptedException {
            if(currentSize.get() == 0){
                currentGenderSemaphore.acquire();
                currentGender = personSex;
            }
            if(!personSex.type.equals(currentGender.type)){
                currentGenderSemaphore.acquire();
                currentGender = personSex;
            }
        }

        public static void main(String[] args){
            UniSexBathroom uniSexBathroom = new UniSexBathroom(2);
            new Thread(new PersonThread(uniSexBathroom, Person.MALE)).start();
            new Thread(new PersonThread(uniSexBathroom, Person.MALE)).start();
            new Thread(new PersonThread(uniSexBathroom, Person.MALE)).start();
            new Thread(new PersonThread(uniSexBathroom, Person.MALE)).start();

            new Thread(new PersonThread(uniSexBathroom, Person.FEMALE)).start();
            new Thread(new PersonThread(uniSexBathroom, Person.MALE)).start();
            new Thread(new PersonThread(uniSexBathroom, Person.FEMALE)).start();
        }
    }

    enum Person {
        MALE("Male"), FEMALE("Female");
        String type;

        Person(String type) {
            this.type = type;
        }
    }

    class PersonThread implements Runnable {
        public UniSexBathroom uniSexBathroom;
        public Person person;
        public PersonThread(UniSexBathroom uniSexBathroom, Person person){
            this.uniSexBathroom = uniSexBathroom;
            this.person = person;
        }

        @Override
        public void run() {
            try {
                uniSexBathroom.occupy(person);
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        }
    }
Your code has a bug.  I can reproduce it fairly consistently by changing your run method to:

@Override
public void run() {
    try {
        for(int i=0;i&lt;100;i++) {
            uniSexBathroom.occupy(person);
        }
    } catch (InterruptedException e) {
        e.printStackTrace();
    }
}


Notice, that I've increased the number of times that each thread tries to occupy the bathroom.  Starting/Stopping threads is a fairly slow process so in order to encourage more contention I used a loop so more time is spent trying to get into the bathroom.  It results in a null pointer in the acquireGenderSemaphore method on this line:

if (!personSex.type.equals(currentGender.type)) {


It's happening because currentGender is null.  We can be confident about this, because personSex is the same for every call for a given thread.  How is it null?  Well, because this isn't safe:

if (currentSize.get() == 0) {
    currentGenderSemaphore.acquire();
    currentGender = personSex;
}


The problem is that context switches can happen at any time, so just because currentSize wasn't 0 for the if evaluation, it doesn't mean that it's still zero during the execution of the if block.

So:

Thread2 is currently in the bathroom (Size 1)

Thread1 comes in to acquire the lock, checks the value of current size and finds it's 1, skip the if block

if (currentSize.get() == 0) {  // Size still 1


Thread2, which is currently using the bathroom leaves:

noOfEntriesSemaphore.release();
currentSize.decrementAndGet(); // Size becomes 0


Thread2, then checks if they were the last one out, which they were:

if (currentSize.get() == 0) {
    currentGenderSemaphore.release();
    currentGender = null;  // Gender becomes null
}


Thread1 then proceeds, assuming there is still somebody in the bathroom:

if (!personSex.type.equals(currentGender.type)) {


But since currentGender has been set to null by Thread 2 when it left.... Bang.