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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 33 commits into from
Sep 19, 2023
Merged

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

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
@alexanderkirilin alexanderkirilin mentioned this pull request Aug 27, 2023
Closed
@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.

Darksonn
Darksonn reviewed Aug 29, 2023
View reviewed changes
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 •
edited
Loading

@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

Darksonn
Darksonn reviewed Aug 29, 2023
View reviewed changes
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
Darksonn
Darksonn reviewed Aug 31, 2023
View reviewed changes
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
Darksonn reviewed Sep 1, 2023
View reviewed changes
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
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.

Darksonn
Darksonn reviewed Sep 3, 2023
View reviewed changes
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
Darksonn
Darksonn approved these changes Sep 6, 2023
View reviewed changes
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
69 checks passed
@Darksonn
Copy link
Contributor

Sorry, seems like I forgot to press the button.

@satakuma satakuma mentioned this pull request Oct 8, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Darksonn Darksonn Darksonn approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alexanderkirilin @Darksonn