Which one would you prefer: 1. ```rateLimiter.acqu...

# codereview

[Klitos Kyriacou]

Which one would you prefer:
1.

rateLimiter.acquire()
while (System.nanoTime() < endTime) {
    foo()
    bar()
    baz()
    if (System.nanoTime() < endTime)
       rateLimiter.acquire()
}

2.

while (true) {
    rateLimiter.acquire()
    if (System.nanoTime() >= endTime)
        break
    foo()
    bar()
    baz()
}

3.

while (run { rateLimiter.acquire(); System.nanoTime() < endTime }) {
    foo()
    bar()
    baz()
}

[Joffrey]

The 3 cases don't seem to be equivalent. I don't understand the invariant here. Do you need acquire() to only be called when endTime is not reached? In all 3 cases, the first acquire() is done regardless of the current time. In case #1, it seems important to check if endTime is not reached before calling acquire() again (otherwise the if has no reason to be there). But in case 2 and 3, acquire() is called after foo/bar/baz regardless of the current time.

[Klitos Kyriacou]

Good point. The idea was to temporally space the function calls foo(), bar(), baz() without adding an unnecessary wait at the end of the loop (hence the if guard for the rateLimiter in #1). In fact, the calls to foo(), bar() and baz() are almost instantaneous and therefore the call to rateLimiter.acquire() is guarded by the condition from the previous iteration of the loop.

[Joffrey]

In that case it seems to me that this shouldn't really be a problem:

while (System.nanoTime() < endTime) {
    rateLimiter.acquire()
    foo()
    bar()
    baz()
}

acquire() might take some time, and bring you past endTime, but since foo/bar/baz is almost instant, you don't lose much by executing them one extra time. Semantically, it must not be an issue to execute foo/bar/baz after endTime because in your current options, you could be checking the condition 1ns before the end and it would basically mean that you run foo/bar/baz after the end too.

[Klitos Kyriacou]

Thanks. After thinking about it, I agree. I was too concerned that if the rate limit was 10 per second and I ran for 1 second, I should call foo() 10 times, instead of 11 (because rateLimiter.acquire() returns immediately on the first call). But that shouldn't really matter much.

[Joffrey]

You could also consider making aquire() suspending (if not already) and use withTimeout to immediately cancel a pending acquire() instead of relying on a loop condition checking the current nanoTime against a pre-computed endTime :

withTimeout(runDuration) {
    while(true) {
        rateLimiter.acquire()
        foo()
        bar()
        baz()
    }
}

This way you never wait for a long acquire() past the expected end time

[Klitos Kyriacou]

That's neat! The rateLimiter is Guava's old RateLimiter, so it's not suspending per se, but it can be wrapped into one.

[ephemient]

if you're using coroutines you might want to choose something which uses the built-in delay infrastructure instead of forcing you to block a thread

[Klitos Kyriacou]

Is there a coroutine-friendly rate limiter? The ones I know of are from Guava, resilience4j and buckets4j, all of which are Java libraries.

[ephemient]

looks like https://github.com/google/guava/blob/master/guava/src/com/google/common/util/concurrent/RateLimiter.java uses sleepUninterruptibly so even wrapping it in runInterruptible doesn't really help