Search code examples
javalockingjava-threads

Problem with critical section using Retrant Lock with Condition


I have a small project to synchronize multiple (two classes: ships, cars with a few instances with shared bufor class called Harbour) threads at the same time. They will be performing certain action on it. But I can't start with that until I synchronized the threads named "cars" in the Harbour. The Harbour has limited capacity and if this capacity is reached the "car" threads should be waiting until they will get signal that there's a free space to enter. I've used Retrant Lock with Condition but it doesn't work as I think.

public class Harbour {
    final Lock protectNr;
    final Condition protectNrCon;
    int capacity;
    int nrOfCars;

    public Harbour(int capacity) {
        this.capacity = capacity;
        this.protectNr = new ReentrantLock();
        this.protectNrCon = protectNr.newCondition();
    }

    public void carEnterHarbour(String name) {
        try {
            protectNr.lock();
            if (this.nrOfCars == this.capacity)
                protectNrCon.await();

            nrOfCars++;
            System.out.println(name + " enters");
            System.out.println("Number of cars:" + this.nrOfCars);
            protectNr.unlock();
        } catch (InterruptedException e) {
            System.out.println("Error");
        }
    }

    public void carLeavingHarbour(String name) {
        try {
            protectNr.lock();
            this.nrOfCars--;
            protectNrCon.signal();
            System.out.println(name + " leaving");
            System.out.println("Number of cars:" + this.nrOfCars);
        } finally {
            protectNr.unlock();
        }
    }
}

public class Car extends Thread {
    Harbour harbour;
    public Car(Harbour harbour, String name) {
        super(name);
        this.harbour = harbour;
    }

    public void run() {
        for (int i = 0; i < 10; i++) {
            harbour.carEnterHarbour(getName());
            harbour.carLeavingHarbour(getName());
        }
    }
}

public class Test {
    public static void main(String[] args) throws InterruptedException {
        int harbourCapacity = 20;
        final Harbour harbour = new Harbour(harbourCapacity);
        int nrOfCars = 500;
        Car[] cars = new Car[nrOfCars];
        for (int i = 0; i < nrOfCars; i++)
            cars[i] = new Car(harbour, "Car-" + i);

        for (int i = 0; i < nrOfCars; i++)
            cars[i].start();

        for (int i = 0; i < nrOfCars; i++)
            cars[i].join();
    }
}

What I was expecting after executing this code:

Car-386 leaving
Number of cars:**19**
Car-300 enters
Number of cars:**20**
Car-300 leaving
Number of cars:**19**

What I got:

Car-386 leaving
Number of cars:**20**
Car-300 enters
Number of cars:**21**
Car-295 enters
Number of cars:**22**

I also try to change int capacity to volatile int capacity and add some busy waiting but didn't work at all. It looks like Threads are not block on Condition and I wonder why is this happening?


Solution

  • The documentation for Condition warns that spurious wakeups might occur (emphasis mine):

    When waiting upon a Condition, a "spurious wakeup" is permitted to occur, in general, as a concession to the underlying platform semantics. This has little practical impact on most application programs as a Condition should always be waited upon in a loop, testing the state predicate that is being waited for. An implementation is free to remove the possibility of spurious wakeups but it is recommended that applications programmers always assume that they can occur and so always wait in a loop.

    Your code doesn't honor that warning.

    Your carEnterHarbour() must take this possibility of spurious wakeups into account and needs

            while(this.nrOfCars == this.capacity){
                protectNrCon.await();
            }
    

    instead of the simple if statement.


    Depending on your requirements it might be easier to use a Semaphore:

    public class Harbour {
        final Semaphore slots;
    
        public Harbour(int capacity){
            this.slots = new Semaphore(capacity);
        }
    
        public void carEnterHarbour(String name) {
            try{
                slots.acquire();
            }catch (InterruptedException e){
                System.out.println("Error");
            }
    
        }
        public void carLeavingHarbour(String name) {
            slots.release();
        }
    }
    

    Note that when using a Semaphore you don't have those locks in place when entering / leaving the Harbour and therefore it is difficult to get that ordered "car entering" / "car leaving" output together with the number of currently available slots.