Navigation Menu

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

RFC: add futures to libcore #2395

Closed
wants to merge 11 commits into from
Closed

RFC: add futures to libcore #2395

wants to merge 11 commits into from

Conversation

aturon
Copy link
Member

@aturon aturon commented Apr 6, 2018

⚠️⚠️ This RFC is superseded by #2418 ⚠️⚠️


This RFC proposes to add futures to libcore, in order to support the first-class async/await syntax proposed in a companion RFC. To start with, we add the smallest fragment of the futures-rs library required, but we anticipate follow-up RFCs ultimately bringing most of the library into libcore (to provide a complete complement of APIs).

The proposed APIs are based on the futures crate, but with two major changes:

  • The use of pinned types to enable borrowing within futures.
  • Removing the associated Error type (and adjusting combinators accordingly), in favor of just using Item = Result<T, E> instead. The RFC includes an extension trait to provide conveniences for Result-producing futures as well.

Rendered

@aturon aturon added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Apr 6, 2018
}

/// Check whether this error is the `shutdown` error.
pub fn is_shutdown() -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is missing a &self receiver

```rust
pub trait Future {
/// The type of value produced on completion.
type Item;
Copy link
Member

Choose a reason for hiding this comment

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

We were discussing this last week: I believe Output is more in line here with existing conventions (e.g. Fn traits, operator overloading traits), and Item would be more appropriate for Stream (by association with Iterator).

Copy link

Choose a reason for hiding this comment

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

Outcome. Just kidding

Copy link
Contributor

Choose a reason for hiding this comment

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

The main reason to use Item, from my perspective, is that it is 2 characters shorter than Output. For Fn traits this doesn't matter, they have -> syntax, but Future, like Iterator, will often be bound Future<Item = ?>.

I don't have a strong opinion about this choice.

Copy link
Contributor

@Centril Centril Apr 9, 2018

Choose a reason for hiding this comment

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

@withoutboats We could do some sort of Future() -> R deal for all traits of the form:

trait TraitName {
    type Output;
    // other stuff does not matter...
}

This might have a knock on benefit for people who define their own "Fn-like" traits.

Personally I think 2 more characters for a more apt name is the right way to go in this instance.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the Fn => Future, Iterator => Stream correspondence, and I think using Output will avoid confusion in places where futures/streams are being mixed.

Copy link
Member

Choose a reason for hiding this comment

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

@withoutboats I am in strong support of keeping it Item for the reason you mentioned. As someone who works w/ futures all day, I type Item constantly. The shorter name makes a big difference here.

The fn analogy doesn't make sense. a future is not a function. It is a value that will complete in the future.

Item matches Iterator which is closer to what a future is.

Copy link
Contributor

Choose a reason for hiding this comment

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

The fn analogy doesn't make sense. a future is not a function. It is a value that will complete in the future.

The analogy does make sense: both a Fn and a Future produce a single Output. Both an Iterator and a Stream produce multiple Items

Copy link
Member

Choose a reason for hiding this comment

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

To make my position a bit clearer: I think Item made more sense when we had something like Result<Self::Item, Self::Error>, since it was "the item in the result".

Copy link
Member

@cramertj cramertj Apr 17, 2018

Choose a reason for hiding this comment

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

@withoutboats

For Fn traits this doesn't matter, they have -> syntax, but Future, like Iterator, will often be bound Future<Item = ?>.

If we wanted to be really wild, we could make -> sugar work for the Future trait:

fn foo() -> impl Future -> u32 {
    println!("Foo called");
    async {
        println!("Foo polled");
        5
    }
}

fn serve(f: impl FnMut(Request) -> impl Future -> Response) {
    // serve http requests by calling `f`
    ....
}

/// of data on a socket, then the task is recorded so that when data arrives,
/// it is woken up (via `cx.waker()`). Once a task has been woken up,
/// it should attempt to `poll` the future again, which may or may not
/// produce a final value at that time.
Copy link
Member

Choose a reason for hiding this comment

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

I'd love to have it made explicit that it's not an error to poll a future even before a registered 'interest' has caused a task wakeup.

Choose a reason for hiding this comment

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

What would be the use case for polling a future multiple times like that?

(Intuitively, I would expect that assuming that it doesn't happen allows for some implementation simplification, e.g. one doesn't need to guard against multiple wakeups being scheduled for a single I/O event.)

Copy link
Member

Choose a reason for hiding this comment

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

The main one I can think of is implementing select and join, you don’t know which of your sub-futures caused the wakeup so you have to poll them all to see if any are ready yet.

Choose a reason for hiding this comment

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

I would think that such O(N) "poll all the inner futures" behaviour would be considered problematic. Hasn't there been some work to help a future figure out the cause of a wakeup?

Copy link
Member

Choose a reason for hiding this comment

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

IMO select and join should only be used for a very small number of futures, e.g. selecting a single future with a timeout to cancel it. If you have many sub-parts then these should be spawned off to separate tasks (and so be woken separately by the Executor) and a more efficient way to notify when all/one is complete should be used.

Copy link
Member

Choose a reason for hiding this comment

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

As noted, select and join are the ones that spring to mind. My main goal is to make it very clear whether or not it's permitted (and the recommended way of doing select if not - I can think of ways, but they're not great) as this could cause a nasty ecosystem split if some authors make the simplifying assumptions @HadrienG2 mentions and others require otherwise.

@aturon

This comment has been minimized.

@sbstp
Copy link

sbstp commented Apr 6, 2018

Depending on how likely SpawnErrors are to occur, it might be nice to have a provided alternative to spawn that simply panics, because a lot of users might end up simply calling .unwrap() on the result.

I guess that beyond the trait, a particular executor could provide this method if it does not have a limit on the number of tasks and such.

where E: BoxExecutor;

/// Get the `Waker` associated with the current task.
pub fn waker(&self) -> &Waker
Copy link
Contributor

Choose a reason for hiding this comment

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

missing ;


// this impl is in `std` only:
impl From<Box<dyn Future<Item = ()> + Send>> for Task {
pub fn from_box(task: ) -> Task;
Copy link
Contributor

Choose a reason for hiding this comment

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

missing argument type? or maybe an indication that the argument type isn't interesting (aka _) ?

@aturon
Copy link
Member Author

aturon commented Apr 6, 2018

@sbstp Note, such a convenience is provided by the Context::spawn method.


impl<'a> Context<'a> {
/// Note: this signature is future-proofed for `E: Executor` later.
pub fn new<E>(waker: &'a Waker, executor: &'a mut E) -> Context<'a>
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand how this is the case, if this is stabilised with argument that's going to change, how is that future proof? is it because Executor will be a supertype/parent of BoxExecutor, thus the type is only being opened up to things it couldn't have been before?

Copy link
Member Author

Choose a reason for hiding this comment

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

Along those lines, yes -- because there will be a blanket impl from BoxExecutor to Executor.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks, I was confused on my first read-thru, but realising that it's an argument that will effectively be a subtype of the eventual trait.

@aturon
Copy link
Member Author

aturon commented Apr 6, 2018

One note concerning vetting and stabilization, which is very important: the basic shape of the futures API changes (regarding the task system) have been vetted for a couple of months on 0.2 preview releases, and integrations. The pinning APIs have been quasi-formally checked. And the proposed stabilization plan here will give us an additional 6-7.5 months of experience with these APIs before they are shipped on stable.


/// Represents that a value is not ready yet.
///
/// When a function returns `Pending`, the function *must* also
Copy link

Choose a reason for hiding this comment

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

An alternative design that should be at least mentioned in the alternatives section is to make Poll::Pending wrap some "I have scheduled my task!" token, which a future can obtain either from Poll::Pending being returned from one of its children futures or by explicitly constructing such a token (using some method/API whose naming would make it painfully clear it's not okay to just call it without actually scheduling the task to be woken up by some external mechanism), yay learnability, yay typesystem. This was proposed (and, I assume, rejected) before, so it should make sense to link to that discussion from this RFC.

Copy link

@HadrienG2 HadrienG2 Apr 7, 2018

Choose a reason for hiding this comment

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

I agree, it is really nice when strong "you have been warned" wording in comments can be replaced with a little type hack which ensures that the API simply cannot be used incorrectly.

Copy link

Choose a reason for hiding this comment

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

Well, incorrect usage is still possible if you have multiple children futures and forget to poll some of them, but seems much less likely.

@sbstp
Copy link

sbstp commented Apr 6, 2018

@aturon Context is a lower-level API used inside of manual Future::poll implementations, whereas Executor is a higher level API used outside of Future::poll. So the Context::spawn method is not available everywhere Exectutor is, at least to my understanding.

Copy link

@HadrienG2 HadrienG2 left a comment

Choose a reason for hiding this comment

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

Although my comments may sound like a very confused "Hey, what's going on?", they are only about making sure that the motivations and design of whatever gets engraved in RFC stone is made crystal clear.

I'm actually pleasantly surprised by how far Rust's futures have progressed towards being something that one can reasonably describe on a ~1k line budget. Good job!

/// the associated task onto this queue.
fn wake(&Arc<self>);
}
```

