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

Futures 0.2 #1

Merged
merged 6 commits into from
Feb 7, 2018
Merged

Futures 0.2 #1

merged 6 commits into from
Feb 7, 2018

Conversation

cramertj
Copy link
Collaborator

@cramertj cramertj commented Jan 31, 2018

Release a breaking change to the futures crate.
The main goals of this breakage are:

  • Separation of futures into multiple, smaller crates, which are reexported
    in a futures facade crate. This will make it possible to gradually evolve
    futures libraries without major ecosystem breakage.
  • Removal of deprecated functionality.
  • Minor API cleanup (renamings, generalizations, etc.).

This breakage may (depending on time to accept and implement) overlap with library changes resulting from the release of the tokio crate (formerly tokio-core) and mio 0.7. The tokio changes will result in changes to many popular libraries in the Rust async ecosystem. By releasing futures 0.2 at or around the same time, we hope to minimize the number of breaking changes to the ecosystem.

Rendered

futures-02.md Outdated
Transitioning to use these new APIs will already require some amount of library
breakage or deprecation and replacement. Specifically, functions which took
an explicit handle to a `tokio` reactor core will no longer need to do so.
Functions which formerly accepted the old tokio-core` handles will not be
Copy link
Member

Choose a reason for hiding this comment

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

missing `

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

futures-02.md Outdated
- `futures-core` for the `Future`, `Stream`, `IntoFuture`, `IntoStream` traits
and their dependencies.
- `futures-io` which includes the updated `AsyncRead` and `AsyncWrite` traits.
- `futures-sink` for the `Sink` trait which is used for sending values
Copy link
Member

@sfackler sfackler Jan 31, 2018

Choose a reason for hiding this comment

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


covered below

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Stream has seen a lot of heavy usage, and I don't anticipate that its core interface (poll for a Poll<Option<Item>, Error>) will change significantly going forward. Sink, on the other hand, has a number of potential inbound changes, including the possibility of changing Sinks to allow them to take inputs of multiple different types (this is necessary for variance in things like references).

futures-02.md Outdated

