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 and task system to libcore #2418

Closed
wants to merge 2 commits into from

Conversation

aturon
Copy link
Member

@aturon aturon commented Apr 24, 2018

Note: this is a heavily-revised, more conservative version of #2395

This RFC provides the library component for the first-class async/await syntax proposed in a companion RFC. It is intentionally minimal, including the smallest set of mechanisms needed to support async/await with borrowing and interoperation with the futures crate. Those mechanisms are:

  • The task system of the futures crate, which will be moved into libcore
  • A Future trait, which integrates the PinMut APIs with the task system to provide futures (i.e. asynchronous values).

Rendered

@aturon aturon added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Apr 24, 2018
@aturon aturon self-assigned this Apr 24, 2018
@aturon
Copy link
Member Author

aturon commented Apr 24, 2018

Summary of differences from the original RFC:

  • The main trait is called Async rather than Future, to align with async blocks; it now lives in core::ops.
  • No AsyncResult in the proposal; this may be provided in the futures crate.
  • No combinators are included in the proposal; they'll be provided completely out of tree in the futures crate.
  • Futures 0.3 is a more conservative transition that immediately works on stable Rust, but is also compatible with async/await on nightly.
  • There's a minimal path to stabilization that is based almost entirely on tech that has already been vetted for months or years.

@ashfordneil
Copy link

Firstly, is Executor in this rfc a trait or an object? It is defined as a trait, but then used as an object in the Context::new function, I'm not sure if this was intentional due to the "final Executor trait" not being ready, but right now it's a little confusing.

Secondly, why are Waker, Context and (possibly?) Executor all trait objects / structs instead of just traits? This introduces an extra layer of dynamic dispatch (on top of the probable boxing of Async that any executor would have to do) to poll any future that interacts with its context. As an alternative that would reduce the amount of dynamic dispatch, we could make Async::poll a method that is generic over C: Context.

As far as raw implementations of the Async trait are concerned, writing a generic poll method shouldn't be any harder than writing a poll method that uses trait objects in its context. Writers of async functions or end users of combinators shouldn't be affected at all by the change. The only potential issue I can see is how to make Async usable as a trait object (for an executor) with this change. I had a look at the problem, and I think it can be solved.

Basically, instead of the executor making a trait object out of Async and then working with those objects, the executor could define its own trait that has a monomorphised poll method (to work specifically with its implementation of Context) and work with a trait object of that. I've attached a playground that does just this below, and creation of the trait objects all compiles without issue so I think it would be fine.

https://play.rust-lang.org/?gist=079aa77b8679fcf80fd9d56cf7fd00cb&version=stable

Thinking on this, there could be some situations - Async aware mutexes come to mind - where you may still need to create trait objects of the Waker trait to store them. This change wouldn't prevent anyone from creating that trait object themselves if they needed to, but it wouldn't default to handing out trait objects, meaning that in the situations where a trait object isn't necessary, the trait object could be avoided.

Sorry for the wall of text, but what are your thoughts? At the moment I can't see any real disadvantage to doing this*, but there could be some big issue that I missed entirely, so I look forward to hearing other people's opinions on this.

*my playground doesn't use Pin yet so here's hoping that doesn't break everything 🤞

Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

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

As mentioned in the previous RFC, this is roughly in line with what I had hoped the end result would be 👍. I included a couple of nits / questions inline.

/// Any task executor must provide a way of signaling that a task it owns
/// is ready to be `poll`ed again. Executors do so by providing a wakeup handle
/// type that implements this trait.
pub trait Wake: Send + Sync {
Copy link
Member

Choose a reason for hiding this comment

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

Why does this trait need to be moved into core? If UnsafeWake exists in core, then would it not be possible to leave this trait in a crate?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this could live out of tree to start with. To be clear, though, I do expect more of the futures crate to move into core over time.

/// Provides the reason that an executor was unable to spawn.
pub struct SpawnErrorKind { .. }

impl SpawnErrorKind {
Copy link
Member

Choose a reason for hiding this comment

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

Tokio currently requires an "at capacity" error variant.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

/// spawn failures.
///
/// NB: this will remain unstable until the final `Executor` trait is ready.
pub fn executor(&mut self) -> &mut BoxExecutor;
Copy link
Member

Choose a reason for hiding this comment

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

BoxExecutor is not defined anywhere. Is it possible to implement FuturesUnordered with this current definition?

Also, given the known issues* with this strategy, would it be wiser to punt on executor being tied to context?

* spawning in drop and spawning from a function.

Copy link
Member Author

Choose a reason for hiding this comment

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

This should be Executor.

We can always delay on stabilizing this portion of the API if we are feeling uneasy. Remember, everything that goes into std goes on the nightly channel indefinitely; there's a separate process for actually committing it to stable.

@MajorBreakfast
Copy link
Contributor

MajorBreakfast commented Apr 25, 2018

A little downside of Async is that there is also the async keyword, so you can't simply create variables that are called "async".

fn main() {
     let async = print_async(); // No no
     println!("Hello from main");
     futures::block_on(async);
}

Edit: In my PR to the companion RFC, I've just called it my_async to avoid the problem.

@MajorBreakfast
Copy link
Contributor

MajorBreakfast commented Apr 25, 2018

Since the variable cannot be called "async", it could be consistently called "op" in the documentation. A consistent name would be good. "op" would be wonderfully short

let op = print_async(); // Can't call it "async"!
futures::block_on(op);

Edit:
Sry for bikeshedding, but this variable thing somehow really annoys me. (And it looked so promising!) It's inconvenient for everyday code° and it's inconvenient for the documentation.

° Example: In JavaScript, when I create a Promise, I usually call it "promise". This is non-imaginative, I know, but if I have only one it's also super clear and I don't have to come up with a name. Instead, I use the lowercase variant and can continue coding right away.

Alternative names for Async:

  • Task: Like C#, it's even shorter and it is really descriptive of what it we intend it to represent
  • Promise: Like JavaScript. Not as descriptive because "promise" implies a bit that it is already in progress. However, Rust's implementation is lazy.

I think the similarity between the async keyword and the Async trait was an idea with a really good intention. I thought for a long time that it'd be really nice if there was a symmetry here. I liked the previous "Future" well enough, though, so I never voiced that. But, now I start to see why no other language (that I'm aware of) decided to actually call it "Async". 🙂It's inconvenient because of the async keyword!

@yasammez
Copy link

I am not bothered by the naming issue. Normally when I create a future in JS I either await it on the spot (no need for a binding) or gather multiple different things to await them at once later in which case I give them "business" names like customerPromise, basketPromise, etc.. I have never been tempted to name anything "async" :-)

@MajorBreakfast
Copy link
Contributor

MajorBreakfast commented Apr 25, 2018

@yasammez In real code you have that option, yes. But, in documentation where the intention is to stay brief and generic it's problematic. It'll bite us, I'm telling you

@alkis suggests "Lazy" as an alternative in the async/await RFC.

@rushmorem
Copy link

A little downside of Async is that there is also the async keyword, so you can't simply create variables that are called "async".

@MajorBreakfast In that case, why not just name the variable future?

@MajorBreakfast
Copy link
Contributor

@rushmorem Unfortunately that'd be misleading. The new RFC standardizes Future under a different name to highlight the differences to Future from the futures crate (this was first proposed by @aturon). The two traits are very similar, but they're not the same. Good documentation has to be very precise in such matters to not confuse beginners

@tmccombs
Copy link

Task is already taken for the overall task.

@MajorBreakfast
Copy link
Contributor

MajorBreakfast commented Apr 25, 2018

@tmccombs Exactly. "overall task". The meaning of "overall task" would be clearer if it consisted of child tasks. Currently we have to explain what an Async is and then what a task is. But a task is really just a top level and scheduled Async with Output=(). We could just call both tasks and be done with it 🙂

@rpjohnst
Copy link

A task is more than simply an Async<Output=()>- it is a scheduled Async (with any Output).

@glaebhoerl
Copy link
Contributor

(It doesn't feel right to me to name a variable async in the first place because "async" (unlike "future") isn't even a noun.)

/// 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 computation 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.

