aherbert commented on PR #1475: URL: https://github.com/apache/commons-lang/pull/1475#issuecomment-3451116897
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. -- 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]
