CODEWITHSUNDEEP
javaconcurrencysemaphore
user2916610
user2916610

Reputation: 765

Is this code snippet about semaphore necessary?

Here's the code from Java Concurrency in Practice, showing how to make execute block when the work queue is full by using a Semaphore to bound the task injection rate. The semaphore is equal to the pool size plus the number of queued tasks you want to allow.

public class BoundedExecutor {
    private final Executor exec;
    private final Semaphore semaphore;
    public BoundedExecutor(Executor exec, int bound) {
        this.exec = exec;
        this.semaphore = new Semaphore(bound);
    }
    public void submitTask(final Runnable command)
            throws InterruptedException {
        semaphore.acquire();
        try {
            exec.execute(new Runnable() {
                public void run() {
                    try {
                        command.run();
                    } finally {
                    semaphore.release();
                    }
                }
            });
        } catch (RejectedExecutionException e) {
            semaphore.release();
        }
    }
}

My question is about

catch (RejectedExecutionException e) { semaphore.release(); }

Isn't it unnecessary while we have semaphore.acquire(); above?

If the work queue is full then 'semaphore.acquire' should be block, and there would be no RejectedExecutionException.

Upvotes: 2

Views: 100

Answers (4)

Mister Smith
Mister Smith

Reputation: 28168

I think you are confusing the inner executor (passed by parameter in the constructor) with the outer one (BoundedExecutor). Even if the BoundedExecutor class has the executor word in the name, and has a method that submits tasks, it is not implementing the Executor interface.

It is kind of a decorator class. What it does is to offer the functionality a normal Executor offers but limiting the number of tasks that can be submitted.

The exception you mention is thrown by the inner executor's execute method. Because the critical section can throw Exceptions, you need to release the lock in the finally clause. Otherwise you would be incorrectly counting one up for a failed submission.

Upvotes: 0

Jiri Tousek
Jiri Tousek

Reputation: 12440

Even if you assume that the executor will never reject your request (e.g. because you believe the bound is lower than executor's pool size + queue limit), it's still good practice not to rely on your assumptions. The above code will work correctly even if your assumption is wrong, and won't have any negative effect if your assumption is correct.

As it stands, the code above doesn't set any bounds on the executor and only sets bound on the semaphore, so it certainly is possible to choose a bound that's higher than what the executor will accept.

Upvotes: 0

Giovanni Botta
Giovanni Botta

Reputation: 9816

The RejectedExecutionException can be thrown by the executor if the task cannot be accepted. If this happened, it means that the semaphore was already acquired, but the executor decided it was not able to accept the task.

This behavior depends on the implementation of the executor, so it is independent from calling acquire() successfully and if it does happen, the semaphore must be released to indicate that there is a free slot for a new task.

Ideally the method should return true or false to indicate whether this happened or not.

Upvotes: 1

Kayaman
Kayaman

Reputation: 73558

The documentation says throws RejectedExecutionException if this task cannot be accepted for execution. You want to be certain that the semaphore is released if the task can't be accepted for any reason.

Upvotes: 1

Related Questions