Rust: Tracking issue for RFC 2592, futures_api

[cramertj]

This is a tracking issue for the std::{future, task} RFC 2592 under the feature gate futures_api.

Steps:

• [x] Implement the RFC (https://github.com/rust-lang/rust/pull/57992)
• [x] Migrate futures-preview and some existing uses of it (Fuchsia) to the latest unstable API to ensure compatibility.
• [x] Make RawWakerVTable fields private and add a const fn constructor.
• [ ] Stabilization PR (see instructions on forge)

Unresolved questions from FCP:

• [x] Should Future::poll take &Waker or a &Context from which an &Waker can be obtained?

[carllerche]

The &Waker argument comes with an implication of Send + Sync + Clone. It is also intended to be accessible from futures being polled and code that notifies the future (not necessarily in a future itself). Passing &Waker directly is a forwards compatibility hazard as it would limit extending the future API. In previous comments, I provides examples where it would be useful to pass in data that is not Send and must remain in the context of the future being polled.

Copying comments:

@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.

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.

[carllerche]

The RawWakerVTable struct contains all public fields and no constructor. This seems like a fowards compatibility hazard as I am not sure how methods could be added to the vtable in the future.

[cramertj]

@carllerche I didn't imagine we'd plan to add fields to that VTable in the future, but I have no opposition to future-proofing against that. I've added a bullet to the top of this issue.

[cramertj]

@carllerche Can you say more about what you have in mind for &Context storage and downcasting? What is Context exactly in your mind? Just another opaque struct with a lifetime parameter and a single fn waker(&self) -> &Waker? And you'd imagine that things like e.g. FuturesUnordered would poll their subfutures with the Context provided but with the waker replaced?

This is what we used to do, so I don't have any particularly strong opposition other than that it complicates the API and has unclear value at this point. If this makes you significantly more comfortable with the API I'm happy to make this change-- it doesn't seem super important to me (extra optional data can always be added through TLS just as well as optional fields of a Context struct, but TLS is slightly less discoverable).

Does anyone have a particularly good reason we shouldn't do this? If not, then I'll submit a PR since it doesn't seem like a significant enough regression in API readability to block on, and it could make future additions more ergonomic.

[carllerche]

@cramertj Yes, that is correct.

The primary reason is forwards compatibility. There are potential additions that can be made to the API but should not be explored immediately. I demonstrated how &Waker prohibits these additions.

Thread-locals do not work in environments that do not have access to thread-locals.

[cramertj]

Thread-locals do not work in environments that do not have access to thread-locals.

Yup-- these environments often don't have threads, either, though, so statics would be fine. But anyways, thanks for the quick response-- I'll open a PR to fix up both of those issues.

[cramertj]

@carllerche Do you care if it's &Context vs. &mut Context? It doesn't seem to matter greatly-- &Context would allow polling multiple futures with the same context, while &mut would potentially allow mutation (I'm assuming we're fine exposing that the Context type is both Send and Sync?).

The current usecase (&Waker) only needs &, but I don't know the full scope of the future extensions you have in mind.

[carllerche]

@cramertj Should it be Send or Sync? My assumption would be that it does not need to be either, and shouldn't (unless there is an argument to make it so that I am not aware of).

As the use case for this argument is to pass "task context" into the future that is only usable from that poll call, i would say &mut Context is the safest. Especially if you consider the use case of potentially adding task-local variables. In that case, being able to set a task-local is important.

[jethrogb]

&mut Context is annoying because you'd have to reborrow if you want to invoke other futures.

[cramertj]

@jethrogb &mut automatically reborrows. It's what we used to do in futures 0.2.

[jethrogb]

Sorry, nevermind, I'm getting my compiler limitations confused :)

[crlf0710]

I think another approach is add another field:
user_data: unsafe fn(*const ()) -> *mut (),
And different runtimes can interpret the * mut () result themselves, like downcasting it into its own type?

[Matthias247]

@crlf0710 How can you be 100% sure that you have been called by your runtime, and not another one, so that it's safe to cast? Futures are interoperable, so a task could be called from various runtimes, even if the code that is current processing the task belongs to a specific runtime (e.g. Tokio).

Regarding Context: I'm fine with having it. Maybe making it &Context makes it more convenient to pass it to multiple subtasks without running into borrow checker issues?

Regarding future-proofing vtable: It's not totally non-future proof. If we find it out there is a significant reason to change things, we can redefine Waker in from storing a RawWaker to enum WakerImpl { RawWaker(v1), RawWaker2(v2) } and add another constructor to it. Obviously with some drawback in memory-usage.

Another point: As recently discussed in discourse, the way of implementing a Waker as an Arc<FutureObj> isn't necessarily the best choice. If the Waker keeps being stored in some leaf IO objects, the task will never be freed (which happened to the respective user).
The alternative implementation that avoids this issue is storing a task handle in the Waker instead of the task itself. However we also need an executor handle in addition to the task handle. So we need to store 2 things. Storing Arc<(Executor,TaskHandle)> inside a RawWaker works - but requires an additional Arc allocation per task in addition to the allocation for the Future.
We could remove that by adding a second data pointer to RawWaker - then one pointer can be used as a handle, and the other as the executor. The drawback is that it increases the size of the Waker by another pointer. And who knows if 2 pointers will ever be enough. The extra allocation can also be somewhat avoided by executors storing and reusing theArc`d taskhandles.

[crlf0710]

@Matthias247 There's indeed unsafety, but how can one expect the Context be inter-operable as well? i.e If i give call Tokio instance's future with another Tokio(or even Romio) instance's Context, how would the runtime recognize that this happens or proceed?

[yoshuawuyts]

I feel like I'm not understanding the reasoning for preferring Context over TLS. @cramertj already pointed out that environments that don't have access to TLS likely also won't have threads, which means 'statics can be used instead for the same purpose.

It also seems odd to me that stdlib would consider extending std structs with random userland extensions to be the encouraged interface. This would blur the boundaries between what's part of std, and what's added by 3rd parties, which seems like a recipe for confusion. More by Boats →

The only argument left for changing the Futures API would be if we were expecting a major addition to the API in std to come along. But given this API has been iterated on for the past 4 or so years, only having an argument of "forwards compatibility" seems unsatisfactory.

I would like people to share exactly which extensions to stdlib futures they envision so we can have an open dialogue about them. If we fail to do this, "forwards compatibility" becomes an impenetrable argument that we all just have to accept as a truth without having a chance to explore what it means.

Appendix

__current API__

fn poll(mut self: Pin<&mut Self>, waker: &Waker) -> Poll<Self::Output>;

__proposed API__

fn poll(mut self: Pin<&mut Self>, context: &mut Context<'_>) -> Poll<Self::Output>;

let waker = context.waker();

---

__edit (2019-03-16)__ There seems to be some confusion about the appendix. The "current" API is what is currently in nightly, and the "proposed" API was taken directly from Taylor's PR to replace Waker with Context.

[shepmaster]

extra optional data can always be added through TLS

Assuming that TLS == thread local storage (and not task local storage), does using this mechanism force a task to always be run on the same thread? If so, is that a problem?

If there are 10 threads and 100 tasks spread across those, would any implementation of task local storage on top of thread local storage require a hashtable-like solution, mapping a given task to its data? Would it still be easy to clean this data up when the task is dropped?

[BigBigos]

Can't the executor set the current task-local-storage state in its thread's thread-local-storage before polling a task? A task should only be polled on one thread at a time and this state can move between threads, alongside the task.

That could even be integrated in the future without executor support. Simply wrap the task in a future that sets the current storage state in TLS on its poll() method and resets it on exit. We could even handle nested tasks by remembering the old state before overwriting it.

Not sure if this is a better approach than passing the state in the context, but it is nonetheless doable.

[carllerche]

@yoshuawuyts

Using thread-local state to store context is not free. In Tokio, the work of swapping out all the thread-locals before calling poll is noticeable in profiles. The futures API already pays the ergonomic cost of passing in a context argument. Allowing libs to inject context in the context struct would allow removing the cost of dealing with thread-local manipulation.

Also, the goal of future proofing the API is to avoid having to explore this path before stabilizing anything.

current:

fn poll(mut self: Pin<&mut Self>, waker: &Waker) -> Poll<Self::Output>;

If worried about typing:

proposed:

fn poll(mut self: Pin<&mut Self>, task: &mut Task) -> Poll<Self::Output>;

The cost of calling task.waker() is negligible given the ratio of implementations that need to get the waker. The vast majority of use cases will be hidden in utilities anyway.

[seanmonstar]

@yoshuawuyts

I'm not understanding the reasoning for preferring Context over TLS. [..] environments that don't have access to TLS likely also won't have threads, which means 'statics can be used instead for the same purpose.

This was the exact same reasoning against adding an argument to poll, but people didn't like it.

In my experience building and maintaining several heavily depended on libraries, forwards-compatibility is pretty important to consider. I'm not pushing for any particular extra feature beyond waking at the moment. I do think it's early to be absolutely certain we wouldn't want additional context passed to a Future, and the proposed changes to a Context gives libstd room to grow if need be.

The ergonomics of creating a waker or context are not as important, it's only going to be done in a couple executor libraries. That it may be a little more annoying isn't reason decide we don't want anything else before knowing.

[jethrogb]

@yoshuawuyts In the RFC thread, @brian0 suggested task spawning be made available through the Context.

[withoutboats]

Moderation note: This conversation about the Waker vs Context API is currently happening in two places, on the tracking issue and on a PR for a specific proposed API change. Could we please move all further discussion about replacing Waker with a further step of indirection to that PR?

---

@jethrogb that functionality used to exist, but was removed. You can find the discussion about it here: rustasync/team#56

[shepmaster]

and on a PR for a specific proposed API change

Could you provide a link to said PR for those of us who aren't currently following it?

[yoshuawuyts]

@shepmaster https://github.com/rust-lang/rust/pull/59119

[withoutboats]

Merging #59119. Proposed stabilization after that merge, discussion to continue in #59725

[ohsayan]

@cramertj #59725 closed, what about this (#59113)