Choose a reason for hiding this comment

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

You may want to clarify this part a bit more. At least two points are currently unclear for the Tokio-uninitiated:

  • The description "Executors do so by implementing this trait." suggests that the Wake trait is directly implemented by an Executor impl. However, it must actually be implemented by a proxy type spawned by the executor, otherwise "wake()" wouldn't be able to tell which task must be awoken.
  • Along the way, a word or two could probably be added on the rationale for this complex self type (Why is a shared reference to an Arc needed? Why is &self not enough?).

Copy link
Contributor

Choose a reason for hiding this comment

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

Along the way, a word or two could probably be added on the rationale for this complex self type (Why is a shared reference to an Arc needed? Why is &self not enough?).

This is something I was wondering.

This RFC describes what the proposed API is, but it doesn't do a very good job of explaining why the API is the way it is. Why do we need these Arcs? It describes an alternative with an unsafe interface that acts essentially like an Arc with shared ownership through cloning, and mentions the ArcObj alternative, but it doesn't really spell out why you need this Arc-like behavior in the first place.

I think it would help to describe a little bit more about exactly how this executor/waker interaction is supposed to work; possibly with an example of a very basic, simple executor which is a single-threaded event loop, and if that doesn't fully clarify why you need Arc then describe why it is that more powerful thread-pool based executors need this Arc-like behavior.

