CODEWITHSUNDEEP
javaconcurrencysemaphore
Chander Shivdasani
Chander Shivdasani

Reputation: 10121

Simple Counting semaphore

To understand the working of counting semaphores, i decided to implement a simple version. I wanted to verify my current implementation is in fact a correct implementation and i havent missed anything obvious

public class CountingSemaphore {
    private int limit;

    public CountingSemaphore(int limit) {
        this.limit = limit;
    }

    public synchronized void acquire() {
        try {
            if (limit == 0)
                wait();

            limit--;
        } catch (Exception e) {
            e.printStackTrace();
        }
    }

    public synchronized void release() {
        try {
            if(limit == 0) 
                notifyAll();

            limit++;
        }catch(Exception e) {
            e.printStackTrace();
        }
    }

}

Upvotes: 1

Views: 1148

Answers (4)

roni bar yanai
roni bar yanai

Reputation: 1492

you have a problem in your code, when you call release you call notify all but here

public synchronized void acquire() {
        try {
            if (limit == 0)
                wait();

            limit--;
        } catch (Exception e) {
            e.printStackTrace();
        }
    }

all threads that are waiting are released, and you can git limit<0, and you sem breaks the common solution here is to use loop.

 while(limit == 0){
     wait();
}

Upvotes: 0

steffan
steffan

Reputation: 619

From a client perspective you should force me to use your semaphore with a value >= 0.

Upvotes: 0

JB Nizet
JB Nizet

Reputation: 691715

wait() should always be enclosed inside a while loop checking for the wakeup condition, due to spurious wakeups. Read the javadoc for more information.

Catching Exception, and swallowing them, is a very bad practice. Your acquire() method should throw InterruptedException. Your release method shouldn't catch Exception.

And I wouldn't use the semaphore itself as the lock: an external class could use it to synchronize something completely unrelated, and it could lead to bad performance or deadlock. I would use a private final lock object.

Upvotes: 3

Keppil
Keppil

Reputation: 46219

This should work except for one detail.
Since you use notifyAll(), (and as @JBNizet points out, due to the risk of spurious wakeups,) you can wake up several waiting threads, all of which will be freed and decrease limit.
Change 

if (limit == 0)
    wait();

to 

while (limit == 0) {
    wait();
}

and you should be fine.

Upvotes: 3

Related Questions