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

Provide select! macro #2152

Merged
merged 10 commits into from
Jan 23, 2020
Merged

Provide select! macro #2152

merged 10 commits into from
Jan 23, 2020

Conversation

carllerche
Copy link
Member

@carllerche carllerche commented Jan 22, 2020

The select! operation is a key operation when writing asynchronous Rust. Up until now, the futures crate provided the main implementation. This PR adds a new select! implementation that improves on the version from futures in a few ways:

  • Avoids needing FusedFuture.
  • Implemented as a hybrid declarative / proc macro, avoiding proc-macro-hack.

Avoiding FusedFuture

The original select! macro requires that provided futures implement a special trait: FusedFuture. Unfortunately, futures returned by async fn do not implement FusedFuture, leading to the requirement of having to call .fuse() on futures before calling select!. The FuseFuture requirement exists to support using select! from within a loop.

The select! implementation in this PR avoids the need for a FusedFuture trait by adding two new features to select!: pattern matching and branch conditions.

The core of the "select! in a loop" problem is that future values may not be used again after they complete, so when using select! from within a loop, branches that have completed on prior loop iterations must somehow be "disabled". The strategy used by futures::select! is to require input futures to implement [FusedFuture]. The FusedFuture trait informs select! if the branch is to be disabled.

In this PR, we use branch conditions. This idea was initially proposed here by @Matthias247. The idea is that the user may supply a condition to guard a branch and informing select! to disable the branch if the future has previously completed. Here is an example of implementing join with select!.

let (tx1, mut rx1) = oneshot::channel();
let (tx2, mut rx2) = oneshot::channel();

tokio::spawn(async move {
    tx1.send("first").unwrap();
});

tokio::spawn(async move {
    tx2.send("second").unwrap();
});

let mut a = None;
let mut b = None;

while a.is_none() || b.is_none() {
    tokio::select! {
        v1 = (&mut rx1), if a.is_none() => a = Some(v1.unwrap()),
        v2 = (&mut rx2), if b.is_none() => b = Some(v2.unwrap()),
    }
}

let res = (a.unwrap(), b.unwrap());

assert_eq!(res.0, "first");
assert_eq!(res.1, "second");

Additionally, this PR builds upon the condition idea by also adding pattern matching. This provides a "post condition" ability where a select branch can be disabled after the future completes. Here is an example with selecting on streams:

let (tx1, mut rx1) = mpsc::unbounded_channel::<i32>();
let (tx2, mut rx2) = mpsc::unbounded_channel::<i32>();

tokio::spawn(async move {
    assert_ok!(tx2.send(1));
    task::yield_now().await;

    assert_ok!(tx1.send(2));
    task::yield_now().await;

    assert_ok!(tx2.send(3));
    task::yield_now().await;

    drop((tx1, tx2));
});

let mut rem = true;
let mut msgs = vec![];

while rem {
    tokio::select! {
        Some(x) = rx1.recv() => {
            msgs.push(x);
        }
        Some(y) = rx2.recv() => {
            msgs.push(y);
        }
        else => {
            rem = false;
        }
    }
}

msgs.sort();
assert_eq!(&msgs[..], &[1, 2, 3]);

The else branch is executed if all branches of a select call become disabled. This is equivalent to the completed branch in futures::select!.

Note that this macro does not support default branches. This behavior is orthogonal to select! and can be implementing using a separate utility.

Implementing (mostly) as a declarative macro

This select! implementation is implemented mostly as a declarative macro. Doing so avoids the need for the user to increase the rustc recursion limit. The macro recurses once per branch, so over 60 select! branches can be used before having to touch the recursion limit.

The decision to avoid a proc macro stems mostly from the fact that proc macros in expression position are not supported in stable Rust. The proc-macro-hack crate exists as a work around, but runs into limitations with regards to nesting. proc-macro-nested supports nesting proc-macro-hack calls, but results in hitting the rustc recursion limit very early. This, of course, is not a criticism of proc-macro-hack as @dtolnay does wonders with what is available. The select! implementation uses some tricks learned from reading proc-macro-hack source.

Provides a `select!` macro for concurrently waiting on multiple async
expressions. The macro has similar goals and syntax as the one provided
by the `futures` crate, but differs significantly in implementation.

First, this implementation does not require special traits to be
implemented on futures or streams (i.e., no `FuseFuture`). A design goal
is to be able to pass a "plain" async fn result into the select! macro.