Copy link

@HadrienG2 HadrienG2 Apr 7, 2018

Choose a reason for hiding this comment

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

After reading the RFC a couple more times, I think I have finally reverse-engineered in my mind why one would want to use an Arc here.

  1. Context only takes a shared reference to a Waker, so all it can provide to the executing future is a shared reference.
  2. But the future may potentially need to be awoken from a different, asynchronous code path, in which case there is a need for an owned Waker value that can be moved around.
  3. There should thus be a way to clone the Waker and potentially send the clone to a different thread, ergo, we need an Arc.

Of those, only point 1 is somewhat controversial. We could have imagined an alternate design in which the Context owns a Waker, and one can get it by consuming the Context (after all, do you still need that context if you are going to suspend the future?).

But this means that a new Waker must be created every time a future is polled, even if it is unused, and atomic increments/decrements are not cheap so we don't want to do that. Alternatively, we might provide Context with a way to create a Waker, but that would ultimately get quite complicated.

So considering that there are use cases for futures that can be awoken from multiple places (anything that combines multiple futures into one: join(), select()...), an Arc was probably deemed to be the most appropriate implementation.

Definitely not obvious, and still does not explain why this implementation needs to leak in the interface all the way to the self type. I agree with you that this RFC would benefit from explaining a bit more the "why", rather than mostly the "what".

/// This function is unsafe to call because it's asserting the `UnsafeWake`
/// value is in a consistent state, i.e. hasn't been dropped
unsafe fn wake(self: *mut self);
}

Choose a reason for hiding this comment

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

Again, please clarify to the tokio-uninitiated why raw pointers are needed here and more usual self types would have been ineffective.

pub fn wake(&self);
}

impl Clone for Waker { .. }

Choose a reason for hiding this comment

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

If I understand the purpose of Waker, you will probably want to assert that it is Send somewhere for completeness (or just mention it in the documentation).

/// means that `spawn` is likely, but not guaranteed, to yield an error.
fn status(&self) -> Result<(), SpawnError> {
Ok(())
}

Choose a reason for hiding this comment

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

This API seems intrinsically racey, as its careful doc admits. What is its intended use? To avoid moving Tasks into an Executor without good reason?

