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

util: PollSemaphore should expose available_permits #3682

Closed
hawkw opened this issue Apr 7, 2021 · 1 comment · Fixed by #3683
Closed

util: PollSemaphore should expose available_permits #3682

hawkw opened this issue Apr 7, 2021 · 1 comment · Fixed by #3683
Labels
A-tokio-util Area: The tokio-util crate C-enhancement Category: A PR with an enhancement or bugfix. C-feature-request Category: A feature request. E-easy Call for participation: Experience needed to fix: Easy / not much E-help-wanted Call for participation: Help is requested to fix this issue.

Comments

@hawkw
Copy link
Member

hawkw commented Apr 7, 2021

Is your feature request related to a problem? Please describe.
The tokio::sync::Semaphore type provides a Semaphore::available_permits method which returns the current number of permits available on the semaphore. tokio_util::sync::PollSemaphore does not expose such a method. It is possible to use PollSemaphore::into_inner or PollSemaphore::clone_inner to unwrap the inner semaphore, but this may require cloning/dropping the semaphore's Arc which shouldn't be necessary to access the permits.

Describe the solution you'd like
We should add a PollSemaphore::available_permits that calls Semaphore::available_permits on the inner Semaphore. We could probably also add a PollSemaphore::add_permits method while we're here. This should be pretty trivial to add.

Describe alternatives you've considered
Alternatively, we could just add a PollSemaphore::inner_ref or similar method for borrowing the inner Semaphore. PollSemaphore could even implement AsRef<Semaphore>.

This might be a little more complex to use (semaphore.as_ref().available_permits() vs semaphore.available_permits()) and is less visible in the API docs, but it would let us easily expose all the methods on the inner type transparently without having to wrap them. This could be both a pro or a con, if there are methods of the inner Semaphore type that PollSemaphore should not expose...

@hawkw hawkw added C-enhancement Category: A PR with an enhancement or bugfix. E-easy Call for participation: Experience needed to fix: Easy / not much A-tokio-util Area: The tokio-util crate C-feature-request Category: A feature request. labels Apr 7, 2021
@Darksonn Darksonn added the E-help-wanted Call for participation: Help is requested to fix this issue. label Apr 7, 2021
@Darksonn
Copy link
Contributor

Darksonn commented Apr 7, 2021

I thought we already had an impl AsRef<Semaphore> for PollSemaphore, but I see that it is missing. It would be nice to add that too.

hawkw added a commit that referenced this issue Apr 7, 2021
## Motivation

The `tokio::sync::Semaphore` type provides a
[`Semaphore::available_permits` method][1] which returns the current
number of permits available on the semaphore.
`tokio_util::sync::PollSemaphore` [does not expose such a method][2]. It
is possible to use `PollSemaphore::into_inner` or
`PollSemaphore::clone_inner` to unwrap the inner semaphore, but this may
require cloning/dropping the semaphore's `Arc` which shouldn't be
necessary to access the permits.

## Solution

This commit adds `PollSemaphore::available_permits`. It also adds
`PollSemaphore::add_permits` and an `AsRef<Semaphore>` impl while
we're here.

[1]: https://docs.rs/tokio/1.4.0/tokio/sync/struct.Semaphore.html#method.available_permits
[2]: https://docs.rs/tokio-util/0.6.5/tokio_util/sync/struct.PollSemaphore.html#implementations

Closes #3682
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 C-enhancement Category: A PR with an enhancement or bugfix. C-feature-request Category: A feature request. E-easy Call for participation: Experience needed to fix: Easy / not much E-help-wanted Call for participation: Help is requested to fix this issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants