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: stabilize std::task and std::future::Future #2592

Merged
merged 26 commits into from Mar 11, 2019

Conversation

aturon
Copy link
Member

@aturon aturon commented Nov 10, 2018

This RFC proposes to stabilize the library component for the first-class async/await syntax. In particular, it would stabilize:

  • All APIs of the std-level task system, i.e. std::task::*.
  • The core Future API, i.e. core::future::Future and std::future::Future.

It does not propose to stabilize any of the async/await syntax itself, which will be proposed in a separate step. It also does not cover stabilization of the Pin APIs, which has already been proposed elsewhere.

This is a revised and significantly slimmed down version of the earlier futures RFC, which was postponed until more experience was gained on nightly.

Rendered

RFC status

The following need to be addressed prior to stabilization:

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

aturon commented Nov 10, 2018

cc @rust-lang/lang -- I haven't tagged this as T-lang, since the Lang Team already approved the async/await RFC and this is "just" about library APIs. But of course y'all should feel free to weigh in.

cc @Nemo157 @MajorBreakfast @tinaun @carllerche @seanmonstar @olix0r

@aturon
Copy link
Member Author

aturon commented Nov 10, 2018

@cramertj can you comment on UnsafeWake and whether waiting to stabilize that piece looks problematic?

@aturon
Copy link
Member Author

aturon commented Nov 10, 2018

cc @rust-lang/libs, please take a look!

@aturon
Copy link
Member Author

aturon commented Nov 10, 2018

@cramertj I only briefly mentioned Fuchsia in the RFC, but it might be helpful for you/your team to leave some commentary here about your experience with the various iterations of the futures APIs.

@Ixrec
Copy link
Contributor

Ixrec commented Nov 10, 2018

Since then, the syntax, the std APIs, and the futures 0.3 crate have all evolved in tandem as we've gained experience with the APIs. A major driver in this experience has been Google's Fuchsia project, which is using all of these features at large scale in an operating system setting.

Although this isn't strictly relevant to the technical merits of the proposed APIs, considering the sheer scope and history of what we're talking about adding to std it seems worth asking: Are there any blog posts discussing Fuchsia's experience in more detail? This is the only part of the historical context I was completely unaware of, and I couldn't find any place that talks about it.

EDIT: I swear I started typing this before aturon's last comment 😅

@carllerche
Copy link
Member

carllerche commented Nov 10, 2018

Thanks for putting this together.

My experience with the proposed Future trait is that it makes sense as an infrastructure level detail. In a world where the vast majority of async code is written with async / await, the trait makes sense. However, async / await is not there yet. In my experimentation to port existing code to use async / await, I hit limitations pretty quickly. Specifically, as far as I could tell, it is not possible to use a transport properly without extra allocation (due to split).

If the proposed Future is expected to be used be implemented by hand significantly, then moving to it is a net negative. The RFC mentions the ergonomic hit.

Because of this, my plan for Tokio will be to stick on futures 0.1 until async / await has reached a point where implementing Future by hand is no longer required for the user.

Also, most of Tokio would require the UnsafeWake trait, so even with the proposed stabilization, Tokio would not be able to migrate to it.

I understand the desire to drive progress forward. As I said, as far as I can tell, the proposed Future trait is good as an implementation detail of async / await. The ecosystem split will be unfortunate, but perhaps there is no other way.

Edit: I should clarify, Tokio will add support for async / await as it stabilizes, but it will not be considered the primary abstraction until it is able to handle all the cases.

@aturon
Copy link
Member Author

aturon commented Nov 10, 2018

@carllerche You and I have talked about this a bunch on other channels, so I'll be repeating myself, but I want to write a response here so that everyone else is on the same page as well.

There are indeed limitations with async/await today, due not so much to the feature itself as the lack of impl-trait-in-traits (or existential types) working sufficiently well (as well as, ultimately, GATs). They limit the ability to move foundational libraries to use async/await internally, and that's part of the reason we're not ready to stabilize the syntax itself yet. However, to be clear, none of these limitations connect to the Future or task APIs. (And, of course, we also have large-scale usage of these APIs and the current async/await mechanism in Fuchsia to draw from; the limitations largely apply to highly generic code.)