pub fn is_shutdown(&self) -> bool;

// additional error variants added over time...
}
Copy link

@HadrienG2 HadrienG2 Apr 6, 2018

Choose a reason for hiding this comment

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

With the current definition of SpawnError, failing to spawn a Task because an Executor with a bounded queue is overloaded means that said Task is lost forever. Is that intended? If not, you may want to provide a way to retrieve the Task which failed to spawn.

```

As stated in the doc comment, the expectation is that all executors will wrap
their task execution within an `enter` to detect inadvertent nesting.

Choose a reason for hiding this comment

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

What can go wrong in this case? Why is it detected and reported as an error?

Copy link
Contributor

Choose a reason for hiding this comment

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

This API grew from our experience of users using a blocking executor inside a future that is itself being executed on some blocking executor. In most cases, the app would just deadlock, since the future.wait() would call park the thread until it was ready, when the only way it could know it was ready was epoll on the same thread.

If we could detect blocking operations of all kinds while in the context of an Enter guard, that'd be amazing!

Copy link

@HadrienG2 HadrienG2 Apr 7, 2018

Choose a reason for hiding this comment

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

Here is an attempt at minimally phrasing this rationale in the doc comments or the RFC.

"Doing so ensures that executors aren't accidentally invoked in a nested fashion. When that happens, the inner executor can block waiting for an event that can only be triggered by the outer executor, leading to a deadlock."

(I would also love for OSes to stop overusing blocking APIs so much. When even reading from or writing to a memory address is a potentially blocking operation, it gets kind of ridiculous...)


```rust
trait Executor {
fn spawn(&mut self, task: Future<Item = ()> + Send) -> Result<(), SpawnError>;

Choose a reason for hiding this comment

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

Since you are talking about taking dyn by value, shouldn't there be a "dyn" keyword here? (In general, use of this keyword is somewhat inconsistent in this RFC)

///
/// This method will panic if the default executor is unable to spawn.
/// To handle executor errors, use the `executor` method instead.
pub fn spawn(&mut self, f: impl Future<Item = ()> + 'static + Send);

Choose a reason for hiding this comment

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

Why is this signature inconsistent with that of executor::spawn()?

/// threads or event loops. If it is known ahead of time that a call to
/// `poll` may end up taking awhile, the work should be offloaded to a
/// thread pool (or something similar) to ensure that `poll` can return
/// quickly.
Copy link

@HadrienG2 HadrienG2 Apr 6, 2018

Choose a reason for hiding this comment

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

You may or may not want to clarify that a Future-aware thread pool is needed here. Most thread pool libraries currently available on crates.io either do not provide a way to synchronize with the end of the computation or do so by implicitly blocking, neither of which is appropriate here. Only some thread pools will be usable here.

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be possible to offload blocking work to any kind of thread pool, and "synchronizing" the result back to a future, using something like futures current oneshot channel:

You send the arguments of your computation, and a oneshot::Sender<ReturnType>, to the other thread. Once computation is completed, you'd call tx.send(ret). In your Future, you'd just await the result from the oneshot::Receiver.

Choose a reason for hiding this comment

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

You're right, this can be done with any kind of fire-and-forget API (and I think this is how current futures-aware thread pools are implemented). It may not be immediately obvious, however, and will not work with blocking APIs (scoped_threadpool, rayon...).

Again, I'm not 100% sure if clarifying this part is needed, just thought it would be another possible area for design documentation improvements.

[guide-level-explanation]: #guide-level-explanation

The `Future` trait represents an *asynchronous* computation that may eventually
produce a final value, but don't have to block the current thread to do so.
Copy link

@HadrienG2 HadrienG2 Apr 6, 2018

Choose a reason for hiding this comment

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

The single most common futures-rs beginner mistake is to assume that a task is spawned in the background as soon as a future is created. So right from the introduction, we may want to stress the fact that Rust's futures are executed lazily, and not eagerly as in most other futures implementations.

Here is a possible wording tweak that subtly stresses this point: "...an asynchronous and lazy computation...".

Also, there is a small don't -> doesn't typo in there.

Copy link

Choose a reason for hiding this comment

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

Would it make sense to add #[must_use] then?

Choose a reason for hiding this comment

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

I definitely think so. I cannot think of any valid use case for creating a future and dropping it afterwards without having done anything else with it, and it is a common mistake.


## Prelude

The `Future` and `FutureRes` traits are added to the prelude.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are they added to the prelude?

Copy link
Member

Choose a reason for hiding this comment

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

At least, could they be moved to std::futures::prelude (or whatever module they will be in)?

Copy link

Choose a reason for hiding this comment

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

I have reservations about this. Could this section be extended with some rationale?

Copy link
Contributor

@Centril Centril Apr 7, 2018

Choose a reason for hiding this comment

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

To provide some context for prelude-or-not evaluation (I don't have an opinion here yet), here's a snippet from std::prelude:

The prelude is the list of things that Rust automatically imports into every Rust program. It's kept as small as possible, and is focused on things, particularly traits, which are used in almost every single Rust program.

So we should decide whether Futures are "used in almost every single Rust program".

Copy link
Member

Choose a reason for hiding this comment

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

I would say that most Rust programs would not need Future or FutureRes. Not even all async programs will need it (if you use mio directly, you have no need for Future).

The worse aspect is that I anticipate including this in prelude to cause problems, as I described in this comment.

@carllerche
Copy link
Member

carllerche commented Apr 7, 2018

Given that futures 0.2 has already been released and has not had any significant usage yet, it seems very rushed to include this in core, especially in the prelude and especially given how many fns are on Future and FutureRes.

edit: Removed "I am not going to get involved in the details of this RFC" because that clearly wasn't true.

}
```

We need the executor trait to be usable as a trait object, which is why `Task`
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like the additional information about Task isn't actually in the no_std section. Could it be expanded upon, and why it exists instead of Box<Future>? (I realize the reason is because there is no Box in no_std.)

subtrait for that case, equipped with some additional adapters:

```rust
trait FutureRes<T, E>: Future<Item = Result<T, E>> {
Copy link
Member

Choose a reason for hiding this comment

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

Could this be spelled out? FutureResult?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should; We don't use these kinds of short forms elsewhere in libstd, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

FutureResult sounds like something that comes out of a Future (like LockResult).

BikeShed::new("FallibleFuture").build()

Copy link
Contributor

Choose a reason for hiding this comment

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

@durka agreed, but why not just reverse the order, ResultFuture?

subtrait for that case, equipped with some additional adapters:

```rust
trait FutureRes<T, E>: Future<Item = Result<T, E>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would trait aliases be a possible way of doing this, instead of a separate trait? Looks like it continues to see progress.

Choose a reason for hiding this comment

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

Doesn't this mean that every corresponding method would have to be added to the Future trait with a where Self: FutureRes bound? Or is there a cleaner way to do this with trait aliases?

@carllerche
Copy link
Member

carllerche commented Apr 7, 2018

To provide more context on my reservation for adding Future and FutureRes to prelude, in my personal usage of futures, I virtually have never encountered the need for futures w/o an error type (this isn't to say that these cases don't exist, just that they are exceedingly rare when working in the context of I/O).

By removing the associated Error type, it now becomes more difficult to work with future bounds that need to include generic errors. For one example, see here, but in my work, doing so is pretty common.

Now, one way to escape situations where generics get out of hand is to define convenience traits w/ a bunch of associated types that are more specific + a blanket impl for the more generic trait that got out of hand.

For example, in advanced cases, working w/ tower's Service trait can get complex. So, tower-h2 has a more specialized version with a blanket impl for the write T: tower::Service here.

I would anticipate that, if Future loses the default Error type, having to reach for such a pattern w/ Future will become more common. The fact that FutureRes is needed here is indication that having to use these tricks will be common. However, if Future and FutureRes are in prelude, this will be prevent being able to do such a trick.

Ideally as @seanmonstar mentioned, something like trait aliases would be used instead of FutureRes. However, either way, I ask that these traits not be added to prelude to enable being able to reach for more specialized Future traits.

As a simple example, lets say that one is working with futures that are always Result<_, MyError>. It would be pretty convenient to define:

trait MyFutRes<T>: Future<Item = Result<T, MyError>> {
    fn map<U>(self, f: impl FnOnce(T) -> U) -> impl MyFutRes<U>
        { .. }
}

The problem is that this convenience trait would not be usable (unless I'm missing something) if Future is in the prelude. This is because any type that implements MyFutRes also implements Future and using the map fn becomes ambiguous.

So, I ask that Future and FutureRes not be included in prelude and instead, perhaps, a std::futures::prelude or a std::async::prelude are created instead. This would make Future and FutureRes similar to io::Read and io::Write, which are not in the prelude either. Instead, they are in std::io::prelude.

@carllerche
Copy link
Member

@aturon

One note concerning vetting and stabilization, which is very important: the basic shape of the futures API changes (regarding the task system) have been vetted for a couple of months on 0.2 preview releases, and integrations.

Could you elaborate more on this? I haven't personally seen anything exercising the 0.2 changes at a significant level. I would be eager to be able to have some samples to dig into showing how the API changes shake out in practice.

where E: BoxExecutor;

/// Get the `Waker` associated with the current task.
pub fn waker(&self) -> &Waker;
Copy link

@HadrienG2 HadrienG2 Apr 7, 2018

Choose a reason for hiding this comment

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

Wakers are about waking up futures that fell asleep, normally from a different stack frame than the one in which the future was polled. So a &Waker reference that is tied to the stack frame on which Future::poll() is being called does not sound very useful, and in every case that I can think of, people will want to call context.waker().clone().

If that is the general use case, shouldn't waker() just return an owned Waker directly?

@est31
Copy link
Member

est31 commented Apr 7, 2018

I think everyone who wrote futures using code immediately saw major ergonomic issues. Improving this reasonably is a really good idea.

This RFC seems like a good idea in principle, given that:

a) the async programming pattern is important for a systems programming language
b) This RFC enables improved language syntax that means ergonomics improvements for this important programming pattern
c) it is a minimal change, meaning that it won't make all Rust programs, related to futures or not, suddenly start a tokio reactor or something

Doubling down on this "add X to libcore" path is not a good idea though. E.g. I don't think that the failure crate should be added, or bindgen or something. Those are nice crates and do really well as crates, so no need to add them to the language.

Also, I think that @carllerche 's points on the schedule should be taken seriously. We shouldn't stabilize something that hasn't been used widely and couldn't mature. Futures shouldn't become the macros of the 2018 edition, stabilized in a rush to get something out of the window but with flaws left and right.

@aturon
Copy link
Member Author

aturon commented Apr 21, 2018

RFC updates!

As per discussion above, I've made several updates:

  • I've renamed FutureRes to FutureResult for the time being. I recognize that we will probably want to find a shorter name here, but want to nail down the basic shape first.

  • I've updated the proposal to match the final desired version of the FutureResult trait, i.e. with both associated types and a super trait bound. As I detailed in my earlier comment, we have various techniques we can use to reach that design through incremental stabilization, depending on how the Chalk work progresses.

  • I've made some minor tweaks to the Executor setup based on my experience implementing 0.3 so far. This is mostly just a refactoring of how we handle forward-compatibility with unsized rvalues.

  • I've added some text recapping the mechanics around Pin (which had been discussed elsewhere), and referenced @Nemo157's excellent comments giving some concrete examples.

  • I've revamped the stabilization section to try to be more clear about the steps involved.

  • I've added significant new text talking about the rationale for changes compared to 0.2.

I believe that the proposal now reflects the current rough consensus on thread, modulo two things: some bikeshedding, and some remaining discomfort around Pin. I'll be posting a follow-up comment on the latter shortly.

@HadrienG2, I still intend on providing fuller rationale in the places you asked for it, but haven't prioritized this since most of them are about existing aspects of the 0.2 design.

Finally, I want to note that the async/await RFC is heading toward FCP. This is partly because there are relatively few open questions about the design, but more importantly because we are eager to begin the experimental phase with these features, to provide maximal time for feedback before any stabilization is proposed. These RFCs are interdependent, so I would like to push toward FCP here in the near future as well, with the very clear understanding that the proposed design is preliminary and we've taken care to provide ample time to gain real-world experience on crates.io before shipping.

@aturon
Copy link
Member Author

aturon commented Apr 21, 2018

@seanmonstar Here's a belated reply to your concerns around Pin.

Fundamentally, a Future is a type that will return a value at some later point. That's all the trait should be about. Being pinned is, frankly, unrelated to returning a future result.

The way I think about it is more analogously to the distinction between self, &self, and &mut self. All of these tell the caller what rights must be owned in order to perform an operation. Pin adds to this list by offering a variant of &mut that can be used to restrict the caller from moving the internal data. (However, if you implement Unpin for your structure, Pin just is &mut).

The only time I'd care about a pinned future is when all of these are true:

  • I want to use async fn (and not implement Future for a type).
  • I want to borrow bindings across await! calls.

If I'm not doing those, then I don't need to be pinned. Yet, the current proposal requires that if I ever want to implement Future, I must always think about pinning. (emphasis mine)

That last statement is incorrect. This has been said elsewhere on thread, but to reiterate: the Unpin auto trait is specifically designed for the case where you don't want to make use of these new borrowing abilities; in this case you can freely convert between Pin and &mut, which means that the futures API is effectively unchanged for you.

However, there is another case to consider, which is when you're writing a future that doesn't use internal borrows itself, but does contain arbitrary user futures. I talk about that case in detail in the updated RFC (and we've also talked about it on IRC). TLDR: there's a trivial safe option that gets you back to the &mut story but with a potential performance penalty, and various paths for building safe abstractions that avoid the cost.

It might be worth re-reading @withoutboats's blog series about pinning if any of this is unclear.

As mentioned in other comments, if all futures must be pinned, that makes other things much harder (even if they didn't actually need the pinning).

Again to clarify: it is not the case that all futures must be pinned.

Select doesn't have a proposed solution.

See @Nemo157's comment proposing the canonical solutions.

Things like poll_ready would be unusable if they needed to poll some internal future. poll_ready would need to be changed as well to take a Pin, which means that the Service is then immovable.

This basically boils down to the same story as above: if all the futures are yours, they can all implement Unpin (and usually will have that impl automatically). If the future comes from your user, then you can follow one of the outlined strategies for that case.

Consider the Iterator trait, and how it currently takes &mut self. There is the goal of eventually allowing generators to be used as iterators. How is that going to work?

The use of generators and the issues around pinning are separate. In particular, for both generators and async fn, we can provide two different modes of operation:

  • Unpinned, which doesn't allow borrowing across yields, but which does implement Unpin
  • Pinned, vice versa

For async fn, the default very clearly wants to be pinned. For generators, the default probably wants to be unpinned.

It's an open question what this will mean in terms of traits, but the situation with iterators is very substantially different from that of async fn, both in terms of what defaults are appropriate, and of what kinds of compositions are desired. I don't think the two design spaces have much insight to offer each other, sadly.

Some combination of sub traits and specialization.

One thing I wanted to mention: it seems quite plausible to offer an optimized form of "pin safety", where if T: Unpin you get T back, but if T: !Unpin you get PinBox<T> back. This would help further reduce any performance issues around the naive way of dealing with unknown futures.


I hope the above helps allay your concerns. But I also think that the best thing we can do here is write code! I've already made significant progress on the futures 0.3 branch and have a PR up for the combinators. For the latter, I made no attempt to build safe abstractions around pinning, but it's already very clear that there are just a couple of basic patterns we can likely encapsulate.

My hope is to publish a futures 0.3-alpha in the very near future -- most likely by the end of next week. As outlined in the RFC, that version would only work on nightly Rust, but should otherwise give an accurate impression of what these APIs are like to work with. Ideally, with cooperation from the Tokio and Hyper maintainers, we can land 0.3-alpha integration behind a flag (or through some other means) and begin to get real-world experiences with these questions ASAP.

I think we should, at the same time, move aggressively to land both this RFC and its companion; as I explained in the previous comment, the motivation is to get as much experimental code in place as we can, so that we can support stabilization discussions down the road with more data.

literally the `async` version of the usual `read`:

```rust
async fn read(&mut self, buf: &mut [u8]) -> io::Result<usize>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps a more specific example can be included, one that cannot be implemented today? An async function returning a future that borrows both self and buf already is possible, for example (beta just used for impl Future).

Copy link
Member

Choose a reason for hiding this comment

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

This specific function can't actually be used today (at least not without lots of unsafe code), since the future return type is bound to the lifetimes of the input types.

Copy link
Contributor

Choose a reason for hiding this comment

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