What is the proper method to release a Semaphore object in Java? [duplicate]

Question

This is the code written in most of the resources online. But it is not correct because consider a scenario when a thread is blocking, and is then interrupted. The thread will still release the lock even though it has not acquired the lock. This is incorrect. So what is the correct implementation of releasing a semaphore in java?

Semaphore lock = new Semaphore(1);

  add(){
    try {
        lock.acquire();

        // critical section

    } catch (InterruptedException e) {

          Thread.currentThread().interrupt();

    }finally{
       lock.release()
    }
}

I think this is the correct solution:-. Is it?

try {
    lock.acquire();

    try {
        // do some stuff here
    } finally {

        lock.release();

    }
} catch(InterruptedException ie) {

    Thread.currentThread().interrupt();
    throw new RuntimeException(ie);

}

Answer 1

Semaphores have no concept of ownership. The permits aren't real things, it's just a count the semaphore keeps. So the question is, is the count right after the method executes? If you're interrupted do you leave the method with one available permit or two?

The semaphore examples on Oracle's site in the api docs don't have any finally blocks, in a lot of cases they're not relevant.

If you're using this semaphore to implement a mutex and it has only one permit, I expect it should have a try-finally block just like using a lock (from the ReentrantLock api doc):

 class X {
   private final ReentrantLock lock = new ReentrantLock();
   // ...

   public void m() {
     lock.lock();  // block until condition holds
     try {
       // ... method body
     } finally {
       lock.unlock()
     }
   }
 }

If the code eats the InterruptedException and lets the thread proceed on its way then whether the count is correct at the end of the method becomes important, it may keep other calls from acquiring the permit.

The general pattern I use whenever I get something, work with it, and release it is to get it above the try block, then work with it in the try block, and close/release/cleanup in the finally block. This goes for IO, JDBC resources, whatever. You can try to avoid this, put the acquisition in the try block, then check for null before you clean it up. You can also try to do too much in your finally block, fail to catch exceptions on close, and create a resource leak. It's better to minimize the amount of code in the finally blocks.

A correct example is at http://jcip.net/listings/BoundedHashSet.java (from the book Java Concurrency In Practice):

public boolean add(T o) throws InterruptedException {
    sem.acquire();
    boolean wasAdded = false;
    try {
        wasAdded = set.add(o);
        return wasAdded;
    } finally {
        if (!wasAdded)
            sem.release();
    }
}

This shows a try block with a finally that does some cleanup (releasing a permit if it turned out nothing got added to the set), where acquire is called before that try-block is entered.

If in this example the acquire call was moved to within the try block, it wouldn't matter. I think putting the call above the try block is better style, but in this example it doesn't affect correctness, because it uses the flag to decide whether to release a permit.

I would use a flag like in the jcip example, set it to true after acquiring, and only release if the flag is set. That way you can put the acquire within the try block.

    boolean wasAcquired = false;
    try {      
         sem.acquire();
        wasAcquired = true;
        // crit sect
     } catch (InterruptedException e) {
        Thread.currentThread.interrupt();
    } finally {
        if (wasAcquired)
            sem.release();
    }

Or consider acquireUninterruptibly(). But think about, if these calls don't throw the InterruptedException, what part of your code makes sure the code actually stops working when an interrupt request is received? It looks like you could fall into an unproductive cycle where threads try to acquire, throw InterruptedException, catch it and set the interrupt status, then do the same thing again the next time a thread tries to acquire, over and over and over. Throwing InterruptedException lets you respond quickly to a request for cancellation while making sure cleanup is done in finally blocks.

Answer 2

Just add a flag that indicates if the lock was acquired:

boolean acquired = false;
try {
    lock.acquire();
    acquired = true;
    // critical section

} catch (InterruptedException e) {

     // do anything

} finally {
   if (acquired) {
       lock.release()
   }
}

Another solution is to use Semaphore.acquireUninterruptibly().