Even without `FuseFuture`, this `select!` implementation is able to
handle all cases the `futures::select!` macro can handle. It does this
by supporting pre-poll conditions on branches and result pattern
matching. For pre-conditions, each branch is able to include a condition
that disables the branch if it evaluates to false. This allows the user
to guard futures that have already been polled, preventing double
polling. Pattern matching can be used to disable streams that complete.

A second big difference is the macro is implemented almost entirely as a
declarative macro. The biggest advantage to using this strategy is that
the user will not need to alter the rustc recursion limit except in the
most extreme cases.

The resulting future also tends to be smaller in many cases.
@carllerche
Copy link
Member Author

Of course, I should add that none of this would be possible w/o @cramertj doing the bulk of the initial exploration via futures::select! 👍

tokio/src/macros/select.rs Outdated Show resolved Hide resolved
tokio/src/macros/select.rs Show resolved Hide resolved
tokio/src/macros/select.rs Show resolved Hide resolved
tokio/src/macros/select.rs Outdated Show resolved Hide resolved
tokio/src/macros/select.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@udoprog udoprog left a comment

Choose a reason for hiding this comment

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

LGTM, only minor nits. Well done!

tokio/src/macros/select.rs Outdated Show resolved Hide resolved
tokio/src/macros/select.rs Outdated Show resolved Hide resolved
tokio/src/macros/select.rs Show resolved Hide resolved
Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

This is awesome! Great work!

tokio/src/macros/select.rs Outdated Show resolved Hide resolved
This makes the borrow checker happy
@cramertj
Copy link
Contributor

This looks neat! I'm glad to have a second set of eyes on this. futures-rs also initially used a declarative macro approach but I moved away from it since I had trouble getting it to give good error messages, and the code was harder to maintain. The way you wrote this looks much cleaner!

I'm curious about the FusedFuture bit, though-- I had really wanted to get rid of that as well, but I really struggled to come up with a design that was hard to misuse that didn't involve something like FusedFuture.

I'm actually curious about your examples in this regard. I don't understand how this loop wouldn't poll rx1 after completion if rx2 had a bunch more messages available.

while rem {
    tokio::select! {
        Some(x) = rx1.recv() => {
            msgs.push(x);
        }
        Some(y) = rx2.recv() => {
            msgs.push(y);
        }
        else => {
            rem = false;
        }
    }
}

The first time rx1.recv() returns a None, it'll disable that first branch, then get a value out of rx2.recv(). But then it starts the loop again, with the first branch no longer disabled (because the disabled mask gets re-initialized at the start of the loop) and will once again create rx1.recv() and poll it, which wouldn't work for e.g. stream.next() because it would return a future that would poll a stream which had already returned None. This seems like a potential footgun to me, WDYT?

@carllerche
Copy link
Member Author

@cramertj Thanks for the kind words.

getting it to give good error messages

I expect the declarative macro will give wonky error messages in some cases. This is one reason why I left a proc macro (that generates the Result). As bad error cases are discovered, this proc macro can be leveraged to provide better error messages. Another option is a "catch all" declarative macro rule that passes the input to a proc macro to generate an nice error message.

I'm actually curious about your examples in this regard. I don't understand how this loop wouldn't poll rx1 after completion if rx2 had a bunch more messages available.

It does poll rx1 after completion and relies on channel receivers being fused in the "traditional" (std::iter) sense. Any stream can be fused to always return None on every call to poll once complete.

@carllerche
Copy link
Member Author

As for whether or not it is a foot gun, it seems no more of a foot gun than std::iter is. One can call Iterator::next() in a loop even if the iterator has previously completed.

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

This is very cool!

I'm personally not a huge fan of the syntax used by any select macro (the <pat> = <expr> => <handler> syntax flows weirdly in my book) but it seems like the ship has sailed on that, so the implementation seems great. I really appreciate the internal notes on the macro, that makes it a lot easier to review!

tokio/src/future/poll_fn.rs Show resolved Hide resolved
tokio-macros/src/lib.rs Outdated Show resolved Hide resolved
tokio-macros/src/lib.rs Show resolved Hide resolved
tokio/src/lib.rs Outdated Show resolved Hide resolved
tokio/src/macros/select.rs Outdated Show resolved Hide resolved
tokio/src/macros/select.rs Outdated Show resolved Hide resolved
tokio/src/macros/select.rs Show resolved Hide resolved
tokio/src/macros/select.rs Outdated Show resolved Hide resolved
tokio/src/macros/select.rs Show resolved Hide resolved
tokio/src/macros/select.rs Show resolved Hide resolved
tokio-macros/src/select.rs Outdated Show resolved Hide resolved
@mystor
Copy link

mystor commented Jan 23, 2020

I think it's possible to entirely avoid the declare_output_enum! procedural macro and generate the Out and Mask types within macro_rules! macros, without doing lots of recursion. I threw together a macro which does this, if you're interested in removing the proc-macro dependency for select!: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=91355d65f96139b88245aaa910af641e

The main change is using a # character for counting variants, rather than a _ one, to allow the macro to distinguish between identifiers and placeholders better.

@carllerche
Copy link
Member Author

@mystor yep! you are correct. I opted to keep a proc macro component in order to provide improved error messages when invalid syntax is used.

@mystor
Copy link

mystor commented Jan 23, 2020

Sounds good :-) - I think some of the error messages produced by the proc-macro, such as "up to 64 branches supported", could be handled by a macro_rules! macro, but if there are other error benefits as well, then you should stick with that!

@carllerche
Copy link
Member Author

I expect forgetting a , before if will be common.

@carllerche carllerche merged commit 8cf98d6 into master Jan 23, 2020
@loyd
Copy link
Contributor

loyd commented Jan 28, 2020

Now it's incredibly easy to get panic using tokio::select inside loops with async fn futures. The approach of futures::select at least reminds about this foot gun.

@withoutboats
Copy link

As for whether or not it is a foot gun, it seems no more of a foot gun than std::iter is. One can call Iterator::next() in a loop even if the iterator has previously completed.

This doesn't seem comparable to me:

  1. Iterators very rarely do anything wrong when they are called after they finish, they almost always return None. But Futures most often panic in that scenario.
  2. The APIs for processing iterator really make it natural not to call next again - for elem in iter and while let Some(elem) = iter.next() (if you need to call next inside the loop) both self-terminate. But this puts all the responsibility on the user to implement a proper fuse guard themselves.

To me it seems sort of the opposite: with iterator, we have made it very easy to avoid a low stakes bug, but with this select design the user is fully responsible ofr avoiding a high stakes bug.

Personally, I wish we had just made FusedFuture::is_complete a required method on Future but we are past that point now! Basically every future has to track that state anyway. I wonder what we would be able to do. I guess we could make it a default method which is only heuristic somehow...


I assume (from omission) that this doesn't change anything about the relationship between Unpin and select, so I'd just mention my own obsession with the select API:

I think the vast majority of select use cases could be solved with two higher level APIs, both of which could pin futures themselves and not require unpin:

  • race! - this takes ownership of some futures, polls them all, runs only the response to the first that complete, drops the rest. You could even avoid the cancellation by passing in borrows of futures (because those also implement Future) instead of by ownership if you really needed it.
  • complete! - equivalent to select in a loop. Takes ownership of some futures, polls all of them to completion, runs the response code for each of them. This would still need a solution to the fuse problem.

Just mentioning these in case you're interested in exploring other concurrency primitives that could be a bit higher level than select and easier to use.

@carllerche
Copy link
Member Author

@withoutboats thanks for the thoughts.

Based on your description of race!, I'm not sure that I understand the difference between this macro and select! as provided here.

@carllerche
Copy link
Member Author

carllerche commented Jan 28, 2020

@loyd it is not "incredibly easy", the future needs to be both Unpin and passed by reference into select!. The default case, passing by ownership, would not hit an issue.

@rcastill
Copy link

Constructive comment ahead:

I started using futures::select and learned why FusedFuture is needed.

Then I used both futures::select and tokio::select interchangeably (thinking tokio::select was the same as future's). Some lines of code later it came to me... wait, why am I not required to .fuse() my futures?

Then after some googling I arrived here, and it took me a while to understand why this didn't need to .fuse() until I went to the docs (yes, my bad, I shouldn't have assumed before reading the docs).

And I must agree with @loyd: with futures::select you are aware why you are doing something (using .fuse()) and why the compiler shouts at you if you don't comply (the Rust way). With tokio's approach, I feel that the user is aware after being really careful or because his/her foot is already bleeding (C way).

All of this is from a single user standpoint. I don't know if my case is relatable.

@Darksonn
Copy link
Contributor

You may be interested in this thread.

@rcastill
Copy link

Nice post. I understand now some of the points against futures::select. But I think I prefer those tiny inconveniences over the foot gun. Maybe in the future there will be a better option, like a native way to select since it looks a bit hacky now.

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.