The 0.1/0.3 compatibility system, which allows for fine-grained/incremental migration, ends up doing a lot to lower the stakes. For example, it's already fairly painless to write code for hyper using async fn, and with a little bit more polish it could feel completely "first-class". So, it seems fine for some low-level libraries to stick with the 0.1 model until the above limitations are sufficiently lifted.

I think everything else you raise is discussed in the RFC as well, so I don't have more to add there!

@nrc
Copy link
Member

nrc commented Nov 11, 2018

What’s the rationale for having both task and future modules? Since future only includes Future, it seems that having two modules doesn’t pull its weight. Are we expecting to move a bunch of future stuff into std in the future?

@Redrield
Copy link

@nrc I'm not sure but perhaps the Iterator-like adapters talked about would be merged into that module once they make their way into std?

@ivandardi
Copy link

If Future is a trait then why isn't this example in the RFC

async fn read_frame(socket: &TcpStream) -> Result<Frame, io::Error> { ... }

written as

async fn read_frame(socket: &TcpStream) -> impl Result<Frame, io::Error> { ... }

?

@rpjohnst
Copy link

A small bikeshed: Waker and LocalWaker might be better as SyncWaker and Waker, with the un-marked name being the local one by analogy to Arc and Rc. This makes the poll method signature a bit more straightforward to read and aligns with existing naming convention. I also think there might be a better and more noun-y name than Waker- maybe Task/task::Handle/etc, similar to 0.1?

I'm not sure I totally understand all the layers of &LocalWaker, NonNull<dyn UnsafeWake>, &Arc<T> where T: Wake, etc. and it seems like it might still be in flux, but exactly how many layers of indirection do we have here? Ideally there would be one (as if poll took Arc<dyn Wake> or equivalent), though I can sort-of see why there might need to be two (as if poll took &Arc<dyn Wake>), but it almost looks like there are three?

onto a single operating system thread.

To perform this cooperative scheduling we use a technique sometimes referred to
as a "trampoline". When a task would otherwise need to block waiting for some
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the same "trampoline" concept which is used in the context of avoiding stack overflows for recursive calls?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep!

@Nemo157
Copy link
Member

Nemo157 commented Nov 11, 2018

@ivandardi that's related to the async/await syntax rather than the library code provided here. The design as I understand it is that async fn itself introduces the unnamable type and so entirely encompasses and hides the impl Future part of the transformed function signature. If you want to discuss further the async/await tracking issue would be the more appropriate location.

@aturon
Copy link
Member Author

aturon commented Nov 11, 2018

@nrc

What’s the rationale for having both task and future modules? Since future only includes Future, it seems that having two modules doesn’t pull its weight. Are we expecting to move a bunch of future stuff into std in the future?

Yes, both modules are expected to grow substantially over time. The futures crate contains a similar module hierarchy with a much richer set of APIs. In addition, there will eventually be a stream module. Generally in std we tend to have a pretty flat set of top-level modules supporting various types, e.g. std::option and std::result.

an API with greater flexibility for the cases where `Arc` is problematic.

In general async values are not coupled to any particular executor, so we use trait
objects to handle waking. These come in two forms: `Waker` for the general case, and
Copy link
Contributor

Choose a reason for hiding this comment

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

These don't look like trait objects to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

They're trait objects internally.

Task execution always happens in the context of a `LocalWaker` that can be used to
wake the task up locally, or converted into a `Waker` that can be sent to other threads.

It's possible to construct a `Waker` using `From<Arc<dyn Wake>>`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not right now, because dyn Wake is not Wake.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be Arc<impl Wake> I suppose.

/// - [`Poll::Ready(val)`] with the result `val` of this future if it
/// finished successfully.
///
/// Once a future has finished, clients should not `poll` it again.
Copy link
Contributor

Choose a reason for hiding this comment

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

No behavior is specified for when clients do do that. I think we should say something. For example, "implementors may panic".

Copy link
Member

@Nemo157 Nemo157 Nov 11, 2018

Choose a reason for hiding this comment

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

I think we could even go a bit stronger than that, “implementors should panic, but clients may not rely on this”. All async fn futures guarantee this, and I believe so do the current futures 0.3 adaptors.

I think it would also be good to mention that calling poll again must not cause memory unsafety. The current mention that it can do anything at all makes it seem like it is allowed to have undefined behaviour, but since this is not an unsafe fn the implementer cannot rely on the client’s behaviour for memory safety purposes.

When a task returns `Poll::Ready`, the executor knows the task has completed and
can be dropped.

### Waking up
Copy link
Contributor

@jethrogb jethrogb Nov 11, 2018

Choose a reason for hiding this comment

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

It's unclear at first glance which of the code blocks starting with trait Wake/struct ExecutorInner/struct Waker are proposed for stabilization.

@brain0
Copy link

brain0 commented Nov 11, 2018

(Probably none of you know me, yet I'd still like to offer my opinion, if that is appropriate.)

It is my understanding that the purpose of having an unstable API is that the ecosystem can experiment with it to ultimately avoid stabilizing a bad API. I don't see that this has happened here. Tokio has created a shim that essentially wraps an "std future" into a "0.1 future" with the only purpose of allowing async/await style futures. Apart from that, I haven't seen any experimentation with the std::future API. If tokio (as indicated above) is not even planning to use the new API instead of the old futures 0.1 API, then stabilizing it as-is will IMO be very bad for the ecosystem.

The situation for std::task is worse: From what I can see, it hasn't been used at all. The tokio shim merely provides a noop waker to satisfy std::future's poll signature, but that waker cannot be used and even panics when you try to. I'd like to see any implementation that actually uses std::task - I've been following TWIR all year and haven't found anything. I cannot see that there are comprehensive examples in the docs for std::task, or any reference implementations that show how the system is meant to be used as a whole.

My information is probably incomplete, so please tell me if I am missing anything.

As a side note, I started implementing an "as simple as possible" task executor based on the std APIs, just to understand them and play with them. I found the std::task stuff really complicated, and quickly realized that it still couldn't do everything I needed - most importantly, I needed to access some of the internal data in my Wake implementation, but this was not possible with LocalWaker. I would have to resort to storing information in thread-locals again, which defies the purpose of having the waker passed as an argument to poll.

@steveklabnik
Copy link
Member

steveklabnik commented Nov 11, 2018 via email

@brain0
Copy link

brain0 commented Nov 11, 2018

Right, sorry, I must have overlooked that, it's even mentioned in the RFC. Is that stuff open source? I'd love to look at it.

@krircc
Copy link

krircc commented Nov 11, 2018

@brain0
Copy link

brain0 commented Nov 11, 2018

Someone on reddit found this: https://fuchsia.googlesource.com/garnet/+/master/public/rust/fuchsia-async/src/ (executor.rs is interesting, for example).

@aturon
Copy link
Member Author

aturon commented Nov 11, 2018

@brain0 after the weekend, I expect that @cramertj (or others from the Fuchsia team) will write in with more extensive detail about their experiences.

The RFC also went to some length to lay out the historical context. These APIs have seen plenty of use, both on top of Tokio/Hyper (through various shims) in e.g. web framework code, in embedded settings, and in custom operating systems (Fuchsia).

Could you spell out your concern re: the task system? It'd be helpful to keep discussion focused on specifics if possible.

@brain0
Copy link

brain0 commented Nov 11, 2018

@aturon First things first: Last weekend, I decided to try out the std::task and std::future system by implementing the simplest task executor I could think of, then combine that with mio. Of course, the result would not be as feature-rich or performant as tokio, but it would demonstrate the new APIs.

There were lots of little details that felt "weird" about the task system:

  • When polling a future, you need a LocalWaker. From the documentation, it takes quite a while to figure out how to actually construct a LocalWaker using only safe code. The only way to do so is the local_waker_from_nonlocal function. I would have expected an inherent method on LocalWaker. But when you read the LocalWaker docs, you only find unsafe fn new(inner: NonNull<dyn UnsafeWake + 'static>). The whole API feels centered around the unsafe features and the safe features are "second class".

  • What we all love about Rust is how its type system ensures thread safety. But somehow, when creating a LocalWaker, you either need to call an unsafe function (local_waker) or construct a waker that does not take advantage of being a local waker (local_waker_from_nonlocal). I don't know if there is a better way, but this whole API feels un-rust-y.
    What could be done here is have two traits LocalWake and Wake (where LocalWake is not necessarily Send + Sync) and create a LocalWaker from Rc<dyn LocalWake>. This LocalWaker could then be transformed to a Waker using a method on LocalWake that returns Arc<dyn Wake>. Of course this would be more code and probably has other downsides and issues. Still, it would feel more like Rust, since the compiler verifies thread safety for us again. (Btw, fuchsia doesn't implement wake_local() either, is there any project that actually implements wake_local() to optimize local task wakeups compared to wake()?)

  • Lack of module-level documentation: Usually in Rust, the module-level documentation explains the "big picture" in quite some detail. The std::task documentation is "Types and Traits for working with asynchronous tasks." Only reading the API documentation (starting with LocalWaker, because that's what I needed for poll) didn't help me that much. It's clearly obvious to the people who designed the system, but not so for everyone else. I understand that std::task is new, but I imagine such documentation should exist before stabilization, not after. Due to the lack of documentation, I looked for projects that actually implemented this API and didn't find any (since I did not find the fuchsia executor). Now that I read some of the fuchsia code, I think I put the pieces together correctly.

  • Related to the first and third points above: I tried to create a waker using only safe code. As it turns out, the only way to do that is to put a Wake implementation into an Arc. Inside the Wake implementation, I need two things: A way of identifying the task that should be woken and some kind of reference to the executor itself. The latter has to be wrapped inside an Arc again, so you effectively get something like Arc<(Identifier, Arc<Executor>)>. This felt wrong at first, since it was atypical of Rust to force me to nest two Arcs. A small example in the docs would have helped here. Reading the fuchsia code earlier today showed me that I was not entirely wrong, since they're doing the same thing.

  • I looked into integrating std::task with the mio crate to do some basic networking to see if my (naive) executor would even work. In mio, to identify which Evented received an event, you get a Token (a newtype around a usize). Of course, I could maintain a map Token->Waker. However, since the mio::Poll is tied to my executor, I thought it would be useful to just extract some internal identifier from my Wake implementation and use that as Token. However, I cannot downcast the Waker's internals to its original type to get that information. (Maybe that problem is mio's fault - mio::Poll could be generic on the Token type, so I could simply use my Waker as a token.)

Sorry for the rather verbose reply, I hope it still helped you understand my concerns.

@carllerche
Copy link
Member

@withoutboats

@cramertj @Matthias247 I'd be interested in your thoughts on providing some sort of Option<TypeId> field of RawWaker to support a downcasting interface for Waker. Does that make sense?

I was wondering if Waker should provide this as well. Doing so could potentially allow a strategy for hooking in the tokio reactor, etc:

waker.downcast_ref::<tokio_reactor::Reactor>()

Something like that might work instead of a set of thread-locals.

(I haven't thought it through though).

Centril added a commit to Centril/rust that referenced this pull request Feb 14, 2019
Update the future/task API

This change updates the future and task API as discussed in the stabilization RFC at rust-lang/rfcs#2592.

Changes:
- Replacing UnsafeWake with RawWaker and RawWakerVtable
- Removal of LocalWaker
- Removal of Arc-based Wake trait
@cramertj
Copy link
Member

@carllerche That seems like an interesting idea, though I'm not sure exactly how it works e.g. when passing through a FuturesUnordered. It seems like a good thing to explore in a future proposal-- we can safely add new, nullable fields to RawWaker later (the current nightly implementation makes the fields private and includes a const fn constructor-- I'll update the RFC to reflect this).

@carllerche
Copy link
Member

@cramertj Thinking more about how a downcasting strategy would work, it seems critical to me to not directly pass &Waker to Future. Instead, Future should take &Context from which a Waker can be obtained.

The reason being, when calling the future w/ the Tokio runtime context (reactor, timer, ...), this info can only stay on the stack. The handle that gets cloned to notify should not be the one that gets downcast. It would be very difficult to support this as well as confusing IMO.

I suspect that the Tokio runtime context is not the only thing that should not be propagated when cloning the waker. I would strongly recommend switching back to &Context for forwards compatibility.

@carllerche
Copy link
Member

Another example, if there is a task-local storage implementation, the necessary context would need to be passed in via the arg to Future::poll. It is also clear that task-local storage is task local and should not be sent to other tasks. The Waker value is intended to be sent to other tasks. Given this, exposing additional concepts via methods on Waker is not correct.

@Matthias247
Copy link
Contributor

Matthias247 commented Feb 17, 2019

@carllerche
Just a short comment regarding Context: It personally liked the extensibility that the struct parameter offered. So I would be fine with adding it back - but I wasn't part of the removal discussion, and there were certainly good reasons for removing it.
Nevertheless I would suggest adding nothing expect Waker to a hypothetical Context for the moment. Stabilizing the current APIs is already hard enough, we shouldn't add more than we need.

Regarding downcasting/thread-locals/etc:
I think it would be helpful if you describe what you actually want to enable and achieve for users. We are mostly talking about the "how" (with downcasting, thread-locals, etc), but not a lot about the "what".

If it's about propagating runtime characteristics into all futures, to e.g. enable a user to use call await tokio_deadline(1000ms) without the user explicitly threading through and utilizing a runtime reference, I would actually discourage this. My reasoning here is that the futures ecosystem should actually be very composable and compatible compared to async IO systems on other platforms. Executors and "reactors" are not necessarily the same thing, and things work totally fine if there are multiple executors in the system (e.g. a tokio one, a futures-rs one, and another one). All of them can interact with sockets/timers/etc which are provided by IO sources. I think adding mechanisms where certain IO code will only work when called from a specific executor should be avoided. It would imho make code less compatible, since e.g. a developer moving some code from executor A to executor B (maybe because B offers an efficient subtask-executor) could lead to breaking the program.

I am personally also an advocate of threading through these kinds of dependencies (in terms of SocketFactory, TimerService or other interfaces/traits) in application code, since I think it makes testing easier, and is extensible for any amount of dependencies. So I personally think the implicitly passed things might do more harm than good (but I totally understand that some people like the convenience, and having both is also ok).

Back to integrating reactor-specific behavior into Waker or Context: What would happen if the task is not polled from a tokio executor, but the user still calls the function which performs the downcast: The cast would obviously fail. In the same fashion as a TLS lookup would fail. Would tokio then implement a fallback, which e.g. references a global runtime instead of the current one? Or would it return an error?

@carllerche
Copy link
Member

To clarify, I am not proposing to add any additional functionality in this RFC. I am showing how the current RFC is not forwards compatible with potential changes that could be good to make at a future time.

For now, I would suggest bringing back a Context that only is able to provide a waker.

@Ralith
Copy link

Ralith commented Feb 17, 2019

I think @Matthias247 enumerated some good reasons why the poll parameter specifically shouldn't be a generic extension point, i.e. to avoid hidden dependencies. I don't think anything's changed since this was discussed in depth when the transition away from Context was originally made.

@seanmonstar
Copy link
Contributor

They seem like fair reasons for not wanting them. Though, Carl showed a couple reasons why it'd be impossible to add them if desired in the future.

I didn't notice any real discussion about removing Context originally, other than the only thing it had was a Waker. Preparing forwards-compatible APIs is pretty important, especially in libstd where breaking changes are impossible.

@carllerche
Copy link
Member

Yes, I'm not here to litigate whether or not X feature is good or not. We specifically don't want to do that now in order to ship. I am pointing out that the current design prohibits future changes and Future in std should be forwards compatible.

@Ralith
Copy link

Ralith commented Feb 18, 2019

There has to be a line drawn somewhere against complicating interfaces just in case we some day change our minds about how they should work; otherwise, you can justify arbitrarily large amounts of indirection and abstraction.

@josephg
Copy link

josephg commented Mar 11, 2019

There hasn't been any action in nearly a month. What are the next steps to merging this into stable?

@yoshuawuyts yoshuawuyts mentioned this pull request Mar 11, 2019
Co-Authored-By: cramertj <cramertaylorj@gmail.com>
@cramertj cramertj merged commit d50ce8a into rust-lang:master Mar 11, 2019
@cramertj
Copy link
Member

This RFC has been merged! The remaining unresolved items should be discussed on the tracking issue.

@asonix
Copy link

asonix commented Mar 11, 2019

woo!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-async-await Proposals re. async / await A-futures Futures related proposals A-traits-libstd Standard library trait related proposals & ideas A-types-libstd Proposals & ideas introducing new types to the standard library. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this RFC. T-lang Relevant to the language team, which will review and decide on the RFC. 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.

None yet