Repository navigation
Tracking issue for RFC 2592, futures_api #59113
Description
This is a tracking issue for the std::{future, task} RFC 2592 under the feature gate futures_api.
Steps:
- Implement the RFC (Update the future/task API #57992)
- Migrate
futures-previewand some existing uses of it (Fuchsia) to the latest unstable API to ensure compatibility. - Make
RawWakerVTablefields private and add aconst fnconstructor. - Stabilization PR (see instructions on forge)
Unresolved questions from FCP:
- Should
Future::polltake&Wakeror a&Contextfrom which an&Wakercan be obtained?
Reacted by shekohex, Max Chertov and Daniel Müller
Reacted by Sean McArthur, Yosh, David Barsky, Tom Houlé, Wesley Wiser, Lucio Franco, Cameron Taggart, Taiki Endo, Tux3, Thomas Eizinger and 35 more
Activity
carllerche commentedon Mar 11, 2019
The
&Wakerargument comes with an implication ofSend + 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&Wakerdirectly 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
&WakertoFuture. Instead,Futureshould take&Contextfrom which aWakercan 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
&Contextfor 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
Wakervalue is intended to be sent to other tasks. Given this, exposing additional concepts via methods onWakeris not correct.carllerche commentedon Mar 11, 2019
The
RawWakerVTablestruct 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 commentedon Mar 11, 2019
@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 commentedon Mar 11, 2019
@carllerche Can you say more about what you have in mind for
&Contextstorage and downcasting? What isContextexactly in your mind? Just another opaque struct with a lifetime parameter and a singlefn waker(&self) -> &Waker? And you'd imagine that things like e.g.FuturesUnorderedwould poll their subfutures with theContextprovided but with thewakerreplaced?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
Contextstruct, 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 commentedon Mar 11, 2019
@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
&Wakerprohibits these additions.Thread-locals do not work in environments that do not have access to thread-locals.
cramertj commentedon Mar 11, 2019
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 commentedon Mar 11, 2019
@carllerche Do you care if it's
&Contextvs.&mut Context? It doesn't seem to matter greatly--&Contextwould allow polling multiple futures with the same context, while&mutwould potentially allow mutation (I'm assuming we're fine exposing that theContexttype is bothSendandSync?).The current usecase (
&Waker) only needs&, but I don't know the full scope of the future extensions you have in mind.carllerche commentedon Mar 11, 2019
@cramertj Should it be
SendorSync? 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
pollcall, i would say&mut Contextis 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.7 remaining items
shepmaster commentedon Mar 12, 2019
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 commentedon Mar 12, 2019
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 commentedon Mar 12, 2019
@yoshuawuyts
Using thread-local state to store context is not free. In Tokio, the work of swapping out all the thread-locals before calling
pollis 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:
If worried about typing:
proposed:
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 commentedon Mar 12, 2019
@yoshuawuyts
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 aContextgives 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 commentedon Mar 12, 2019
@yoshuawuyts In the RFC thread, @brian0 suggested task spawning be made available through the
Context.withoutboats commentedon Mar 12, 2019
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 commentedon Mar 12, 2019
Could you provide a link to said PR for those of us who aren't currently following it?
yoshuawuyts commentedon Mar 12, 2019
@shepmaster #59119
withoutboats commentedon Apr 5, 2019
Merging #59119. Proposed stabilization after that merge, discussion to continue in #59725
Rollup merge of rust-lang#59119 - cramertj:cx-back, r=withoutboats
Rollup merge of rust-lang#59119 - cramertj:cx-back, r=cramertj
Auto merge of #59119 - cramertj:cx-back, r=cramertj
Auto merge of #59119 - cramertj:cx-back, r=withoutboats
ohsayan commentedon May 3, 2019
@cramertj #59725 closed, what about this (#59113)