add token bucket example to Semaphore #5978
Conversation
Darksonn
added
T-docs
tokio/src/sync/semaphore.rs
Outdated
| /// let time_slice = 1.0 / (rate as f32); | ||
| /// | ||
| /// loop { | ||
| /// sleep(Duration::from_secs_f32(time_slice)).await; |
There was a problem hiding this comment.
We should use a tokio::time::Interval instead.
tokio/src/sync/semaphore.rs
Outdated
| /// } | ||
| /// | ||
| /// impl TokenBucket { | ||
| /// fn new(rate: usize) -> Self { |
There was a problem hiding this comment.
Perhaps this should just take a Duration of the amount of time there should be between each permit? That would also allow for permits being added more rarely than once per second.
tokio/src/sync/semaphore.rs
Outdated
| /// let cap = rate - sem.available_permits(); | ||
| /// sem.add_permits(std::cmp::min(cap, 1)); |
There was a problem hiding this comment.
This will let the number of permits grow infinitely large.
There was a problem hiding this comment.
Sorry, I misread. Anyway, please use usize::min instead of std::cmp::min.
tokio/src/sync/semaphore.rs
Outdated
| /// Implement a simple token bucket for rate limiting | ||
| /// | ||
| /// Many applications and systems have constraints on the rate at which certain | ||
| /// operations should occur. Exceeding this rate can result in suboptimal | ||
| /// performance or even errors. | ||
| /// | ||
| /// The provided example uses a `TokenBucket`, implemented using a semaphore, that | ||
| /// limits operations to a specific rate. The token bucket will be refilled gradually. | ||
| /// When the rate is exceeded, the `acquire` method will await until a token is available. |
There was a problem hiding this comment.
We should mention two things here:
- Token buckets allow short bursts that are faster than the allowed rate. In fact, this is part of the point of token buckets.
- This implementation is suboptimal when the rate is large, because it consumes a lot of cpu constantly looping and sleeping.
tokio/src/sync/semaphore.rs
Outdated
| /// async fn acquire(&self) -> Result<SemaphorePermit<'_>, AcquireError> { | ||
| /// self.sem.acquire().await | ||
| /// } |
There was a problem hiding this comment.
I would probably forget the permit here. Otherwise, the user might use this incorrectly and not forget the permit.
|
Can you resolve the merge conflicts? |
maminrayej
force-pushed
the
sem-token-bucket
branch
from
54657a3
to
199b52f
Compare
|
I've rebased onto the master branch and didn't encounter any conflicts. |
maminrayej
force-pushed
the
sem-token-bucket
branch
from
b9d9486
to
84f5eea
Compare
tokio/src/sync/semaphore.rs
Outdated
| /// | ||
| /// impl TokenBucket { | ||
| /// fn new(duration: Duration) -> Self { | ||
| /// let rate = (1.0 / duration.as_secs_f32()) as usize; |
There was a problem hiding this comment.
You're only using this to compute the maximum capacity. Instead, I suggest making the capacity into a separate argument.
There was a problem hiding this comment.
Well the capacity is calculated based on the duration. If one specifies the rate, the duration is determined, and visa versa. Do you mean two new functions, one with duration and one with rate as an argument?
There was a problem hiding this comment.
No, I mean one function with both parameters. Token buckets usually don't compute the capacity based on the duration.
Co-authored-by: Alice Ryhl <aliceryhl@google.com>
Co-authored-by: Alice Ryhl <aliceryhl@google.com>
Co-authored-by: Alice Ryhl <aliceryhl@google.com>
|
I found one typo that's my fault. Otherwise, this looks good to me. |
Co-authored-by: Alice Ryhl <aliceryhl@google.com>
Implements the token bucket example as listed in #5933.