pub trait AsyncRead {
type Error;
unsafe fn initializer(&self) -> Initializer { ... }
Copy link
Member

Choose a reason for hiding this comment

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

We should probably try to figure out what the future of this is on Read - I'm not sure if the initializer-style API is going to be the one that lands.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it would be good to settle that design so that it can be duplicated here.

futures-02.md Outdated
fn poll_write(&mut self, buf: &[u8], ctx: &mut Context)
-> Poll<usize, Self::Error>;

fn poll_vectored_write(&mut self, vec: &[&IoVec], ctx: &mut Context)
Copy link
Member

Choose a reason for hiding this comment

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

The default implementation just passes the first buffer to poll_write?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that was my plan.


`futures-channel` includes the `mpsc` and `oneshot` messaging primitives.
This crate is a minimal addition on `futures-core` designed for low churn
so that channels may appear in the public APIs of libraries.
Copy link
Member

Choose a reason for hiding this comment

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

But it will depend on futures-sink which is going to be less stable, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had actually imagined this as being inverted-- futures-sink would depend on futures-channel and provide the implementation there, since futures-sink is less stable than futures-channel.

Copy link
Member

Choose a reason for hiding this comment

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

Would Sender than have a poll_send inherent method then?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that was my plan, but I'm open to suggestions :). If you think that seems reasonable, I'll add a bit about this to the RFC to explain.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that seems reasonable, but I think it'd be good to clarify :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've added a clarification to the RFC.

futures-02.md Outdated
- `AsyncRead` and `AsyncWrite` now offer separate methods for
non-vectorized and vectorized IO operations. This makes the API easier to
understand, makes the traits object-safe, and removes the dependency on
the `bytes` crate.
Copy link
Member

Choose a reason for hiding this comment

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

But adds one on io-vec, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The dependency on IoVec exists either way since it's exposed through the interface of bytes.

@CAD97
Copy link

CAD97 commented Jan 31, 2018

Rendered

futures-02.md Outdated

## Crate Separation

The `futures` crate will be split into several different libraries.
Copy link

Choose a reason for hiding this comment

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

Could you explicitly say which of these crates are expected to be usable with #[no_std], and which ones will require the standard library?

Copy link
Collaborator Author

@cramertj cramertj Jan 31, 2018

Choose a reason for hiding this comment

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

All of them are compatible with no_std except for ``futures-channels`. I've added a comment addressing this.

@djc
Copy link

djc commented Jan 31, 2018

Context is a very generic name for something that will be broadly used in many different places. It would be nice if there was another shortish name which more obviously relates to futures and their associated machinery.

@Zoxc
Copy link

Zoxc commented Jan 31, 2018

If we are having breaking changes to the Future trait, why not make it return a single value instead of separate success and error types? This simplifies the design further which is helpful for learnability and also aligns with the functionality of generators, so async/await does not need to to any translation.

Given that generators current do not accept arguments, how will you pass the reference to Context into them for async/await?

I'd also like to see a way to use immovable generators without memory allocation, but it might be a bit too early for that.

@Marwes
Copy link

Marwes commented Jan 31, 2018

@Zoxc

If we are having breaking changes to the Future trait, why not make it return a single value instead of separate success and error types? This simplifies the design further which is helpful for learnability and also aligns with the functionality of generators, so async/await does not need to to any translation.

I am not able to find the issue on the futures-rs repo atm but end gist of it is that combinators such as and_then are impossible to implement if there is not an explicit error type. I agree that in that I would like the error type gone but not at the cost of losing combinators such as and_then

@fredrikroos
Copy link

How would async/await look under this proposal? It seems contrary to the vision outlined in the Tokio reform RFC where there is a close correspondence between sync and async code.

@Zoxc
Copy link

Zoxc commented Jan 31, 2018

@Marwes You can still implement those combinators, they would just require the future to return Result by having a Self: Future<Return = Result<R, E>> bound.

@Marwes
Copy link

Marwes commented Jan 31, 2018

@Zoxc

You can still implement those combinators, they would just require the future to return Result by having a Self: Future<Return = Result<R, E>> bound.

Hmm, I know I tried that approach sometime ago but didn't manage to get it working in all cases. From what I can remember it works to implement the methods but when you try to use (some) of them they won't be usable for one reason or another. Is there a POC of this approach somewhere?

@Zoxc
Copy link

Zoxc commented Jan 31, 2018

@Marwes I wrote this thing. I haven't done any more extensive research.

@bill-myers
Copy link

bill-myers commented Jan 31, 2018

Another thing that could be considered is removing all the executor infrastructure and instead just using FuturesUnordered (or some variant of it) to spawn futures.

If it can be done without any significant performance cost, this would significantly simplify the future backend crates.

@skade
Copy link

skade commented Jan 31, 2018

@djc in these cases, I usually recommend to use task::Context for clarification. This is a frequent problem and the Rust module system helps there.

@skade
Copy link

skade commented Jan 31, 2018

I like the general direction, but I have multiple issues with this RFC.

I especially like the split into multiple crates, keeping the futures interfaces in sync is a chore.

First of all, the next iteration of futures should be a fast-track to 1.0. Currently, this RFC mostly amounts to changes that have been kind of on the horizon for the next version anyways. (like finally fulfilling all the documented changes and deprecations). futures always being in "could-change" mode is terrible for the ecosystem.

Then, my long recurring favourite: the RFC alludes to "learnability", especially around tasks. Futures has never been properly document for people to learn the current model. The guide "how to implement a future on your own" isn't written. In my experience, the current model is easy to understand from the outside, once people grok that there is some wakeup mechanism. We have no proper research on this and we can't have any proper research on this, as learnability needs to be analysed for the common case of flipping up the docs and then working from there. There's certainly reasons to use an explicit task model (the case where local storage is hard), but I can't follow arguments on learnability have no proper footing. Not everything that is implicit is hard to learn.

Along those lines, the RFC is completely lacking a plan for documentation, which is is the biggest pain point with futures, currently.

@Nemo157
Copy link
Member

Nemo157 commented Jan 31, 2018

@fredrikroos

How would async/await look under this proposal? It seems contrary to the vision outlined in the Tokio reform RFC where there is a close correspondence between sync and async code.

I believe it would be possible to have async/await implicitly plumb the context through if generators are changed to allow resuming with an argument. We would probably also want some way to explicitly access the context when using async/await as well, I'm not sure if that would be ergonomically possible.

@Nemo157
Copy link
Member

Nemo157 commented Jan 31, 2018

I don't see any mention of changing what polling an Error on a Stream means. Especially since Stream is a part of the futures-core crate under this proposal, making sure that such a fundamental decision is done before 0.2 seems important for forwards compatibility.

EDIT: Also, since this would be a usage/documentation change only, even with a breaking release in the future changing it would be difficult as there would be no compiler errors to inform users of the breakage.

futures-02.md Outdated
changes, but that updating will not normally result in significant or
widespread breakage.

This change comes alongside he release of the new `tokio` crate.
Copy link

Choose a reason for hiding this comment

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

s/he/the/ typo here.

Copy link
Collaborator Author

@cramertj cramertj Jan 31, 2018

Choose a reason for hiding this comment

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

Fixed. (thanks :) )

@fredrikroos
Copy link

fredrikroos commented Jan 31, 2018

@Nemo157

Right, after looking through the futures-await macros I see how you could hide the context argument with generator argument support. I still think this RFC should be more explicit about how it interacts with async/await.

futures-02.md Outdated
schedule a wakeup. Furthermore, the wakeup handle never shows up in async/await
or combinator code.

This RFC proposes to resolve these issues by making the `Task` wakeup handle

Choose a reason for hiding this comment

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

We've had this debate plenty of times, and I suspect it's already decided, but just because:

I think the "learnability cost" is more of an argument coming from people who have not spent much time writing impl Future for T. Those who haven't, when they look at an implementation of Future, and missing context, don't like that it's not obvious how the future will be woken up.

Once someone actually does ingest how the TLS task system works, many have noted it's rather elegant, instead.

Crucially, there is a difference between Futures implementors and users. Users don't impl Future, but instead will be able to just use combinators and await. Implementors must write a lot of impl Future/impl Stream/etc, and having to pass around an additional argument to child futures is just an annoyance, rather than a help.

Copy link

Choose a reason for hiding this comment

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

I'm not sure I would say "users don't impl Future", I generally recommend to implement your own futures for semantic actions instead of handing around very non-descriptive types with many combinators inside.

Copy link

Choose a reason for hiding this comment

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

Context in TLS helps implementors. If one has to weigh towards users or implementors, we should be picking the users.

Also TLS is more expensive than passing an arg.

Choose a reason for hiding this comment

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

The #![no_std] argument is compelling for me- even though TLS is less work once you understand the system, and even assuming the performance is fine (I expect it is), it would be useful for futures to work in that context.

Choose a reason for hiding this comment

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

If one has to weigh towards users or implementors, we should be picking the users.

Somewhat, yes. Weight more towards ease-of-use for users. However, if things are too cumbersome for implementors, you'll have libraries that just skip providing Future support. As mentioned elsewhere in this thread, having to update h2 (or hyper) will be major sadfaces.

Compared to the possibility that documentation could clean up the confusion for less-informed users who just want to implement a few tiny Futures, I'm not sure the weights balance.

futures-02.md Outdated
}

pub trait AsyncRead {
type Error;

Choose a reason for hiding this comment

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

There was a reason that std::io::{Read, Write} did not include an associated error type. Does that reason not apply here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know what that reason is. Do you? I'd imagine it could be to make it so that users don't have to specify the error type in trait objects, but idk.

Copy link
Member

Choose a reason for hiding this comment

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

It turned out to be really painful to pipe the error bounds to all of the relevant places I believe. @alexcrichton tried it out.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I believe that. Still, i think it's worth it in this case, since we're already doing the same thing for Future and Stream-- doing it in AsyncRead and AsyncWrite just seems to be making our story there consistent.

futures-02.md Outdated
- `Task` will be renamed to `Waker`. This name more clearly reflects how
the type is used. It is a handle used to awaken tasks-- it is not a `Task`
itself.
- `Async` variant `NotReady` will be renamed to `Pending`.

Choose a reason for hiding this comment

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

With the new Try trait, would it make sense to explore instead of returning Result<Async<T>, E>, making Async an enum of 3 variants?

Copy link

Choose a reason for hiding this comment

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

How would an enum with 3 variants work with Try? Can you elaborate?

However, `AsyncRead` and `AsyncWrite` don't have any dependencies on the `tokio`
crate or the rest of the `tokio` stack, so they will be moved into the `futures`
group of libraries. `futures-io` depends only on the `futures-core` crate
and the `bytes` crate (for the [`Buf`] and [`BufMut`] traits).

Choose a reason for hiding this comment

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

Further down, it says the bytes dependency is removed. Is this out-of-date?

If so, why the removal of Buf/BufMut? They are extemely useful abstractions, and greatly ease using vector IO.

Copy link
Collaborator Author

@cramertj cramertj Jan 31, 2018

Choose a reason for hiding this comment

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

Yes, this was out-of-date-- bytes is no longer depended upon. You're still more than welcome to use Buf/BufMut! I agree that they're useful abstractions. However, baking them into the futures-io traits means that there is a larger body of code (the bytes crate) which has to stabilize before futures-io can reach 1.0. Additionally, making the futures-io trait methods generic on Buf/ BufMut prevents them from being object-safe.

futures-02.md Outdated
```rust
pub trait Future {
type Item;
type Error;

Choose a reason for hiding this comment

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

It was mentioned in a top level comment, but I prefer keeping discussion scoped to lines, so it's easier to follow specific discussions: What about if Future simply had an Item, and no Error? Combinators like and_then could exist for F: Future<Item=<Result<T, E>>.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good question! I've copy-pasted my response from below:

I can definitely see the advantages of such an approach, and I think the combinators could be adjusted appropriately. I could envision changing e.g. map and map_err into map, map_ok, and map_err or something similar. However, allowing non-Result futures also complicates many parts of the API and combinators-- now every combinator author has to standardize on whether to use Result-typed futures or "plain" futures. I anticipate that, in practice, most async operations involve some potentially fallible operation, so you'd have to manually specify Fut: IntoFuture<Item=Result<T, E>>, E: Into all over the place, which adds a lot of noise and extra type parameters to every function (T and E are now parameters instead of associated type projections). Infallible Futures are rare, and most cases can be represented reasonably via Future<Item = T, Error = !>.

Choose a reason for hiding this comment

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

Could this be resolved with trait aliases?

type Future<T, E> = CoreFuture<Item = Result<T, E>>;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@rpjohnst Combinator methods such as and_then would need to be generic on T and E and have where Self: Future<Result<Item = T, Error = E>>> (or where Self: Future<T, E> with trait aliases). To me, the "result-y" future seems like it should be the default, and CoreFuture is an alias for Future<Item = T, Error = !>. Yes, it's slightly less ergonomic, but I think it's better to take the ergonomic hit on the non-result-y case than on the result-y case.

Copy link

Choose a reason for hiding this comment

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

That's why I named the alias Future rather than ResultFuture- no ergonomic hit on the result-y case, without putting Error in the trait.

Perhaps both the trait and the alias could be named Future, but in different modules, much like io::Result et al. Presumably there'd be no ergonomic hit in either case that way?

Copy link

@Marwes Marwes Feb 1, 2018

Choose a reason for hiding this comment

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

To make things less abstract

With type Error

fn and_then<F, B>(self, f: F) -> AndThen<Self, B, F>
        where F: FnOnce(Self::Item) -> B,
              B: IntoFuture<Error = Self::Error>,
              Self: Sized,

Without type Error

fn and_then<T, E, U, F, B>(self, f: F) -> AndThen<Self, B, F>
        where F: FnOnce(T) -> B,
              B: IntoFuture<Item = Result<U, E>>,
              Self: Sized + Future<Item = Result<T, E>>,

Without type Error but with "CoreFuture" and "CoreIntoFuture" (presumably)

fn and_then<T, E, U, F, B>(self, f: F) -> AndThen<Self, B, F>
        where F: FnOnce(T) -> B,
              B: IntoFuture<U, E>,
              Self: Sized + Future<T, E>,

Copy link

Choose a reason for hiding this comment

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

Without type Error looks fine. It is not like users implement combinators every day.

Having Error be part of future is akin to requiring all functions to return Result.

Copy link

@skade skade Feb 1, 2018

Choose a reason for hiding this comment

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

Having Error be part of future is akin to requiring all functions to return Result.

Is that a bad thing? Once you combine the first future with resulty behaviour, your whole chain is going to be resulty anyways. As previously stated, most futures implement fallible operations.

Futures are currently very easily explained as "future results".

Making and_then generic over 5 parameters looks incredibly noisy, IMHO.

Also, does anyone have a rough gauge how that interacts with inference/compile times?

-Implementation of Sink will go in futures-sink
-Will be the only one that doesn't support std
@carllerche
Copy link

Learnability

As mentioned by others, the learnability argument is not strong. There has not been a strong effort to improve the documentation / learning path to futures, which is the main learning blocker. Trying to improve learnability before fixing documentation is premature.

I think @seanmonstar broke down the issue well with the "user" vs. "implementor" groups. Adding an explicit context argument doesn't actually change anything from the "user" side of things, only the implementation side of things.

Ergonomics

Making the task argument explicit will add significant noise on the implementation side of things. For example, the vast majority of the h2 library is implemented without combinators. Many functions will now require an additional argument without actually providing much direct benefit to the library.

no_std

It would be worth explaining how no_std benefits exactly and how futures would be used without an allocator. The notify handle requires 'static + Send and providing that without allocation is not immediately obvious. I assume there would be some strategy with preallocating static memory, but I am not sure?

Waker

This adds yet another naming metaphor (Unpark / Notify) and there exists in the tokio-reform branch a Wakeup trait.

Adding Waker without doing a full evaluation of the existing naming and coming up with a uniform strategy will add additional confusion.

Unresolved questions

As others have mentioned, there are still quite a number of other unresolved questions that have been pending a futures 0.2 release. Given the impact of releasing breaking futures releases, it would be worth addressing these pending issues or say they are "won't fix".

The issues mentioned above are:

  • Removing the Error associated type from Future / Stream.
  • Determining the behavior of Stream (is an error final? this also depends on above).

@cramertj
Copy link
Collaborator Author

@djc

Context is a very generic name for something that will be broadly used in many different places.

The Context type is intentionally placed in the task module so that it can be referred to as task::Context. Task is a poor name on its own, as you're in a task, not being given one 😄 . TaskContext is too long and is much the same as task::Context.

It would be nice if there was another shortish name which more obviously relates to futures and their associated machinery.

I totally agree! I'm more than wiling to change the RFC if someone comes up with a better name.

@Zoxc

If we are having breaking changes to the Future trait, why not make it return a single value instead of separate success and error types? This simplifies the design further which is helpful for learnability and also aligns with the functionality of generators, so async/await does not need to to any translation.

I can definitely see the advantages of such an approach, and I think the combinators could be adjusted appropriately. I could envision changing e.g. map and map_err into map, map_ok, and map_err or something similar. However, allowing non-Result futures also complicates many parts of the API and combinators-- now every combinator author has to standardize on whether to use Result-typed futures or "plain" futures. I anticipate that, in practice, most async operations involve some potentially fallible operation, so you'd have to manually specify Fut: IntoFuture<Item=Result<T, E>>, E: Into<Error> all over the place, which adds a lot of noise and extra type parameters to every function (T and E are now parameters instead of associated type projections). Infallible Futures are rare, and most cases can be represented reasonably via Future<Item = T, Error = !>.

Given that generators current do not accept arguments, how will you pass the reference to Context into them for async/await?

I'd planned to still include them in TLS via a shim in the futures-await crate.

I'd also like to see a way to use immovable generators without memory allocation, but it might be a bit too early for that.

I would absolutely love this and I'm super excited about making this work ASAP. @withoutboats has a proposal which would allow this to work with minimal changes to rustc or the existing plans for futures. They'll be explaining this plan in upcoming blogposts. (Also, I want to thank you for all of your work on generators-- this wouldn't be at all feasible without the immense amount of work you've done.)

@fredrikroos

How would async/await look under this proposal? It seems contrary to the vision outlined in the Tokio reform RFC where there is a close correspondence between sync and async code.

This proposal was designed specifically to complement the Tokio reform RFC in many ways. One crucial thing to notice is that the changes to the poll method don't affect the use of combinators-- the task::Context plumbing is done internally.

@bill-myers

Another thing that could be considered is removing all the executor infrastructure and instead just using FuturesUnordered (or some variant of it) to spawn futures.

The executor infrastructure is being moved to a different crate where it can be iterated on independently. I will say that, personally, I often reach for FuturesUnordered when I want this sort of behavior, and I would like to focus on FuturesUnordered more in the coming changes to documentation.

@skade

First of all, the next iteration of futures should be a fast-track to 1.0

Yes! It's super important that we be able to ship things. The goal with the 0.2 release is to split things up and ensure that we have things structured in a way that will allow for minimal breaking changes to the core infrastructure, while allowing us to remain flexible with the less-stable aspects. I'd expect to see the various crates reach 1.0 gradually as a result of this change-- first futures-core, then futures-channel, etc...

Futures has never been properly document for people to learn the current model. The guide "how to implement a future on your own" isn't written.

zomg I want this so bad. @aturon has been hard at work on a new book that is essentially "futures from the ground up" which will focus on this experience.

There's certainly reasons to use an explicit task model (the case where local storage is hard), but I can't follow arguments on learnability have no proper footing. Not everything that is implicit is hard to learn.

I totally agree! Implicit != hard to learn. However, this particular instance of implicitness has empirically shown learnability problems for many people (e.g. almost everyone I've ever explained the Future trait to).

Along those lines, the RFC is completely lacking a plan for documentation, which is is the biggest pain point with futures, currently.

Documentation is a huge priority for this release. 0.2 gives us a solid point to make breaking changes so that we can stop having documentation that has gradually bit-rotten over time.

@skade
Copy link

skade commented Jan 31, 2018

I totally agree! Implicit != hard to learn. However, this particular instance of implicitness has empirically shown learnability problems for many people (e.g. almost everyone I've ever explained the Future trait to).

I haven't seen this empirical evidence and have no issue teaching futures (through 2 lectures and a couple of trainings). I'm sorry to put it bluntly, an individual that does training and runs into issues when teaching something is still anecdata.

Yes! It's super important that we be able to ship things.

In this case, this should be in the RFC.

Documentation is a huge priority for this release.

Same here.

@yazaddaruvala
Copy link

yazaddaruvala commented Feb 1, 2018

@cramertj I'll try to explain with code

If you're creating a library, this is less applicable, but when writing web services:

// Its not clear if we need to spawn these futures.
let from_table_one: Future<...> = self.database.call(...);
let from_table_two: Future<...> = self.database.call(...);

// Has the database client has already abstracted away the need to drive these futures?
// or is it the users responsibility? Each client might make a different decision for style/ergonomics.
pool.spawn(from_table_one);
pool.spawn(from_table_two);

await!(from_table_one);
await!(from_table_two);
// Very clear that we need to spawn these futures.
let from_table_one: LazyFuture<...> = self.database.call(...);
let from_table_two: LazyFuture<...> = self.database.call(...);

pool.spawn(from_table_one);
pool.spawn(from_table_two);

 // Ideally, we would get a compile time error if we await an un-spawned future
await!(from_table_one);
await!(from_table_two);
// Very clear that we need to spawn these futures.
let from_table_one: LazyFuture<...> = self.database.call(...);
let from_table_two: LazyFuture<...> = self.database.call(...);

let from_table_one: EagerFuture<...> = pool.spawn(from_table_one);
let from_table_two: EagerFuture<...> = pool.spawn(from_table_two);

// Compile time checks to ensure this will not cause the thread to wait indefinitely
await!(from_table_one); // i.e. await can only take an `EagerFuture`
await!(from_table_two);

@skade
Copy link

skade commented Feb 1, 2018

Many people have already posted about their struggles learning futures, and many of us have firsthand experience struggling to understand futures or helping others to understand it. I'd love more input from folks, but it would be easiest for me to personally digest if they left those thoughts in a comment on this RFC (request for comment 😄).

Okay, so, here's my short list of struggles that I meet during trainings :). I gave 4 trainings on Futures last year, as well as 2 university lectures with exercises.

  1. Aligning combinators right is a struggle (adjusting the error, etc.).
  2. IntoFuture is a great interface, but terribly hard to discover.
  3. Programming with futures that can return new futures is something you have to get used to in general.
  4. Gluing the primitives together (e.g. that you can see a stream as a future that gives you the head and the rest of the stream) is easy once you know about, but hard to discover in a soup of API.
  5. At some point, they will call wait in tokio and be confused
  6. No one finds concat without help or having read hyper examples.
  7. Knowing that you can also just fully dispatch a new future tree on e.g. a cpupool and communicate over channels is necessary.
  8. Know that you can send channel handles over channels tends to be a revealing thing
  9. Traceability is not there, even simple solutions such as https://github.com/skade/futures-poll-log help a lot
  10. Understanding that futures build a data structure that executors know how to evaluate is usually not a given.
  11. What does F: Future + 'static mean and why is that useful and not cheating?
  12. People struggle a lot that Futures talk about the... well... future. And that the future expands from now to infinite. That infinite futures are a usable thing.
  13. select, join, select_all... How is select useful for properly writing a long-running server, for example?

I never once - and this confuses me - hear someone being confused about poll not taking a task, after they have been told that the task notification machinery is a thread local. I don't want to say it doesn't happen.

It should be mentioned that I usually train Java/Ruby/JavaScript people that are very used to interfaces that have additional background knowledge. But the C and C++ programmers had similar issues.

In general, futures has many great examples around, if you know where to find them - they are somewhere in the docs or test suites of projects using them.

@gnzlbg
Copy link

gnzlbg commented Feb 1, 2018

@carllerche

It would be worth explaining how no_std benefits exactly and how futures would be used without an allocator.

no_std does not mean that futures will be used without an allocator in all no_std projects (even though that might still be possible). no_std projects can have global allocators, or multiple local ones (static pools of memory, buffers on the stack), all of which can implement the Alloc trait if that were necessary. @japaric probably has more thoughts on this. What a no_std project does not necessarily have is the full std library, but it can have parts of it (libcore, liballoc, etc.).

@djc
Copy link

djc commented Feb 1, 2018

While I initially ingested the proposed RFC without much criticism on this point, I have to say that I find myself agreeing more with the no-Context argument side of the discussion than with the RFC proposal. Besides the input from @skade, which I think should weigh somewhat heavily as broader/more diverse input on learnability, I have the following personal experience:

  1. As an implementer of several Future impls, I had no trouble dealing with the lack of connection to a Task or a Context. In a sense, it is just an abstraction, but at least in my implementations the abstraction wasn't leaky at all -- I could do everything I needed to do without access to TLS.
  2. It seems the main disadvantage for learnability (that is, leakiness of the abstraction) would not be on most authors of Future impls, but only on a pretty select set that implement fairly low-level Future impls (at the level of mio itself, for example). In all of the cases that I tried (dealing with network protocols, near the tokio-io/tokio-proto level of abstraction) it was just composing higher-level futures out of lower-level ones, meaning passing around Context would just be boilerplate.

Instead, I would say the main challenge was how the combinators interacted with lifetimes and function boundaries (mostly in writing types -- where impl Trait will help).

I can't speak to the performance and no_std arguments, though, which by themselves might provide more weight in favor of making Context explicit.

@cramertj
Copy link
Collaborator Author

cramertj commented Feb 1, 2018

The conversation around task::Context is currently eclipsing what I feel are many more valuable and important changes in this RFC. In order to make progress on the more essential elements of this RFC, I've separated out the task::Context argument into its own RFC. Apologies for any confusion this may cause.

@aturon
Copy link
Contributor

aturon commented Feb 1, 2018

Thanks @cramertj for this inaugural RFC! I'm excited to get the crate moving strongly this year.

I want to give some thoughts and context on some of the bigger themes that have come up here.

Overall roadmap and the crate split

The futures team would like to iterate toward and publish a 1.0 in 2018. We aren't going to be able to do that in one big step, in part because we are anticipating further changes to work well with async/await (see @withoutboats's blog series, which will ultimately talk about this).

So part of the motivation for this RFC is to kick off that iteration, and also make space for further iterations by breaking up the crates -- in particular, splitting up the combinators and the different core traits to allow for more independent revisions.

However, this is not the intended final design. In the end, I think we want a smaller set of crates like:

  • futures: includes Future, Stream, Sink, AsyncRead, AsyncWrite and the combinators.
  • futures-channel: as in this RFC
  • futures-executor: as in this RFC

I think it'd be good to get consensus on at least a rough final design like the above. For one thing, this design suggests that the futures facade should not include futures-channel and futures-executor, because it will eventually not be a facade.

@cramertj @withoutboats @alexcrichton I'm curious to get your thoughts on this.

Documentation

Documentation is hugely important, and is something I've started working on, but has been on hold while we work out the designs here in this RFC. I don't personally think the RFC needs to talk about docs; as @cramertj says it's a top priority and work is under way. I think it should develop in parallel with the work described here, and when we get further along we can assess the timing of the 0.2 release relative to the docs.

The error type

A frequent question about the futures design is the fact that futures bake in errors, rather than composing via Result.

The core reason for this is that error handling affects the behavior of many of the combinators. For example, the join combinator has specific handling for getting an error from one of the joined futures -- it bubbles the error out, cancelling the other future. It would not be possible to provide a join combinator that worked on arbitrary types and then somehow get this behavior via composition. And several other combinators work similarly.

As already mentioned on thread, there are also several combinators that fundamentally require Result, so would have to be implemented only on futures that happen to use Result.

If you put those two together, there's actually very little in the futures API that makes sense without errors. And in the async I/O world, errors are endemic anyway, so the current setup is a bit more streamlined.

All that said, part of the hope with the 0.2 release is to make Future<Item = T, Error =!> more of a thing in the API; this could eventually be aliased to a separate PureFuture trait. That should give at least some of the benefit of removing the built-in errors.

The one piece we don't get: when you do happen to use a future that doesn't produce an error, you often get type inference failures. We're hoping to comb through the combinators and look carefully at where we use conversion traits to see if we can improve this situation.

Assorted smaller issues

The RFC gives an explicit list of smaller open issues that we plan to tackle in 0.2. Rather than trying to pack discussion of all of those into a single RFC thread, it seems best to continue those discussions in each of the issues. The goal of this RFC is to get consensus on the broad structure of a 0.2 release.

Conclusion/state of play

With @cramertj's recent change to pull out Context into a separate RFC, this RFC is now fundamentally about iterating the futures crate by making some of the known-needed breaking changes, and setting up the crates for further iteration. Does anyone have concerns about that overarching story?

@carllerche
Copy link

Without the explicit task argument passed to the traits, it hardly seems worth a breaking change at this point.

You can still release futures 0.2 as a facade. Move all the traits as they are into a futures-core crate. Deprecate / hide all the trait fns that will move off of the Future crate and update futures 0.1 to re-export the types of futures-core 0.2.

Because, otherwise, it just seems like a major ecosystem break to rename Async::NotReady -> Async::Pending, which hardly seems worth it.

@aturon
Copy link
Contributor

aturon commented Feb 1, 2018

@carllerche There are a number of other breaking changes queued up, and as I explained in my comment, and as the RFC makes clear, part of the motivation here is to make room for further iteration in this space as we push toward 1.0.

In addition, with the impending release of tokio 0.1, there's going to be a bump in the ecosystem anyway, so the futures team believes this is a good opportunity to do this work.

@seanmonstar
Copy link

With the removal of the Context argument, I agree that it seems like this can be done with much less total mayhem breakage.

futures-core

  • Put Future and Stream in futures-core, deprecating all of the combinator methods, pointing people to a FutureExt/StreamExt trait in futures-util.

    Hm, will method resolution be angry at having both Future and FutureExt in scope, and trying to call fut.and_then?

  • Yes, Async::Pending is ever-slightly nicer than NotReady, but that is a LOT of code that needs a find/replace for aesthetics. Is it really needed?

  • I do think Stream::poll -> Stream::poll_next is also a nice change, makes it feel more like an asynchronous Iterator.

    But, is it feasible to try to deprecate a required trait method in favor of a new one? A default poll_next() { self.poll() } could exist, but then people need to implement both poll and poll_next, and could make them different...

    Probably not an actual problem anyone updating their code can just move the function to poll_next, and change poll() { self.poll_next() }.

  • As this RFC suggests, punt Sink out of the core, it's not stable yet (it is definitely much more awkward to use than Future and Stream).

futures-io

I'd actually propose to drop this idea, at least for any sort of futures 1.0. It's a much more volatile landscape, and there's a lot of unresolved questions, or changes that would be needed with more experience. Here are some:

  • AsyncRead and AsyncWrite probably should not have an associated type, but use std::io::Error, since it happens to be IO. But the probably here makes these traits pretty unstable.
  • The poll_read_vectored and poll_write_vectored are much less convenient than the existing read_buf and write_buf that utilize Buf and BufMut. If those traits were moved to a smaller crate than bytes, and thus could be stabilized more quickly, I think that'd help keep their usage in AsyncRead and AsyncWrite.
  • There is still a dependency on iovec, which while it is small and likely to not change much, it seems like the weight of it would be comparable to a smaller buf-like crate.

Why not keep tokio-io for now, but limit it down to just the traits, and move the framed and codecs and length_delimited and stuff into a tokio-codec crate instead?


I have some thoughts about the Executor trait specifically, but that's probably not as on-topic here, and could probably just be a new issue in the futures repo.

@aturon
Copy link
Contributor

aturon commented Feb 1, 2018

@seanmonstar

Again, as I mentioned to @carllerche, there are numerous breaking changes which have been queued for some time, and which are referenced by this RFC. And per my overview post, a key goal here is to allow for iteration on the road to a 1.0 this year. Futures has not had a breaking release in 1.5 years, and a lot of cruft has piled up. If we're going to push to stability this year, we need the freedom to do this iteration.

Can you elaborate on what you mean by "total mayhem"? Again, part of the goal is to coordinate with the breakage that the forthcoming tokio release will bring in the async ecosystem. I also believe that, with the facade strategy, most changes should be mechanical.

re: futures-io, I think that the vectored operations should be dropped for the time being, and re-added as default methods at a later point (when things are more clear). I agree that we should use std::io::Error. I think otherwise the traits are good to go.

@seanmonstar
Copy link

there are numerous breaking changes which have been queued for some time

a lot of cruft has piled up

Can you elaborate on what you mean by "total mayhem"?

Yep, I'm not saying zero breaking changes. I just mean that the change from Async::NotReady to Async::Pending affects a large amount of code; the other breaking changes do not. It seems that rename would be the only change that would currently break everyone (besides deprecations, but that's fine).

I think that the vectored operations should be dropped for the time being, and re-added as default methods at a later point (when things are more clear).

That would be very unfortunate, hyper already makes use of them to improve throughput of long chunked streams. I've already gone through the cycle of (flatten to a single buffer -> add vectored IO -> remove vectored IO to use tokio -> add vectored IO again now that AsyncWrite supports it) and I don't want do it again.

I still think trying to tie them down to the futures crate to be premature.

@aturon
Copy link
Contributor

aturon commented Feb 1, 2018

@seanmonstar Renaming from NotReady to Pending should be a really easy change to make. Am I missing something here?

I don't have strong opinions on the vectored APIs; I think everyone agrees that supporting vectored operations is important, but I didn't realize that hyper was already using them extensively. @cramertj is out today, but he can probably elaborate more on the Buf vs iovec choice.

Note that this RFC includes several other changes to AsyncRead and AsyncWrite.

As to futures-io as a general concept: the AsyncRead/Write traits are intended to play a role akin to Read and Write in the futures-based world. Code that just needs to read and write asynchronously should be able to use these traits without being tied to any particular event loop. For example, such code should be usable with GTK's event loop, or with Fuchsia's. The traits are not actually related to Tokio -- they are related to the task abstraction in futures -- hence the move. That's all orthogonal to the finer details we've been discussing.

@seanmonstar
Copy link

Should be a really easy change to make. Am I missing something here?

Yes, it's not complex to replace a word. I'm just saying that this change breaks everyone, and when that happens, the reason should probably be really good. I'm just not sure that "slightly more pleasing aesthetically" is such a reason, but I won't press harder on this. I do like Pending.

I understand the goal for AsyncRead and AsyncWrite, and the reason behind wanting to make them part of futures-io. I'm simply saying that I don't think those traits are as close to stable as Future or Stream, and so why try to bundle them in when tokio-io can explore them?

@aturon
Copy link
Contributor

aturon commented Feb 1, 2018

@seanmonstar But at the same time you agree that there are other breaking changes, right? IOW, if we're doing a 0.2, it's the right time to perform queued up renamings as well.

Note, futures-io as proposed in this RFC is a separate crate, specifically to allow iteration.

@seanmonstar
Copy link

It might be stretching to hope to include AsyncRead and AsyncWrite in a futures 1.0. If it's proposed to be a separate crate, instead, then it seems like it doesn't need to be part of this RFC (futures 0.2), right?

@aturon
Copy link
Contributor

aturon commented Feb 1, 2018

@seanmonstar Are there specific changes you envision?

@seanmonstar
Copy link

Off the top of my head, I think there's several parts that are unstable:

  • Whether they should be trait AsyncRead: Read and trait AsyncWrite: Write or not.
  • Should there be an associated error type, or always be std::io::Error.
  • Using Buf and BufMut (those traits might need slight changes too).
  • How best handle vectored IO. This is largely elegant with Buf, but there's still slight improvements, such as when a Buf might want more iovecs than the AsyncWrite creates.
  • With default implementations, such as with vectored IO, it's really easy for wrappers to forget to pass through to the underlying type, and so very easily losing out on optimizations some types provide.
  • Probably something else.

Still, I'm not sure this is the right place to discuss this, since as you said, futures-io is a proposed separate crate, and so doesn't seem related to the goal of this RFC (futures 0.2). Shouldn't that part just be removed from the RFC?

@aturon
Copy link
Contributor

aturon commented Feb 5, 2018

@seanmonstar All of the points you're mentioning are changes this RFC is proposing to make.

  • Inheriting from Read/Write. This is considered a misfeature that makes it too easy to plug async objects into code that isn't prepared for them -- something we've encountered when bringing up larger teams into programming with the async I/O model. Instead, we can provide explicit wrappers that allow you to plug these things together (which is useful to be able to do!), giving a more clear acknowledgement of what's going on. That's what this RFC proposes.

  • The error type. I clarified with @cramertj that we should change back to io::Error in this RFC -- there was some confusion about what this would mean for non-tier-1 platforms, but there's a good story for it.

