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

[Stabilization] Future APIs #59725

Closed
withoutboats opened this issue Apr 5, 2019 · 68 comments

Comments

Projects
None yet
@withoutboats
Copy link
Contributor

commented Apr 5, 2019

Feature name: futures_api
Stabilization target: 1.36.0
Tracking issue: #59113
Related RFCs:

I propose that we stabilize the futures_api feature, making the Future trait available on stable Rust. This is an important step in stabilizing the async/await feature and providing a stable, ergonomic, zero-cost abstraction for async IO in Rust.

The futures API was first introduced as a part of the std library prior to 1.0. It was removed from std shortly after 1.0, and was developed outside of std in an external crate called futures, first released in 2016. Since that time, the API has undergone significant evolution.

All the APIs being stabilized are exposed through both core and std.

The future Module

We shall stabilize these items in the future module:

  • The std::future module itself
  • The std::future::Future trait and both of its associated items (Output and poll)

We do not stabilize the other items in this module, which are implementation details of async/await as it currently exists that are not intended to be stabilized.

The task Module

We shall stabilize these items in the task module:

  • The std::task module itself
  • The std::task::Poll enum
  • The std::task::Waker struct and all three of its methods (wake, wake_by_ref, will_wake, and new_unchecked). new_unchecked shall be renamed to from_raw.
  • The std::task::RawWaker type and its method new
  • The std::task::RawWakerVTable type and its method new (see #59919)
  • The std::task::Context type and its methods from_waker and waker

Notice: Late Changes to the API

We have decided to merge the future-proofing changes proposed in #59119 to leave room for some potential extensions to the API after stabilization. See further discussion on that issue.

Notes on Futures

The poll-based model

Unlike other languages, the Future API in Rust uses a poll based execution model. This follows a back and forth cycle involving an executor (which is responsible for executing futures) and a reactor (which is responsible for managing IO events):

  • Poll: The executor polls a spawned future until it returns. It passes in a waker; waking that waker will cause the executor to poll this future again. If the future encounters IO, it gives the waker to the reactor and returns pending.
  • Wake: Once there is more progress to be made by polling the future, the reactor calls the wake method on the waker it has registered. This causes the future to be polled again by the executor, continuing the cycle.

Eventually, the future returns ready instead of pending, indicating that the future has completed.

Pinning

The Future trait takes self by Pin<&mut Self>. This is based on the pinning APIs stabilized in 1.33. This contract allows implementers to assume that once a future is being polled, it will not be moved again. The primary benefit of this is that async items can have borrows across await points, desugared into self-referential fields of the anonymous future type.

Changes proposed in this stabilization report

  • Waker::new_unchecked is renamed to Waker::from_raw

std has unsafe constructors following both names, from_raw is more specific than new_unchecked (its a constructor taking the "raw" type which is possibly invalid, asserting that this instance is valid).

  • Waker::wake takes self by value and new Waker::wake_by_ref takes self by reference

The most common waker implementation is to be an arc of the task which re-enqueues itself when the waker is woken; to implement this by reference, you must clone the arc and put it on the queue. But most uses of wake drop the waker as soon as they call wake. This results in an unnecessary atomic reference increment and decrement; instead we now provide by a by-value and by-reference implementation, so users can use the form most optimal for their situation.

Moderation note

The futures APIs have been discussed at enormous length over the past 3 years. Every aspect of the API has been debated, reviewed and considered by the relevant teams and the Rust community as a whole. When posting to this thread, please make a good faith effort to review the history and see if your concern or proposal has been posted before, and how and why it was resolved.

@withoutboats

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2019

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link

commented Apr 5, 2019

Team member @withoutboats has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

  • async-fn-should-not-be-allowed resolved by #59725 (comment)
  • RawWakerVTable-if-generic-would-be-better-even-if-only-for-niche-uses resolved by #59725 (comment)

Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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.

@stjepang

This comment was marked as resolved.

Copy link
Contributor

commented Apr 5, 2019

Just a suggestion for a small change to the waker API that seems to be unaddressed: add a "wake and drop Waker" method, i.e. wake() variant that consumes self. Or change the wake() method so that it consumes self.

This is the most common pattern in practice - we typically don't want to reuse the waker after waking the task. Currently, in most executors, wake() will increment the refcount of the task and drop(waker) will decrement the refcount, which means every time we wake a task, we're wasting two atomic instructions.

I'm also okay with simply changing wake(&self) to wake(self). In that case, waker.clone().wake() will do the same thing waker.wake() currently does (typically also with the same performance characteristics).

See also this comment by @cramertj: aturon/rfcs#16 (comment)

But if anyone thinks this is a too controversial/non-trivial/bikesheddable change, don't worry about it since we can easily add it anytime later.

@withoutboats

This comment was marked as resolved.

Copy link
Contributor Author

commented Apr 5, 2019

It can be done backward compatibly but the thing is it would require adding another constructor to RawWaker that takes the extra fn pointer, which doesn't seem great. I'd be in favor of solving this now, since I don't think its very controversial.

There are use cases where it would be more expensive to go by value, so we shouldn't just change the signature of wake without providing an alternative. If we consider this optimization worth supporting, I would propose this change:

  • Change the signature of wake to be by value, matching @stjepang's proposed API.
  • Add Waker::wake_by_ref which has the signature of the current wake, and a corresponding field to RawWakerVTable.
@jethrogb

This comment was marked as resolved.

Copy link
Contributor

commented Apr 5, 2019

The future Module

We shall stabilize these items in the future module:

  • The std::future module itself
  • The std::future::Future trait and both of its associated types (Output and poll)

Do you mean “associated items”?

The task Module

We shall stabilize these items in the future module:

Do you mean “task module”?

@cramertj

This comment was marked as resolved.

Copy link
Member

commented Apr 5, 2019

+1-- thanks for bringing that up, @stjepang, I was also about to mention that change. I agree with @withoutboats's proposed solution.

@Centril

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

Another thing to note is that we need a #[rustc_const_allow_fn_pointer_only_for_raw_wakers] mechanism specifically for some of the APIs that are being stabilized to make it all work. See discussion in #59119 (comment) for an elaboration.

Basically we need a way on stable Rust to pass values of fn pointer types into const fns. The aforementioned attribute hack will be used to allow this for RawWakerVTable::new. However, this does not stabilize the ability to define such functions on stable Rust. This is not necessary for the purposes of the futures API.

This has not been implemented yet but will be done so adjacent to merging the stabilization PR (hopefully before, and ideally it should not need beta backporting since we have some days to get it merged before master=>beta promotion).

@oli-obk @cramertj Can one of you do the impl / review? (also cc me on that PR)


The change to add wake_by_ref seems great to me.


@rfcbot reviewed

@cramertj

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

Another thing probably worth calling out is the list of Try impls for Poll. They're all designed to bubble up errors as easily as possible inside Future and Stream implementations, but they do not propagate Pending-- that is done with the futures::ready! macro. It's useful to have these as two separate steps, since it's relatively common in manual future implementations to want one but not the other.

There's an impl for Poll<Result<T, E>> and an impl for Poll<Option<Result<T, E>>> (that propagates errors but not Pending or None).

These can be found here.

@eddyb

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

It just crossed my mind that the RawWaker (and the associated vtable) can be made safe to construct: #59739 (comment)

Does anyone know if that had been discussed before? Or even tried and found "subtly impossible (atm)"?
Because I'd rather avoid adding a stable API involving *const ().

@Centril

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2019

So... based on @eddyb's idea and discussions with them as well as with @Nemo157,
I'd like to propose a last-last minute change to the API, namely:

extern {
    type Private;
}

pub struct RawWaker {
    data: NonNull<Private>,
    vtable: &'static RawWakerVTable<Private>,
}

pub struct RawWakerVTable<T: ?Sized = ()> {
    clone: for<'a> fn(&'a T) -> RawWaker,
    wake: unsafe fn(NonNull<T>),
    wake_by_ref: for<'a> fn(&'a T),
    drop: unsafe fn(NonNull<T>),
}

impl RawWaker {
    pub fn new<T>(data: NonNull<T>, vtable: &'static RawWakerVTable<T>) -> Self {
        ...
    }
}

impl<T> RawWakerVTable<T> {
    #[rustc_promotable]
    #[rustc_allow_const_fn_ptr]
    pub const fn new(
        clone: for<'a> fn(&'a T) -> RawWaker,
        wake: unsafe fn(NonNull<T>),
        wake_by_ref: for<'a> fn(&'a T),
        drop: unsafe fn(NonNull<T>),
    ) -> Self {
        ...
    }
}

No other public facing changes are made.

An implementation exists in a playground (aside from #[rustc_allow_const_fn_ptr] which is implemented in 9b34828).

The rationale for doing so is as @eddyb put it:

[...] it's a neat middle-ground between full-on traits and type-unsafe *const ()

In particular, we move from NonNull and to T instead of ().
Moreover, two function pointers stop being unsafe.

@eddyb

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

Note that if you just change *const () to *const T, users don't have to change because they can just rely on T being inferred to (), so that's one compromise you could make.

@carllerche

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

FWIW, w/ raw pointers, it is possible to store additional data in used pointer bits, rendering the pointer invalid. This would be fine if the invalid pointer is passed to a vtable fn as a raw pointer, but passing it as a ref would forbid this use case.

@Centril

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2019

@carllerche So then to support that use case you'd want?:

pub struct RawWakerVTable<T: ?Sized = ()> {
    clone: unsafe fn(NonNull<T>) -> RawWaker,
    wake: unsafe fn(NonNull<T>),
    wake_by_ref: unsafe fn(NonNull<T>),
    drop: unsafe fn(NonNull<T>),
}

(NonNull<T> should work since it is a #[repr(transparent)] newtype to a *const T)

The change to T still buys us some type safety. The for<'a> fn(&'a T) variants can probably be added later as an additional associated function on RawWakerVTable.

@carllerche

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

@Centril I don't have an opinion on the details as long as the use cases are supported (non valid pointers and forwards compatibility).

@Centril

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2019

@carllerche Fair :) Thanks for the input!

