antifuchs / ratelimit_meter

A leaky-bucket-as-a-meter rate-limiting implementation in Rust
MIT License
33 stars 6 forks source link

Remove Decision? #12

Closed waywardmonkeys closed 6 years ago

waywardmonkeys commented 7 years ago

You could use Result instead of having Decision<T> and then Result<Decision<T>>

In this case, you'd use () as the success type and the existing T for the failure case: Result<(), T>

This would improve the usability and readability, I suspect and let you get rid of some usages of unwrap where it doesn't seem to add a lot.

Or are there cases where you'd return a failing result that wasn't a failed decision?

antifuchs commented 7 years ago

That's a tough one. On the one hand, I do agree that Result for these results would make sense. On the other, there are cases where an implementation of a rate-limiter would be completely right to fail without being able to provide a decision. The Threadsafe rate limiter is an example of that - if acquiring the mutex would fail, it can't make a decision at all.

HOWEVER, note the existence of #10 - with lockfree rate-limiters, we can get rid of Threadsafe (at a fractional cost to single-threaded performance, but at sub-microsecond/op speeds, I don't think people will care a whole ton), which will allow getting rid of that Result wrapper, which would allow us to use straight-up Results as decisions.

So, I think that's a cool idea. Blocked on another thing, but I'll take a look at it once the lockless work is out.

antifuchs commented 6 years ago

Done in #15! Whee!