  • Vectored I/O. The rest of your questions are all about the treatment of vectored I/O. There are several reasons behind the proposed move to iovec:

    • Object safety. The proposed traits give you vectored operations that are usable on trait objects, which is not possible with the Buf-based methods.
    • Foundational API. The iovec approach provides the minimal functionality needed to implemented vectored operations, and also ties in with a much simpler and more stable library than bytes. At the same type, you can layer a Buf-based API on top via an extension trait, and have that work with trait objects as well!
    • Minimizing API surface. This gives us the most conservative API surface that supports vectored operations, which is a good foundation for further work later on down the road.

I'll update the RFC soon to detail some of this rationale a bit more. But ultimately, I believe this should only improve the story for libraries like hyper.

@aturon
Copy link
Contributor

aturon commented Feb 5, 2018

I just pushed up an update reflecting the framing with respect to 1.0 and some other details, including some discussion of docs.

Because we are trying to coordinate with the impending Tokio major release, we are hoping to move forward on this RFC in the near future. You can track implementation work on the 0.2 branch.

I made some updates to AsyncRead/AsyncWrite, and also updated the APIs shown in the companion RFC.

At this point, the major open question from the perspective of the futures team is whether to pass a "task" argument explicitly, rather than using TLS. Please weigh in on that RFC; we hope to reach a decision fairly soon.

@seanmonstar
Copy link

Inheriting from Read/Write. This is considered a misfeature [...] That's what this RFC proposes.

I realize some feel this way, hence the proposal. That's why I also said it's an unresolved question around the design of the trait. It's being proposed to remove that inheritance, and I'm suggesting that we don't actually know for sure this is something that should be done yet.

Vectored I/O

Minimizing API surface area makes sense, for sure!

I still wonder if Buf were removed from the bytes crate, if it could be stabilized and we could use it instead.


Simply put, as this an RFC (request for comments), my comments are that AsyncRead and AsyncWrite are a bit immature to suggest pulling them into futures proper, and to instead focus on the core of futures and let other crates (tokio-io, whatever-io) explore it for now.

@aturon aturon merged commit d114779 into rust-lang-nursery:master Feb 7, 2018
@aturon
Copy link
Contributor

aturon commented Feb 7, 2018

This RFC has been merged! Thanks for the discussion, all.

Merging this RFC represents the intent to go forward with a futures 0.2 release as broadly specced out by this RFC; you can follow progress on the 0.2 branch.

The discussion around Context remains open (#2), though the futures team would like to reach a preliminary decision there in the near future to allow for gathering some experience prior to crates.io publication (if we go in the direction of merging the RFC).

@glaebhoerl
Copy link

(:bike: Maybe we could have trait Promise<T> = Future<Item = T, Error = !>? I'm not very familiar with the connotations of these words in other languages, but I can read it as saying "I promise you'll get the T, and not an error".)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.