-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
doc: Add examples to Semaphore
#3808
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking the time to do this!
I think generally the examples are more complicated than they need to be. You don't have to show off every feature of a method in its example, and if you feel the need to add { ... }
blocks inside the example, it should probably be two examples.
Also, don't be afraid of duplicating the same example several times. The for loop example from the beginning could absolutely make sense to have on several different methods throughout the documentation.
I think an example of the static SEM: Semaphore = Semaphore::const_new(10); |
Co-authored-by: Alice Ryhl <[email protected]>
@Darksonn Thank you for the quick feedback! I updated all of them to simplify as you suggested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems better now. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style nits, rest looks great to me now as well. cheers!
Co-authored-by: John-John Tedro <[email protected]>
@udoprog Thank you for the review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure these are my last comments.
tokio/src/sync/semaphore.rs
Outdated
/// ``` | ||
/// use std::sync::Arc; | ||
/// use tokio::sync::Semaphore; | ||
/// | ||
/// #[tokio::main] | ||
/// async fn main() { | ||
/// let semaphore = Arc::new(Semaphore::new(5)); | ||
/// let mut join_handles = Vec::new(); | ||
/// | ||
/// for _ in 1..=5 { | ||
/// let permit = semaphore.clone().try_acquire_owned().unwrap(); | ||
/// join_handles.push(tokio::spawn(async move { | ||
/// // perform task... | ||
/// // explicitly own `permit` in the task | ||
/// drop(permit); | ||
/// })); | ||
/// } | ||
/// | ||
/// for handle in join_handles { | ||
/// handle.await.unwrap(); | ||
/// } | ||
/// } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example isn't great. You wouldn't use try_acquire_owned
like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only realistic example I could think of that doesn't involve writing a poll method is something like this. Do you have any feedback?
use std::sync::Arc;
use tokio::sync::Semaphore;
use tokio_stream::{self as stream, StreamExt};
#[tokio::main]
async fn main() {
// Simulate a stream of incoming requests from some I/O resource
let mut requests = stream::iter(1u32..=20);
// Allow up to 4 concurrent operations
let semaphore = Arc::new(Semaphore::new(4));
let mut join_handles = Vec::new();
while let Some(id) = requests.next().await {
match Arc::clone(&semaphore).try_acquire_owned() {
Ok(permit) => {
join_handles.push(tokio::spawn(async move {
perform_operation(id).await;
// Hold the permit until our operation is over.
drop(permit);
}));
}
Err(TryAcquireError::NoPermits) => {
// We're busy, we can respond immediately to tell the client to try again later.
}
Err(TryAcquireError::Closed) => {
unreachable!() // no one calls `semaphore.close()`
}
}
}
for handle in join_handles {
let _ = handle.await;
}
}
async fn perform_operation(_id: u32) {
tokio::time::sleep(Duration::from_nanos(1)).await
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just do the same thing as the example on try_acquire
. It's a lot simpler even if it doesn't show off the move-to-new-task feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thank you for the review.
tokio/src/sync/semaphore.rs
Outdated
/// for _ in 1..=5 { | ||
/// let permit = semaphore.clone().try_acquire_many_owned(2).unwrap(); | ||
/// join_handles.push(tokio::spawn(async move { | ||
/// // perform task... | ||
/// // explicitly own `permit` in the task | ||
/// drop(permit); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
Motivation
Closes #3795
Add examples to
Semaphore
and its methods. All doctests pass. Let me know if you find anything too verbose or confusing.