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

Open
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@Matthias247
Contributor

Matthias247 commented Nov 7, 2018

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

@Matthias247 Matthias247 force-pushed the Matthias247:event branch from 80b0e5c to 5821331 Nov 7, 2018

}

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

This comment has been minimized.

@Nemo157

Nemo157 Nov 8, 2018

Member

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

This comment has been minimized.

@Matthias247

Matthias247 Nov 9, 2018

Contributor

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

This comment has been minimized.

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

This comment has been minimized.

@Nemo157

Nemo157 Nov 8, 2018

Member

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.

This comment has been minimized.

@Matthias247

Matthias247 Nov 9, 2018

Contributor

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

@Matthias247 Matthias247 referenced this pull request Nov 9, 2018

Open

[Stabilization] Pin APIs #55766

0 of 5 tasks complete
@Matthias247

This comment has been minimized.

Contributor

Matthias247 commented Nov 9, 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.

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.

Matthias247 added some commits Nov 7, 2018

Add a new ManualResetEvent synchronization primitive.
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 Matthias247 force-pushed the Matthias247:event branch from f450f5a to 33f6330 Dec 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment