Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add token bucket example to Semaphore #5978

Merged
merged 10 commits into from
Sep 23, 2023

Conversation

maminrayej
Copy link
Member

Implements the token bucket example as listed in #5933.

@github-actions github-actions bot added the R-loom-sync Run loom sync tests on this PR label Sep 3, 2023
@Darksonn Darksonn added T-docs Topic: documentation A-tokio Area: The main tokio crate M-sync Module: tokio/sync labels Sep 4, 2023
/// let time_slice = 1.0 / (rate as f32);
///
/// loop {
/// sleep(Duration::from_secs_f32(time_slice)).await;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use a tokio::time::Interval instead.

/// }
///
/// impl TokenBucket {
/// fn new(rate: usize) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 138 to 142
/// let cap = rate - sem.available_permits();
/// sem.add_permits(std::cmp::min(cap, 1));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will let the number of permits grow infinitely large.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I misread. Anyway, please use usize::min instead of std::cmp::min.

Comment on lines 105 to 113
/// 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 147 to 152
/// async fn acquire(&self) -> Result<SemaphorePermit<'_>, AcquireError> {
/// self.sem.acquire().await
/// }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would probably forget the permit here. Otherwise, the user might use this incorrectly and not forget the permit.

@Darksonn
Copy link
Contributor

Darksonn commented Sep 8, 2023

Can you resolve the merge conflicts?

@maminrayej
Copy link
Member Author

I've rebased onto the master branch and didn't encounter any conflicts.

tokio/src/sync/semaphore.rs Outdated Show resolved Hide resolved
///
/// impl TokenBucket {
/// fn new(duration: Duration) -> Self {
/// let rate = (1.0 / duration.as_secs_f32()) as usize;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're only using this to compute the maximum capacity. Instead, I suggest making the capacity into a separate argument.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I mean one function with both parameters. Token buckets usually don't compute the capacity based on the duration.

tokio/src/sync/semaphore.rs Outdated Show resolved Hide resolved
tokio/src/sync/semaphore.rs Outdated Show resolved Hide resolved
maminrayej and others added 5 commits September 23, 2023 12:11
Co-authored-by: Alice Ryhl <aliceryhl@google.com>
Co-authored-by: Alice Ryhl <aliceryhl@google.com>
Co-authored-by: Alice Ryhl <aliceryhl@google.com>
@Darksonn
Copy link
Contributor

I found one typo that's my fault. Otherwise, this looks good to me.

Co-authored-by: Alice Ryhl <aliceryhl@google.com>
Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

@Darksonn Darksonn merged commit f5b8cf9 into tokio-rs:master Sep 23, 2023
69 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate M-sync Module: tokio/sync R-loom-sync Run loom sync tests on this PR T-docs Topic: documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants