commons-lang [LANG-1656] Fairness setting in TimedSemaphore

Thanks for your contribution to Apache Commons! Your help is appreciated!

Before you push a pull request, review this list:

[X] Read the contribution guidelines for this project.
[X] Read the ASF Generative Tooling Guidance if you use Artificial Intelligence (AI).
[X] I used AI to create any part of, or all of, this pull request.
[X] Run a successful build using the default Maven goal with mvn; that's mvn on the command line by itself.
[X] Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied. This may not always be possible, but it is a best practice.
[X] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
[X] Each commit in the pull request should have a meaningful subject line and body. Note that a maintainer may squash commits during the merge process.

Description of Issue This PR adresses "LANG[1790] - Fairness setting in TimedSemaphore" : https://issues.apache.org/jira/browse/LANG-1790

The existing TimedSemaphore implementation provides rate limiting across fixed time periods, but uses manual synchronisation (wait() / notifyAll()) for blocking. Under heavy contention, this approach can lead to non-deterministic thread ordering and inconsistent blocking behaviour. Additionally, dynamically changing the limit during an active period did not immediately update the available permits, allowing extra acquires within that window.

Implementation Fairness Support: Added a new builder flag that enables optional FIFO fairness for permit acquires. Fairness is handled by a backing 'java.util.concurrent.Semaphore' constructed with the fair flag, ensuring that waiting threads acquire permits in order. Refactored Permit Management: Replaced counter synchronisation with a dedicated Semaphore to manage permits. The semaphore now handles all blocking, acquisition, and wake-up behavior, simplifying the internal logic and improving correctness under contention. Dynamic Limit Adjustment: Updated setLimit(int) to immediately resize the active permit capacity for the current period. End of period refill: The endOfPeriod() method now explicitly tops up the semaphore to the configured limit at each interval, ensuring clean period rollover and accurate permit replenishment.

Validation and Testing

FIFO ordering , Method: testFairnessFIFOOrdering_acrossPeriods() When fair = true, and the limit is set to 1, threads that arrive earlier should be served earlier across consecutive periods. The method acquires T1 (thread 1) first, then T2 and T3. Through manually calling endofPeriod() twice, the completion order asserted is as follows:
1. T1
2. T2
3. T3 This test proves that the fairness flag actually allows for FIFO queuing
Dynamic resizing of the limit , Method: testSetLimitResizesBucketWithinPeriod() It checks that, in the middle of a period, given a decrease in the limit takes place, the code immediately prevents further acquires within the same period if the new limit has already been reached. It starts with limit = 3, acquires twice, then it decreases the limit to 1. It first tries tryAcquire(), which is expected to fail. Upon failure, endofPeriod() is called, and only 1 permit should be available in the new period. This validates the behaviour of setLimit() and the coordination between statistical counters.
Period rollover and unblocking , Method: testEndOfPeriodTopUpReleasesBlockedThreads() It checks that endofPeriod() refills permits to the configured limit, and ‘wakes’ blocked threads through notifyAll(), which allows for the next thread waiting to acquire. It starts with assignment limit = 1, T1 acquires and T2 blocks. After manually calling endOfPeriod(), T2 should complete straight away. This confirms the new top-up and unblocking logic are correctly implemented
NO_LIMIT short circuit with fairness , Method: testAcquireNoLimitWithFairnessEnabled() Through ‘limit <= NO_LIMIT’, acquire() never blocks, even when fairness is enabled. It does this via building through Builder with NO_LIMIT and assuring fair = true. Then, acquire() is called many times without waiting for period rollover This confirms the short circuit path ignores the Semaphore cleanly and remains viable alongside the fairness option
Permits after increase , Method: testGetAvailablePermitsAfterLimitIncreaseWithinPeriod() It checks that whether the limit increase in the middle of a period influences the remaining (non-blocking) available permits, reported via getAvaliablePermits() alongside tryAcquire(). It does this as, when limit = 1, acquire occurs once, then the limit increase to 3, then two more non-blocking acquires should now pass and be valid, and then finally the 4th one should fail This verifies that counters and semaphore capacity stay consistent when upscaling the limit during a period

Oct 26 '25 11:10 TeeleTitan

@ppkarwasz , @chtompki or @aherbert Any thoughts on this PR?

Oct 27 '25 11:10 garydgregory

I think this is an improvement. However it is difficult to write concurrent code and there may be issues I did not notice. The class seems to have issues around the computation of permits, acquires and limits as it makes some assumptions on the limit based on whether it is zero or negative but then does not guard the limit to the range [0, MAX_VALUE]. Thus the limit can be set to a large negative value and break existing functionality.

I noted that the fair flag is not enforced when using the semaphore as it uses tryAcquire() which does not respect fairness. So the current unit test is not robust enough to detect this.

Also not changed in the PR, but it broken in the current code is the computation in the method getAvailablePermits(). This method can return negative values which do not make sense. It should return a value in the positive range if there is a configured limit. If the limit is disabled then it should return either -1 as a documented flag or Integer.MAX_VALUE to state that you can have as many permits as you desire. I prefer returning MAX_VALUE so that clients can compare this value to any positive number of permits that they desire.

Oct 27 '25 12:10 aherbert

I love you guys!On Oct 27, 2025, at 8:48 AM, Alex Herbert @.***> wrote:aherbert left a comment (apache/commons-lang#1475) I think this is an improvement. However it is difficult to write concurrent code and there may be issues I did not notice. The class seems to have issues around the computation of permits, acquires and limits as it makes some assumptions on the limit based on whether it is zero or negative but then does not guard the limit to the range [0, MAX_VALUE]. Thus the limit can be set to a large negative value and break existing functionality. I noted that the fair flag is not enforced when using the semaphore as it uses tryAcquire() which does not respect fairness. So the current unit test is not robust enough to detect this. Also not changed in the PR, but it broken in the current code is the computation in the method getAvailablePermits(). This method can return negative values which do not make sense. It should return a value in the positive range if there is a configured limit. If the limit is disabled then it should return either -1 as a documented flag or Integer.MAX_VALUE to state that you can have as many permits as you desire. I prefer returning MAX_VALUE so that clients can compare this value to any positive number of permits that they desire.

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

Oct 27 '25 14:10 chtompki

Hello @TeeleTitan

Would you please fix the builds and address comments?

Thank you

Oct 30 '25 11:10 garydgregory