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

doc: Add examples to Semaphore #3808

Merged
merged 10 commits into from Jun 11, 2021

Conversation

oguzbilgener
Copy link
Sponsor Contributor

Motivation

Closes #3795

Add examples to Semaphore and its methods. All doctests pass. Let me know if you find anything too verbose or confusing.

@Darksonn Darksonn added A-tokio Area: The main tokio crate M-sync Module: tokio/sync T-docs Topic: documentation labels May 26, 2021
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 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.

tokio/src/sync/semaphore.rs Outdated Show resolved Hide resolved
tokio/src/sync/semaphore.rs Outdated Show resolved Hide resolved
tokio/src/sync/semaphore.rs Outdated Show resolved Hide resolved
tokio/src/sync/semaphore.rs Outdated Show resolved Hide resolved
@Darksonn
Copy link
Contributor

I think an example of the const_new method would be nice. It could be very short:

static SEM: Semaphore = Semaphore::const_new(10);

@oguzbilgener
Copy link
Sponsor Contributor Author

@Darksonn Thank you for the quick feedback! I updated all of them to simplify as you suggested.

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.

This seems better now. 👍

Copy link
Contributor

@udoprog udoprog left a 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!

tokio/src/sync/semaphore.rs Outdated Show resolved Hide resolved
tokio/src/sync/semaphore.rs Outdated Show resolved Hide resolved
tokio/src/sync/semaphore.rs Outdated Show resolved Hide resolved
tokio/src/sync/semaphore.rs Outdated Show resolved Hide resolved
tokio/src/sync/semaphore.rs Outdated Show resolved Hide resolved
oguzbilgener and others added 2 commits June 1, 2021 15:45
Co-authored-by: John-John Tedro <udoprog@tedro.se>
@oguzbilgener
Copy link
Sponsor Contributor Author

@udoprog Thank you for the review!

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.

I'm pretty sure these are my last comments.

tokio/src/sync/semaphore.rs Outdated Show resolved Hide resolved
Comment on lines 401 to 422
/// ```
/// 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();
/// }
/// }
Copy link
Contributor

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.

Copy link
Sponsor Contributor Author

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
}

Copy link
Contributor

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.

Copy link
Sponsor Contributor Author

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.

Comment on lines 458 to 463
/// 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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

@Darksonn Darksonn merged commit 2ab1fb0 into tokio-rs:master Jun 11, 2021
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 T-docs Topic: documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Semaphore needs examples
3 participants