-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Add public semaphore implementation #1973
Add public semaphore implementation #1973
Conversation
01eab80
to
a075294
Compare
a075294
to
5b25b1d
Compare
tokio/src/sync/semaphore.rs
Outdated
/// will never return and wait for new permits to be added until `n` | ||
/// permits were removed from the semaphore. | ||
pub async fn remove_permits(&self, n: usize) -> Result<(), AcquireError> { | ||
for _ in 0..n { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Such an API is tricky, because it allows for deadlocks. Imagine a few tasks, which all try to acquire n
permits. Since every task might get a few of them they will eventually all be stuck. A Semaphore
should be fair towards users, recognize that a caller required a higher amount of permits and reserve them for the caller. In a similar fashion as a reader/writer lock must give the writer some priority in order to prevent it from ever getting scheduled.
I'm not sure if that is possible by building on the existing low level semaphore, or whether you would need a different implementation for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's also weird that this doesn't return a Permit
, so the user manually needs to release everything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Such an API is tricky, because it allows for deadlocks. Imagine a few tasks, which all try to acquire
n
permits.
Aquiring multiple permits at the same time is not currently in the scope of the Semaphore implementation. It would be a nice feature to have but also require a completely different implementation.
It's also weird that this doesn't return a Permit, so the user manually needs to release everything.
remove_permits
aquires n
permits and calls forget
on them. A forgotten permit is consumed and can't be returned. What do you mean by the user manually needs to release everything
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that w/ Matthias
here, if we support n
it should be an "atomic" op. Could we just leave this fn out for now and revisit later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I just wanted to have a counter part to add_permits
. It had to be async
because it needs to wait for all permits to become availabl, but there is nothing wrong with removing it or putting it behind a feature flag for the time being. It's simple to write this code in your own code if ever needed.
tokio/src/sync/semaphore.rs
Outdated
} | ||
|
||
self.add_permits_locked(0, true); | ||
pub fn close(&self) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find closing a Semaphore
a bit akward. While it can certainly be enabled using this underlying primitive, the purpose is a bit unclear. And any other framework doesn't allow it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have never seen this kind of API before either. The POSIX semaphore API looks a lot different: sem_wait
, sem_trywait
, sem_timedwait
, sem_getvalue
, sem_post
, ...
Maybe calling that thing Semaphore
is wrong altogether. How about a different name like Multilock
or `Multipass'?
I quite like the idea of being able to close
it. It adds symmetry with channel
where closing the Receiver
also makes the Sender::try_send
fail with TrySendErrro::Closed
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove close
from this type here. It was mostly added for mpsc
. If we hit a use case, we can revisit it for 0.3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found that one to be quite useful when implementing some kind of "shutdown", but I'm fine with removing it from the API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering: Shall I remove both remove_permits
and try_remove_permits
from the PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When removing the close
function the case where acquire
returns an Err
should no longer happen so the AcquireError
will go away. The TryAcquireError
can also be replaced by an Option
as there is now only one possible reason why no permit was returned.
tokio/src/sync/semaphore.rs
Outdated
#[derive(Debug)] | ||
enum ErrorKind { | ||
pub enum TryAcquireError { | ||
/// The semaphore is closed and no new permits can be acquired. | ||
Closed, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the other comment about closing
a Semaphore
I know the low level Semaphore is already available in tokio, and it makes sense to reuse things. However from a usage point of view it might make sense to port the futures_intrusive::sync::Semaphore over - or to wrap it. It has some useful features which this one is lacking so far. E.g. one can fairly await more than one permit. And the |
|
Re: using The goal of this PR is to get "something" up w/ a minimal API set that is forwards compatible and we can revisit the details over time. I agree that it isn't a "perfect" solution. |
- Remove functions: remove_permits, try_remove_permits, close - Remove error enums: AcquireError, TryAcquireError
I hate to see the code go. I reduced the API to a bare minimum. |
Add public semaphore implementation
Motivation
Semaphores went missing from the public tokio interface. See #1825
Solution
This PR adds a new Semaphore interface which is a lot simpler to use and less error prone. Permits which are dropped are automatically returned to the Semaphore.
The private and low level semaphore module was previously renamed to
semaphore_ll
(low level).