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

Add a new ManualResetEvent synchronization primitive #1315

Closed
wants to merge 8 commits into from

Conversation

Matthias247
Copy link
Contributor

The primitive can be used to let one or multiple tasks wait for a certain event.
Compared to the existing oneshot channel, the difference is that the ManualResetEvent doesn't carry a value, and is reusable.

In addition to the thread-safe ManualResetEvent a non-allocating LocalManualResetEvent variant is provided. This can be e.g. used as a cancellation token in nested asynchronous functions.

This CR is currently a bit raw and mostly about asking for feedback. The goal was not only to implement the primitive, but also to try to see how far one get with non-allocating futures by exploiting Pinning (as described in #1272). This turned out to work quite well. Even though the synchronous cancellation requirement for Drop() requires the usage of a Mutex and thereby makes things more complicated compared to the C++ implementation at https://lewissbaker.github.io/2017/11/17/understanding-operator-co-await.

For simplicity things have been added into the channel module and into some async/await tests. However another module might be better suited. Also better documentation is missing. And the usage of the pinning APIs could certainly be improved.

Some use-cases for the primitive:

  • Using it for implementing poll_ready() on higher level streams, e.g. TCP/HTTP/TLS/etc streams
  • Using it as a cancellation token

}

/// Returns a future that gets fulfilled when the event is set.
pub fn poll_set(&'a self) -> impl Future<Output = ()> + FusedFuture + 'a {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally poll_* functions are ones that return Poll<_>, like InnerEventWaiter::poll_waiter. Is there some other appropriate name for this? Maybe just wait?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right! I derived the name from the existing async code like AsyncRead. But since those are not really awaitable futures the situation is a bit different.
How about get_awaiter()? That's a bit aligned with the C# terminology. get_future() is obviously also ok, but less specific.

@Nemo157
Copy link
Member

Nemo157 commented Nov 8, 2018

The intrusive list part of this looks correct to me (although, I haven't really thought about panic safety). Is it possible to pull it out to a module that provides a fully safe generic API? That would remove all unsafety except pin-projection from the actual ManualResetEvent code.

let mut nr_selects = 0;

// Since the futures from poll_set are Unpin, they must be alias by a reference which is
// !Unpin for select
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The inverse, poll_set returns an impl Future + !Unpin which pin_mut!() allows you to treat as an impl Future + Unpin by pre-pinning it to the current stack.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching it, I got confused while writing this. Changed it.

@Matthias247
Copy link
Contributor Author

The intrusive list part of this looks correct to me (although, I haven't really thought about panic safety). Is it possible to pull it out to a module that provides a fully safe generic API? That would remove all unsafety except pin-projection from the actual ManualResetEvent code.

It probably is, but I'm not sure if it's beneficial as long as it's not reused by another module (like an async Semaphore for example). Currently I think I find it preferable to understand all the logic in a single place, since it isn't that much code.

The primitive can be used to let one or multiple tasks wait for a certain event.
Compared to the existing oneshot channel, the difference is that the ManualResetEvent doesn't carry a value, and is reusable.

In addition to the thread-safe ManualResetEvent a non-allocating LocalManualResetEvent variant is provided. This can be e.g. used as a cancellation token in nested asynchronous functions.
@Matthias247
Copy link
Contributor Author

Replaced by #1389

@Matthias247 Matthias247 closed this Jan 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants