Thread-safe irregular rate limiter

Question

Google's Guava library has a great RateLimiter, but I also needed a way to limit on an irregular basis, such as if an API I'm using has been throttled. I basically built a wrapper around Guava's RateLimiter, only using the simple acquire() method from it.

import java.util.concurrent.ExecutionException;
import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ScheduledFuture;
import java.util.concurrent.TimeUnit;

import com.google.common.util.concurrent.RateLimiter;

public class CustomLimiter {
    private RateLimiter guavaLimiter;
    private Throttle apiThrottle = new Throttle();

    public CustomLimiter() {
        this.guavaLimiter = RateLimiter.create(.8);
    }

    public double acquire() {
        try {
            apiThrottle.waitChoke();
        } catch (Exception e) {
            throw new IllegalStateException("Someone else handle this");
        }
        return guavaLimiter.acquire();
    }

    public void rateLimitNotify() {
        apiThrottle.startChoke();
    }

    class Throttle {
        private final ScheduledExecutorService executor = Executors.newScheduledThreadPool(1);
        private ScheduledFuture future;

        public void startChoke() {
            future = executor.schedule(new EndThrottle(this), 5, TimeUnit.SECONDS);
        }

        public void waitChoke() throws InterruptedException, ExecutionException {
            if(future == null || future.isDone() || future.isCancelled()) 
                return;
            future.get();
        }
    }

    class EndThrottle implements Runnable {

        private Throttle throttle;
        @Override 
        public void run() {
            System.out.println("I feel like I should be doing something with the throttle here");
        }

        public EndThrottle(Throttle throttle) {
            this.throttle = throttle;
        }
    }
}

Multiple threads will be using this limiter and the API, and they may each encounter throttles. This allows each thread to encounter any throttle notification and tell the limiter, and it only keeps record of the most recently encountered one, since it will also be the last to expire.

Initially, I thought I would need to synchronize everywhere, but in the end, I didn't use any. Is this code still usable from multiple threads?

As a side question- ScheduledExecutorService seemed like the best way to delay without forcing a block unless something needs to wait, but it asks for a Runnable, which I don't seem to really utilize. Is this poor design?

Answer 1

Initially, I thought I would need to synchronize everywhere, but in the end, I didn't use any. Is this code still usable from multiple threads?

No. When one thread sets

private ScheduledFuture future;

other may not see it in

    public void waitChoke() throws InterruptedException, ExecutionException {
        if(future == null || future.isDone() || future.isCancelled()) 
            return;
        future.get();
    }

Making future volatile would help. However, there's another problem with it changing before the test and future.get(). Here, a local variable would help.

As a side question- ScheduledExecutorService seemed like the best way to delay without forcing a block unless something needs to wait, but it asks for a Runnable, which I don't seem to really utilize. Is this poor design?

I guess, it's fine but you've overcomplicated it. Do you need EndThrottle#throttle? Do you need EndThrottle at all?

Why not just this?

future = executor.schedule(new Runnable(), 5, TimeUnit.SECONDS);

Overall, it doesn't look bad, but concerning thread-safety, there may be more problems.