aherbert commented on code in PR #1475:
URL: https://github.com/apache/commons-lang/pull/1475#discussion_r2465373467
##########
src/main/java/org/apache/commons/lang3/concurrent/TimedSemaphore.java:
##########
@@ -123,6 +124,7 @@ public static class Builder implements
Supplier<TimedSemaphore> {
private long period;
private TimeUnit timeUnit;
private int limit;
+ private boolean fair = false;
Review Comment:
Remove the default initialiser.
##########
src/main/java/org/apache/commons/lang3/concurrent/TimedSemaphore.java:
##########
@@ -290,41 +314,48 @@ public TimedSemaphore(final ScheduledExecutorService
service, final long timePer
* @throws InterruptedException if the thread gets interrupted.
* @throws IllegalStateException if this semaphore is already shut down.
*/
- public synchronized void acquire() throws InterruptedException {
+ public void acquire() throws InterruptedException {
prepareAcquire();
- boolean canPass;
- do {
- canPass = acquirePermit();
- if (!canPass) {
- wait();
+ if (getLimit() <= NO_LIMIT) {
+ synchronized (this) {
+ acquireCount++;
+ }
+ return;
+ }
+ if (semaphore.tryAcquire()) {
+ synchronized (this) {
+ acquireCount++;
+ }
+ return;
+ }
+ for (;;) {
+ synchronized (this) {
+ if (semaphore.tryAcquire()) {
+ acquireCount++;
+ return;
+ }
+ this.wait();
}
- } while (!canPass);
- }
-
- /**
- * Internal helper method for acquiring a permit. This method checks
whether currently a permit can be acquired and - if so - increases the internal
- * counter. The return value indicates whether a permit could be acquired.
This method must be called with the lock of this object held.
- *
- * @return a flag whether a permit could be acquired.
- */
- private boolean acquirePermit() {
- if (getLimit() <= NO_LIMIT || acquireCount < getLimit()) {
- acquireCount++;
- return true;
}
- return false;
}
/**
* The current time period is finished. This method is called by the timer
used internally to monitor the time period. It resets the counter and releases
* the threads waiting for this barrier.
*/
- synchronized void endOfPeriod() {
+ void endOfPeriod() {
lastCallsPerPeriod = acquireCount;
totalAcquireCount += acquireCount;
periodCount++;
acquireCount = 0;
- notifyAll();
+ final int avail = semaphore.availablePermits();
+ final int toRelease = Math.max(0, getLimit() - avail);
Review Comment:
This will overflow if I use `setLimit(Integer.MIN_VALUE)` so we need some
better guarding on the limit range.
##########
src/main/java/org/apache/commons/lang3/concurrent/TimedSemaphore.java:
##########
@@ -437,6 +469,17 @@ private void prepareAcquire() {
*/
public final synchronized void setLimit(final int limit) {
this.limit = limit;
+
+ if (semaphore != null) {
Review Comment:
IIUC the semaphor is only null when `setLimit` is called in the constructor.
I would avoid the call to this method in the constructor and set the limit
directly. The null check can then be removed here.
One caveat here is that limit is checked for <= 0 in several places to
change behaviour. There is no usage of limit when it is negative. However
setting it to Integer.MIN_VALUE will cause some of the difference computations
to overflow. I would add a guard for limit when setting it to `max(NO_LIMIT,
limit)`.
In order to guard the limit to the positive range it could be set only
within a private method:
```java
private void updateLimit(int value) {
limit = Math.max(0, value);
}
```
##########
src/main/java/org/apache/commons/lang3/concurrent/TimedSemaphore.java:
##########
@@ -290,41 +314,48 @@ public TimedSemaphore(final ScheduledExecutorService
service, final long timePer
* @throws InterruptedException if the thread gets interrupted.
* @throws IllegalStateException if this semaphore is already shut down.
*/
- public synchronized void acquire() throws InterruptedException {
+ public void acquire() throws InterruptedException {
prepareAcquire();
- boolean canPass;
- do {
- canPass = acquirePermit();
- if (!canPass) {
- wait();
+ if (getLimit() <= NO_LIMIT) {
+ synchronized (this) {
+ acquireCount++;
+ }
+ return;
+ }
+ if (semaphore.tryAcquire()) {
Review Comment:
Javadoc for Semaphor states we should use [tryAcquire(0,
TimeUnit.SECONDS)](https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/Semaphore.html#tryAcquire-long-java.util.concurrent.TimeUnit-)
to preserve fairness. So this behaviour should change based on the constructor
flag, e.g. using a helper method:
```java
private boolean fairTryAcquire() throws InterruptedException {
return fair ? semaphore.tryAcquire(0, TimeUnit.SECONDS) :
semaphore.tryAcquire();
}
```
##########
src/main/java/org/apache/commons/lang3/concurrent/TimedSemaphore.java:
##########
@@ -475,8 +516,17 @@ protected ScheduledFuture<?> startTimer() {
* @throws IllegalStateException if this semaphore is already shut down.
* @since 3.5
*/
- public synchronized boolean tryAcquire() {
+ public boolean tryAcquire() {
prepareAcquire();
- return acquirePermit();
+ if (getLimit() <= NO_LIMIT) {
+ synchronized (this) { acquireCount++; }
+ return true;
+ }
+
+ final boolean ok = semaphore.tryAcquire();
Review Comment:
Guard this tryAcquire with the fair flag. However in this method it would
add a throws InterruptedException to the API. So it should be documented as not
respecting the fair policy, e.g. copy the wording from the Semaphore javadoc on
`tryAcquire`.
##########
src/main/java/org/apache/commons/lang3/concurrent/TimedSemaphore.java:
##########
@@ -235,10 +249,18 @@ public static Builder builder() {
/** A flag whether shutdown() was called. */
private boolean shutdown; // @GuardedBy("this")
+ /** Fairness mode checker */
+ private final boolean fair;
Review Comment:
This flag is never used outside the constructor. Currently it could be
dropped. However if we wish to preserve fairness then it should be used in
`acquire`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