As I said in the last RFC, I think it's important to make it explicit that it's not an error to poll a future even before a registered 'interest' has caused a task wakeup (otherwise you can't be sure you can implement join/select).

@MajorBreakfast
Copy link
Contributor

@rpjohnst I've edited my post above to reflect this. My point stands, though: Having two terms for this makes it just more complicated than it needs to be. I mean, it's easier to understand if we don't have that. One less thing to learn and such

@glaebhoerl Good point. "reader_async" vs "reader_task"... "async" just doesn't sound so well because it's an adjective

@aturon
Copy link
Member Author

aturon commented Apr 25, 2018

@MajorBreakfast I think that @rpjohnst's point is that there are two distinct concepts here: async values, and tasks. A task is something that is being actively run by an executor, whereas an async value is by itself inert. Tasks generally work with async values internally, and are responsible for driving them to completion.

Understanding this distinction is critical to understanding Rust's unique approach to async programming, so I personally find having distinct terms for the concepts to be very helpful in teaching.

Regarding the larger question: I think that Async works very well as a name, both because of the very direct relationship to the construction form (async blocks) and because it is generally descriptive: Async<Output = T> is an async computation that produces a T. I understand the concern about examples in generic documentation, but my perspective is that docs where you're tempted to write let future = ... could probably be improved by giving a more meaningful name and computation anyway -- the same way that the best documentation avoids using foo/bar/etc in favor of more real names/examples.

@MajorBreakfast
Copy link
Contributor

MajorBreakfast commented Apr 25, 2018

@aturon

A task is something that is being actively run by an executor, whereas an async value is by itself inert.

Does this difference really merit its own terminology? I mean a "moving car" is still called a "car". No special term required to make the difference clear.

my perspective is that docs where you're tempted to write let future = ... could probably be improved by giving a more meaningful name and computation anyway

I'd say it depends. Sometimes it'd just be irrelevant or confusing. For instance in the example code for Future::map().

@rpjohnst
Copy link

It's very similar to the distinction between "process" and "program." A task has extra data associated with it beyond the impl Async that it's running.

@MajorBreakfast
Copy link
Contributor

MajorBreakfast commented Apr 25, 2018

@rpjohnst Yeah sure. But the user doesn't need to care about such implementation details because they're invisible. As far as the user is concerned it's a "running task". (Or a "running program" in your example)

@aturon
Copy link
Member Author

aturon commented Apr 25, 2018

@MajorBreakfast But that's just the thing: an Async is not a running task, and until it's driven by one it will do nothing. It's like the difference between a FnOnce() -> () and a running thread.

@MajorBreakfast
Copy link
Contributor

MajorBreakfast commented Apr 25, 2018

Async is not a running task

@aturon Yes. it'd be just a "Task" at first. True to the meaning of the english word "task": a piece of work. Then once it's driven by an executor, it becomes a "running task".

@Thomasdezeeuw
Copy link

Currently for waking on the same thread I use mio-st's (the name of the fork) Notifier struct, which is basically an Id (usize) and an Rc to a struct Poller which holds a Vec<Id>. Waking is as simple as adding the id to the vector, later these will be drained and the futures will be run.

For the multi-threaded waker I have to following design idea, but not yet implemented. Have a single struct, lets call this MultiThreadedWaker, hold a reference to Poller much like Notifier. The other side of the channel will the be Waker implementation that will just send an Id to the MultiThreadedWaker. MultiThreadedWaker will poll from the channel in a loop, implemented as a Future, and just adds the Id to Poller, much like the thread-local version.

I think this wouldn't allocate (much), only the initial setup and maybe adding onto the channel, this would depend on the implementation. So I wouldn't be double-allocating in this design.

(I know of the design flaw that It's unknown when to poll the MultiThreadedWaker future.)

@cramertj
Copy link
Member

A similar API to the one proposed by this RFC is currently implemented and available on nightly (std::task, std::future) and is being experimented with in the 0.3 release of the futures-preview crate, which supports the nightly implementation of the async/await RFC. The implemented API has been evolving over time as it is used in Fuchsia and in combination with Tokio via the futures 0.1 compatibility shim.

I'd like to propose that we postpone this RFC as-is until we've had a chance to test out and iterate the current implementation. Proposals for changes can be discussed via issues on the futures-rs Github and on the wg-net-async Discord channel. Once the system has stopped undergoing significant churn or new discussion around the APIs that would be stabilized in std::task and std::future, we can reopen a new RFC that reflects the results of the experimentation. I think this approach gives us maximal flexibility while iterating and testing, while at the same time reducing the confusion that could result from in-place changes to this RFC.

@seanmonstar
Copy link
Contributor

@cramertj I've been wondering what the difference is between an "accepted RFC that has an unstable impl in std" and a "postponed/not-accepted RFC that has an unstable impl in std". They seem functionally the same...

@cramertj
Copy link
Member

@seanmonstar

I've been wondering what the difference is between an "accepted RFC that has an unstable impl in std" and a "postponed/not-accepted RFC that has an unstable impl in std". They seem functionally the same...

There are definitely similarities, but the key difference is that we won't stabilize a feature without first accepting an RFC followed by FCP, then having a separate stabilization proposal for the feature followed by FCP. Normally we don't allow implementation of features that haven't been through the RFC/eRFC process, but in this case the work in std was necessary in order to implement and experiment with RFC 2394 (async/await).

The key takeaway is that nothing here can be proposed for stabilization until an RFC has been merged.

@jimmycuadra
Copy link

This would've been the perfect application of an eRFC in my opinion. Ideally the general outline of what was desired would've been proposed here and implementation experiments begun once it was merged. That way there's a clear process of:

  1. eRFC, stating what the goal is and forming agreement that experimentation in nightly is a desired.
  2. Experimentation in implementation to determine the best API.
  3. RFC proposing the API that was settled on.

@cramertj
Copy link
Member

@jimmycuadra I agree! I think that would've been a good way to do it. As the situation stands, I think the async/await RFC is roughly acting as our eRFC here in that it explains in detail what syntax will be supported, but not what the actual libraries or implementation will look like, so that aspect is still undergoing experimentation, and a new RFC will be opened once the API has consensus and is changing infrequently.

@MajorBreakfast
Copy link
Contributor

I agree with you both:

  • I think this should have been an eRFC
  • and I think we should write a proper RFC once everything has settled down. This will make it possible to get extensive feedback on the proposed final design

@alexcrichton
Copy link
Member

@rfcbot fcp postpone

Formally registering @cramertj's postponement above!

@rfcbot
Copy link
Collaborator

rfcbot commented Aug 30, 2018

Team member @alexcrichton has proposed to postpone this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-postpone This RFC is in PFCP or FCP with a disposition to postpone it. final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. and removed proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. labels Aug 30, 2018
@rfcbot
Copy link
Collaborator

rfcbot commented Sep 3, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@carllerche
Copy link
Member

If the RFC has been postponed, why is there a final comment period?

@mark-i-m
Copy link
Member

mark-i-m commented Sep 3, 2018

FCP is part of the normal process. The RFC is not actually postponed until FCP finishes.

@steveklabnik
Copy link
Member

steveklabnik commented Sep 3, 2018 via email

@golddranks
Copy link

Is there going to be quick-n-dirty eRFC for justifying the experimentation in compiler/stdlib, or is that too much bureaucracy? (Only the proper RFC once we are ready for that?)

@cramertj
Copy link
Member

cramertj commented Sep 5, 2018

@golddranks The RFC justifying the current experimentation is the async/await RFC.

@rfcbot rfcbot added the finished-final-comment-period The final comment period is finished for this RFC. label Sep 13, 2018
@rfcbot
Copy link
Collaborator

rfcbot commented Sep 13, 2018

The final comment period, with a disposition to postpone, as per the review above, is now complete.

By the power vested in me by Rust, I hereby postpone this RFC.

@rfcbot rfcbot added postponed RFCs that have been postponed and may be revisited at a later time. and removed final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. disposition-postpone This RFC is in PFCP or FCP with a disposition to postpone it. labels Sep 13, 2018
@rfcbot rfcbot closed this Sep 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
finished-final-comment-period The final comment period is finished for this RFC. postponed RFCs that have been postponed and may be revisited at a later time. T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.