@Matthias247

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2019

Why NonNull? For something like a global executor one doesn't need the pointer data, and it could simply stay unused/null. Of course it could also be set to a dummy value, but what would be the rationale for this?

@Centril

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2019

@Matthias247 Well the thinking was that you wouldn't want to pass in null to this so it would be more type-safe to use NonNull. I'm happy to use *const T instead.

Really, the bigger point was to move from () to T and let RawWakerVTable be generic. I think that is more type-safe, ergonomic (since you obviate the need for one cast), and since it is generic it also offers better code reuse.

@withoutboats

This comment has been minimized.

Copy link
Contributor Author

commented Apr 7, 2019

@eddyb @Centril I am not in favor of pursuing this change to the futures API.

First, the only additional check you're encoding in the type system is that when you construct a RawWaker the data pointer you pass has the same type as the arguments to your vtable function. This will not actually prevent you from having to do unsafe casts, since you want is not actually a *const but actually usually an Arc (or in niche use cases not an arc, but also not a raw pointer). This type is also likely to be arbitrary, for that reason, rather than a real meaningful type - I would probably just keep using (). This would also complicate the APIs for use cases which swap out the wake method but use an underlying waker. I really don't see that this would provide much benefit in preventing bugs when using this API; it wold essentially just be a hoop to jump without checking anything meaningful.

Also, this is a niche use case (creating a waker) and even in that niche use case there is a much more ergonomic and type safe API we eventually want to add for the majority of those use cases, which would look roughly like this:

trait Wake {
   fn wake(self: Arc<Self>);

   // default impl overrideable if you dont need ownership
   fn wake_by_ref(self: &Arc<Self>) {
      self.clone().wake()
   }
}

impl Waker {
    pub fn new(wake: Arc<dyn Wake>) -> Waker { ... }
}

This is blocked on a few other things (&Arc as a valid receiver, changes to the std/core infrastructure so we can add std-only methods to core types), so we decided to stick with the RawWaker API for the moment. But in the long term, RawWaker will be a niche within a niche: constructing wakers that don't conform to the simple majoritarian implementation of just being an arc'd trait object. These are things like embedded executors and the intermediate executors in concurrency primitives like futures-unordered.

So this seems like it would complicate the API without actually benefiting the targeted users, who are also a small minority of users. The () is not the challenging unsafe bit of the waker API.

@Centril

This comment has been minimized.

Copy link
Contributor

commented Apr 7, 2019

This will not actually prevent you from having to do unsafe casts, since you want is not actually a *const but actually usually an Arc (or in niche use cases not an arc, but also not a raw pointer).

It won't, this is true. However, if we take e.g. @Nemo157's embrio-rs then we can move from:

// Leaving out `wake_by_ref` here... it would ofc be included in a real implementation.

static EMBRIO_WAKER_RAW_WAKER_VTABLE: RawWakerVTable = RawWakerVTable {
    clone: |data| unsafe { (*(data as *const EmbrioWaker)).raw_waker() },
    wake: |data| unsafe { (*(data as *const EmbrioWaker)).wake() } },
    drop,
};

    pub(crate) fn raw_waker(&'static self) -> RawWaker {
        RawWaker::new(
            self as *const _ as *const (),
            &EMBRIO_WAKER_RAW_WAKER_VTABLE,
        )
    }

into:

static EMBRIO_WAKER_RAW_WAKER_VTABLE: RawWakerVTable<EmbrioWaker> = RawWakerVTable {
    clone: |data| unsafe { (*data).raw_waker() },
    wake: |data| unsafe { (*data).wake() } },
    drop,
};

    pub(crate) fn raw_waker(&'static self) -> RawWaker {
        RawWaker::new(
            self as *const _,
            &EMBRIO_WAKER_RAW_WAKER_VTABLE,
        )
    }

This seems to me strictly better, ergonomic, and readable since it has fewer casts.

This type is also likely to be arbitrary, for that reason, rather than a real meaningful type - I would probably just keep using ().

The use of EmbrioWaker above does not seem arbitrary to me. It seems meaningful.
However, if you want to keep using (), you can of course do that.

This would also complicate the APIs for use cases which swap out the wake method but use an underlying waker. I really don't see that this would provide much benefit in preventing bugs when using this API; it wold essentially just be a hoop to jump without checking anything meaningful.

As the generic parameter T is defaulted to () there is zero extra complication and drawbacks to those that don't need more than (). It is not true that you would need to jump through hoops. Rather, this is a net reduction in hoops that need to be jumped through for all users.

(To be clear, the version I'm talking about here is:

pub struct RawWakerVTable<T: ?Sized = ()> {
    clone: unsafe fn(*const T) -> RawWaker,
    wake: unsafe fn(*const T),
    wake_by_ref: unsafe fn(*const T),
    drop: unsafe fn(*const T),
}

)

But in the long term, RawWaker will be a niche within a niche: constructing wakers that don't conform to the simple majoritarian implementation of just being an arc'd trait object.

I understand this point but does that matter? Even within the niche of the niche it provides an improvement without any cost to those that only ever want (). Why should we intentionally pessimize an API because it is niche?

@Nemo157

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

This will not actually prevent you from having to do unsafe casts, since you want is not actually a *const but actually usually an Arc (or in niche use cases not an arc, but also not a raw pointer).

In most cases isn't the pointer being passed around actually the Arc pointer? @Centril's latest version would allow removing casts completely and just using Arc::into_raw/Arc::from_raw for the internal conversion (e.g. here).

@Centril

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

@rfcbot concern RawWakerVTable-if-generic-would-be-better-even-if-only-for-niche-uses

@cramertj

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

If the pointer is being passed around that way (in an Arc), it can use the even safer ArcWake trait. The point of the RawWakerVTable was to provide more fine-grained access in edge-case scenarios, and I don't think making it generic buys us anything in those cases (and, in fact, can make things harder by requiring functions to be generic that otherwise would not be).

The common case will already be addressed primarily through entirely safe traits. Adding extra guardrails to the unsafe escape hatch isn't necessary here.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

I am ambivalent here. I see the benefits of making RawWaker incrementally more type-safe and (at least in some cases) ergonomic, but I also feel like the overriding concern here is getting futures out the door, so that the rest of the ecosystem can start to build on them.

EDIT: To be clear, for this reason, I'd probably be inclined to move forward with the API as it is, so as to avoid further delay.

@cramertj

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

@pnkfelix did you mean to stop FCP with that comment? I'm happy to work on fixing #60069-- it's a frankly trivial implementation bug, and I don't think FCP for this issue should be delayed by it.

@cramertj

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

A fix is ready in #60088.

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

@cramertj my goal was to register a blocking issue. I did not mean for the FCP timer to be reset (that is, I did not realize that would be a consequence of filing such a concern), but I did want to ensure that the future API's would not be stabilized with that issue outstanding.

@cramertj

This comment has been minimized.

Copy link
Member

commented Apr 19, 2019

Yeah, I'm not sure we have a lever for that unfortunately. It seems like what you'd want is to issue a blocking concern on the stabilization PR so that that PR can only be merged once the issue is fixed, but I don't think we have a mechanism for that at the moment other than leaving a comment.

@lnicola

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2019

Will the timer resume after the concern is marked as resolved, or will it reset to 10 days?

@cramertj

This comment has been minimized.

Copy link
Member

commented Apr 19, 2019

@lnicola The current behavior is set to reset to 10 days. However, I think there's consensus around stabilizing at the end of the original FCP provided that #60088 lands first.

@Centril

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2019

Yep. Remember that rfcbot is a tool. We don't have to always follow the bot to the letter.

@lnicola

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

#60088 has landed.

@golddranks

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

It has now been 10 days since the final comment period started.

@Centril

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

@pnkfelix Can you checkout #60088 to see if you are happy?

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

@rfcbot resolve async-fn-should-not-be-allowed

@rfcbot

This comment has been minimized.

Copy link

commented Apr 23, 2019

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

@shengsheng

This comment has been minimized.

Copy link

commented Apr 23, 2019

@rfcbot another ten days?

@cramertj

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

nah, now we get to ignore rfcbot ;)

bors added a commit that referenced this issue Apr 23, 2019

Auto merge of #59739 - cramertj:stabilize, r=withoutboats
Stabilize futures_api

Fix #59725.
Based on #59733 and #59119 -- only the last two commits here are relevant.

r? @withoutboats , @oli-obk for the introduction of `rustc_allow_const_fn_ptr`.

Centril added a commit to Centril/rust that referenced this issue Apr 23, 2019

Rollup merge of rust-lang#59739 - cramertj:stabilize, r=withoutboats
Stabilize futures_api

cc rust-lang#59725.
Based on rust-lang#59733 and rust-lang#59119 -- only the last two commits here are relevant.

r? @withoutboats , @oli-obk for the introduction of `rustc_allow_const_fn_ptr`.

Centril added a commit to Centril/rust that referenced this issue Apr 23, 2019

Rollup merge of rust-lang#59739 - cramertj:stabilize, r=withoutboats
Stabilize futures_api

cc rust-lang#59725.
Based on rust-lang#59733 and rust-lang#59119 -- only the last two commits here are relevant.

r? @withoutboats , @oli-obk for the introduction of `rustc_allow_const_fn_ptr`.

Centril added a commit to Centril/rust that referenced this issue Apr 24, 2019

Rollup merge of rust-lang#59739 - cramertj:stabilize, r=withoutboats
Stabilize futures_api

cc rust-lang#59725.
Based on rust-lang#59733 and rust-lang#59119 -- only the last two commits here are relevant.

r? @withoutboats , @oli-obk for the introduction of `rustc_allow_const_fn_ptr`.

Centril added a commit to Centril/rust that referenced this issue Apr 24, 2019

Rollup merge of rust-lang#59739 - cramertj:stabilize, r=withoutboats
Stabilize futures_api

cc rust-lang#59725.
Based on rust-lang#59733 and rust-lang#59119 -- only the last two commits here are relevant.

r? @withoutboats , @oli-obk for the introduction of `rustc_allow_const_fn_ptr`.
@Centril

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

Stabilization PR was merged.
Leaving this issue open to let FCP complete naturally.
Closing when that happens.

@sntdevco

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

@Centril How far is 1.36.0 from roll-out?

@Mathspy

This comment has been minimized.

Copy link

commented Apr 29, 2019

@rfcbot

This comment has been minimized.

Copy link

commented May 3, 2019

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

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.