Parametrize the rate limiter with sleep-fn

[marksto]

Hi @sunng87!

Here's a proposal for making the existing Rate Limiter implementation more malleable to changes, in particular to different sleep semantics. (This keeps the bucket’s tokens accounting logic intact, just improves the overall design a bit.)

The current Thread/sleep is interruptible in a sense that an InterruptedException is not given any special treatment, which may leave a rate limiter in an incorrect state wrt to the :reserved-tokens state. While this may not be that critical for most applications (the state will be incorrect per se, but the calls will also not be executed, so the main rate limit invariant will be preserved), there are cases when the degradation of throughput is not desirable/acceptable — tight quotas, SLAs, multiple clients sharing the same limiter with a single quota, etc. In these cases it makes sense to have some sort of a rollback pattern in place (for instance, like in Bucket4j methods) so to keep the bucket’s token accounting correct.

A trivial example of such rollback function would be:

(defn- rollback!
  "Undo a reservation of `permits` back into the bucket."
  [^TokenBucketRateLimiter rate-limiter permits]
  (swap! (.-state rate-limiter) update :reserved-tokens - permits))

which is to be called upon an InterruptedException during Thread/sleep. However, the existing design does not provide for such expansion, and I thought about how this could be added in the most unobtrusive way — an obvious candidate is to provide a new parameter, i.e. sleep-fn.

Another approach to the problem would be to have sleeps uninterruptible. For example, the Bucket4j has such blocking strategy built-in. While this may help keep the rate limiter in a correct state, it has its obvious drawbacks and it also requires similar redesign (a parametrisation with sleep-fn).

I tried to make commits atomic and self-explanatory. But, please, note that this is nothing more than a suggestion for possible design improvements. I am open to discussing any alternatives and/or changes, as well as eventually providing some concrete sleep-fn implementations (this PR is a groundwork, after all).

Cheers,
Mark

[sunng87]

@marksto Thank you for the patch. Overall this looks good to me. I will take a look deep into the detail tomorrow when I find time.

[sunng87]

This looks good to me. I hope you can include some other candidates of sleep-fn impl to build into the library. And for the rollback function, are you going to implement it in a sleep function?

[marksto]

@sunng87 Hi Ning!

I hope you can include some other candidates of sleep-fn impl to build into the library.

Yes, I can add one, which endorses an uninterruptible sleep instead of a regular one. I will add this in the scope of the current PR in a few days, if you don't mind. There's no rush. =)

And for the rollback function, are you going to implement it in a sleep function?

Yep, that was the point behind having this sleep-fn parametrised with everything that one may need in order to implement the rollback functionality (as well).

[sunng87]

@marksto Sounds good.

We can have:

• normal sleep (Thread/sleep)
• rollback-able sleep
• uninterrupt-able sleep

[marksto]

... the point behind having this sleep-fn parametrised with everything that one may need in order to implement the rollback functionality

After giving it some time I've figured out that this rollback feature does not belong to the sleep-fn (a.k.a. the "blocking strategy" in the Bucket4j's parlance), since the bucket's state is an atom and thus it requires CAS semantics in order to reset the state to the previous value only if it wasn't changed by another thread in the meanwhile. And that, in turn, requires a more substantial rewrite of the existing implementation of do-acquire! and do-try-acquire fns. (Which is a matter of a dedicated follow-up PR some day.)

That said, I've simplified the sleep-fn's contract, so that it just sleeps (somehow) for a given number of millis, and I've also added an uninterruptible counterpart to a normal sleep (and this is the exact implementation I use in my own project, so it has proven to work as expected). Tokens get refilled upon each call to the IRateLimiter protocol methods anyway. So, this should be just fine to have only tunable sleep semantics for now.

The only thing that is still missing are unit tests for the newly added uninterruptible sleep-fn. I will to add them soon.

[marksto]

Hi @sunng87! I finally had time to wrap things up by providing a comprehensive test coverage that also illustrates the subtle difference in various sleep semantics that the lib now provides us with. Would be more than happy to have this PR merged if everything looks reasonably good for you.

[sunng87]

@marksto Thank you! The patch looks good to me.

[marksto]

@sunng87 Would you be so kind to release a new version?

[sunng87]

Just uploaded 0.12.0. Let me know if there is any issue. Thank you for the patch!