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

Semaphore example using an Arc<Semaphore> for semaphore passing #5956

Merged
merged 33 commits into from
Sep 19, 2023

Conversation

alexanderkirilin
Copy link
Contributor

A simple example of limiting request processing using a task-safe semaphore and demonstrating acquire_owned for: #5933

@github-actions github-actions bot added the R-loom-sync Run loom sync tests on this PR label Aug 27, 2023
@Darksonn Darksonn added T-docs Topic: documentation A-tokio Area: The main tokio crate M-sync Module: tokio/sync labels Aug 28, 2023
@Darksonn
Copy link
Contributor

Your branch is behind #5939, which has already been merged. Would you be able to update your branch to latest master? You should be able to do this by merging master into your branch and fixing the resulting conflicts.

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

alexanderkirilin commented Aug 29, 2023

@Darksonn I think the event handler loop in the updated example is causing issues with CI.

The test suite has been running for 30 minutes so far, and I can't find a way to cancel the workflows.

Would you be able to stop it? Workflow: https://github.com/tokio-rs/tokio/actions/runs/6014142339

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 think this is looking pretty good. The example could use a few more comments on the semaphore-related stuff.

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
alexanderkirilin and others added 4 commits August 29, 2023 12:49
Co-authored-by: Alice Ryhl <aliceryhl@google.com>
…aphore-example-file

# Conflicts:
#	tokio/src/sync/semaphore.rs
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
alexanderkirilin and others added 4 commits August 31, 2023 07:16
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.

It's looking good. I have a few comments to simplify the language a bit.

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
tokio/src/sync/semaphore.rs Outdated Show resolved Hide resolved
alexanderkirilin and others added 6 commits September 1, 2023 08:54
Co-authored-by: Alice Ryhl <aliceryhl@google.com>
Co-authored-by: Alice Ryhl <aliceryhl@google.com>
Co-authored-by: Alice Ryhl <aliceryhl@google.com>
Co-authored-by: Alice Ryhl <aliceryhl@google.com>
Co-authored-by: Alice Ryhl <aliceryhl@google.com>
@alexanderkirilin
Copy link
Contributor Author

@Darksonn Thank you for the suggestions. This looks much more concise and focused than what I had begun with; and I've learned a few things about technical writing by reflecting on them.

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 think these suggestions are the last ones. Then I'm happy to merge this.

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 Show resolved Hide resolved
alexanderkirilin and others added 5 commits September 6, 2023 07:31
Co-authored-by: Alice Ryhl <aliceryhl@google.com>
Co-authored-by: Alice Ryhl <aliceryhl@google.com>
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! Looks good to me.

@alexanderkirilin
Copy link
Contributor Author

@Darksonn Merged latest commits from upstream. Would this be able to be merged?

@Darksonn Darksonn merged commit e6553c4 into tokio-rs:master Sep 19, 2023
70 checks passed
@Darksonn
Copy link
Contributor

Sorry, seems like I forgot to press the button.

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