I wrote a simple rate limiter for limiting remote service usage:

import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.Semaphore;
import java.util.concurrent.TimeUnit;

class SimpleRateLimiter {
    private Semaphore semaphore;
    private int maxPermits;
    private TimeUnit timePeriod;
    private ScheduledExecutorService scheduler;

    public static SimpleRateLimiter create(int permits, TimeUnit timePeriod) {
        SimpleRateLimiter limiter = new SimpleRateLimiter(permits, timePeriod);
        limiter.schedulePermitReplenishment();
        return limiter;
    }

    private SimpleRateLimiter(int permits, TimeUnit timePeriod) {
        this.semaphore = new Semaphore(permits);
        this.maxPermits = permits;
        this.timePeriod = timePeriod;
    }

    public boolean tryAcquire() {
        return semaphore.tryAcquire();
    }

    public void blockAcquire() throws InterruptedException {
        semaphore.acquire();
    }

    public void stop() {
        scheduler.shutdownNow();
        semaphore.drainPermits();
    }

    public int getPermitCount() {
        return semaphore.availablePermits();
    }

    public void schedulePermitReplenishment() {
        scheduler = Executors.newScheduledThreadPool(1);
        scheduler.scheduleWithFixedDelay(() -> {
            semaphore.release(maxPermits - semaphore.availablePermits());
        }, 0, 1, timePeriod);

    }
}

And to use it, I have the following:

SimpleRateLimiter rateLimiter = SimpleRateLimiter.create(100, TimeUnit.SECONDS);
...
//In some thread loop:
if (rateLimiter.tryAcquire()) {
    System.out.println("Permit left: "   rateLimiter.getPermitCount());
...
}

All was fine until on one normal day it stopped working. Checking the logs I found the rateLimiter.getPermitCount() gets to 104, which (I suspected) making maxPermits - semaphore.availablePermits() becomes negative and throws Exception, causing the single thread scheduler inside schedulePermitReplenishment() ceased working. My question is, in what circumstances getPermitCount() can pass over 100 given the semaphore can only be accessed internally and only accessible by a single thread?

Thanks

CodePudding user response：

There are a few problems with your code. But one problem is a potential race condition in the schedulePermitReplenishment method.

If this would be called concurrently and you would have e.g. 9 available permits and 10 max permits, then you could end up with the following:

semaphore.release(10 - 9);

Which is the same as sempahore.release(1). So if 2 threads would call this concurrently, you end up with 11 permits instead of 10.

And as you already indicated, the next time schedulePermitReplenishment is called, you will end up with an 10-11=-1 permits that is going to be released and you end up with an exception (which is indeed swallowed by the executor).

Why do you need to replenish the permits? Are tickets getting lost? Because this could be another source of the number of available tickets being larger than the 'max' number of tickets. E.g if one ticket is taken and you would call replenish, and the ticket is eventually returned, you also end up with too many tickets.

I'm not exactly sure how this code is going to be used, but you could also have a data race on the executor field. Maybe it is better to create the executor in the constructor and not replace it.