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

Allow acquiring n permits from PollSemaphore #5137

Merged
merged 1 commit into from
Oct 30, 2022

Conversation

duarten
Copy link
Contributor

@duarten duarten commented Oct 27, 2022

Adds PollSemaphore::poll_acquire_many, to allow acquiring n permits
from it.

Motivation

Feature parity with Semaphore.

Solution

Introduce the more general poll_acquire_many function, to which the poll_acquire one now delegates.

@github-actions github-actions bot added the R-loom Run loom tests on this PR label Oct 27, 2022
@Darksonn Darksonn added A-tokio-util Area: The tokio-util crate M-sync Module: tokio/sync labels Oct 29, 2022
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 implementation is wrong. You're only handling the case where the permits are immediately available.

Regardless, I'm not sure this should be part of the poll method. Perhaps we should instead provide a method that sets the number of permits to acquire per acquire?

@duarten duarten force-pushed the duarte/poll-semaphore-acquire-n branch from a769f2b to 9c624c5 Compare October 29, 2022 16:17
@duarten
Copy link
Contributor Author

duarten commented Oct 29, 2022

This implementation is wrong. You're only handling the case where the permits are immediately available.

Oops, fixed.

Regardless, I'm not sure this should be part of the poll method. Perhaps we should instead provide a method that sets the number of permits to acquire per acquire?

That seems much less ergonomic, especially since the number of permits to acquire may change over time (when using a semaphore to limit requests with different weights or when bounding memory usage).

@Darksonn
Copy link
Contributor

You have to reset the future every time you wish to change the number of permits you wish to acquire. Otherwise the method will return a different number of permits than what you're asking for, if the previous permits had a different count.

You would need to add a test for this kind of thing.

@duarten duarten force-pushed the duarte/poll-semaphore-acquire-n branch 5 times, most recently from 286902e to d722c37 Compare October 29, 2022 17:36
tokio-util/src/sync/poll_semaphore.rs Outdated Show resolved Hide resolved
tokio-util/src/sync/poll_semaphore.rs Outdated Show resolved Hide resolved
@duarten duarten force-pushed the duarte/poll-semaphore-acquire-n branch 2 times, most recently from 83ea7d8 to 59ddfd4 Compare October 29, 2022 21:04
Adds `PollSemaphore::poll_acquire_many`, to allow acquiring `n` permits
from it.
@duarten duarten force-pushed the duarte/poll-semaphore-acquire-n branch from 59ddfd4 to 7b655ab Compare October 29, 2022 21:04
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 29c6de0 into tokio-rs:master Oct 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio-util Area: The tokio-util crate M-sync Module: tokio/sync R-loom Run loom tests on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants