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

Stabilize async_await in Rust 1.39.0 #63209

Merged
merged 4 commits into from
Aug 20, 2019

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Aug 2, 2019

@Centril Centril added T-lang Relevant to the language team, which will review and decide on the PR/issue. relnotes Marks issues that should be documented in the release notes of the next release. A-async-await Area: Async & Await F-async_await labels Aug 2, 2019
@Centril Centril added this to the 1.38 milestone Aug 2, 2019
@rust-highfive

This comment has been minimized.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 2, 2019
@Centril Centril added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 2, 2019
@Centril Centril mentioned this pull request Aug 2, 2019
4 tasks
@cramertj

This comment has been minimized.

@Centril

This comment has been minimized.

@bors

This comment has been minimized.

@Centril Centril force-pushed the stabilize-async-await branch from 16df48a to c8dd6e7 Compare August 3, 2019 12:31
@bors

This comment has been minimized.

Centril added a commit to Centril/rust that referenced this pull request Aug 8, 2019
…sts, r=cramertj

Test interaction between `async { ... }` and `?`, `return`, and `break`

Per the second checkbox in rust-lang#62121 (comment), test that `async { .. }` blocks:
1. do not allow `break` expressions.
2. get targeted by `return` and not the parent function.
3. get targeted by `?` and not the parent function.

Works towards resolving blockers in rust-lang#63209.

r? @cramertj
@est31

This comment has been minimized.

@jonas-schievink

This comment has been minimized.

@Centril

This comment has been minimized.

@est31

This comment has been minimized.

Centril added a commit to Centril/rust that referenced this pull request Aug 12, 2019
…s, r=nikomatsakis

`async fn` lifetime elision tests

Add `async fn` version of the tests in rust-lang#61207 per the first checkbox in rust-lang#62121 (comment).
Works towards resolving blockers in rust-lang#63209.

r? @nikomatsakis
cc @cramertj
@pietroalbini
Copy link
Member

Rust 1.38 is scheduled to branch off today, but this PR didn't land yet and there are a bunch of blockers still open. If we still want to stabilize async_await in 1.38 we need to land this stabilization PR right now and then backport the blockers.

I'd prefer to stabilize async_await in Rust 1.39 though, to give the compiler and language team more time to properly iron the feature out. I'm aware lots of people would like to use async_await as soon as possible, but I'm not happy rushing things out.

cc @rust-lang/lang @rust-lang/release

Centril added a commit to Centril/rust that referenced this pull request Aug 13, 2019
…s, r=nikomatsakis

`async fn` lifetime elision tests

Add `async fn` version of the tests in rust-lang#61207 per the first checkbox in rust-lang#62121 (comment).
Works towards resolving blockers in rust-lang#63209.

r? @nikomatsakis
cc @cramertj
Centril added a commit to Centril/rust that referenced this pull request Aug 13, 2019
…s, r=nikomatsakis

`async fn` lifetime elision tests

Add `async fn` version of the tests in rust-lang#61207 per the first checkbox in rust-lang#62121 (comment).
Works towards resolving blockers in rust-lang#63209.

r? @nikomatsakis
cc @cramertj
@crlf0710

This comment has been minimized.

@XAMPPRocky

This comment has been minimized.

tesuji added a commit to tesuji/hyper that referenced this pull request Aug 21, 2019
`async_await` is stabilized in rust-lang/rust#63209.
This commit fixes `hyper` build with latest nightly compiler.
tesuji added a commit to tesuji/hyper that referenced this pull request Aug 21, 2019
`async_await` is stabilized in rust-lang/rust#63209.
This commit fixes `hyper` build with latest nightly compiler.
@killercup
Copy link
Member

@xfix nope, was a relic of an unfulfilled future: #63762

@XAMPPRocky
Copy link
Member

XAMPPRocky commented Aug 21, 2019

@golddranks My understanding of that comment is that it's possible to cause UB in async await with -Zmutable-alias specifically, not in the general case. -Z flags are always unstable so bugs arising from their use wouldn't block stabilisation. There's also a significant period of time from the stabilisation PR to being available on stable, so any critical issues can be appropriately addressed. It is pretty uncommon for such issues to merit a stabilisation PR to be reverted though.

@Centril
Copy link
Contributor Author

Centril commented Aug 21, 2019

@KiChjang Yes, what I'm trying to make sure of is that since no discussion ensued after @RalfJung 's comment, was the newly found issue taken in consideration and checked that it's not a serious issue for now?

@golddranks Yep; It was considered on a language team meeting and our understanding matches @XAMPPRocky's re. -Z mutable-alias.

@hanna-kruppe
Copy link
Contributor

hanna-kruppe commented Aug 21, 2019

@Centril I think this understanding is flawed. As already laid out in #63209 (comment), -Z mutable-alias is unstable & off by default (indeed, only exists in the first place) solely to work around long-standing LLVM optimizer bugs. If those bugs weren't there, we'd be happily emitting noalias and would have an live unsoundness at our hands. That's because this is not just some random LLVM IR generation experiment we've tried on a whim: Rust-the-language puts aliasing restrictions on mutable references (and other pointers) to allow more aggressive optimizations and better reasoning about code. So we are really talking about aliasing restrictions at the language/MIR level, noalias in LLVM IR is just one specific way those may be exploited. And it's by no means the only way these aliasing restrictions show up -- consider miri checking for UB, MIR optimizations, and other non-LLVM optimizers (or different formats for communicating Rust's aliasing restrictions to LLVM).

While we don't yet have these aliasing restrictions fully fleshed out, many (all?) reasonable forms they could take appear incompatible with async/await (at least as it is currently lowered), since in @comex's example it leads to two "clearly distinct" pointers aliasing. This is clearly visible even at the MIR level, after the state machine transform. Thus the only hope I see for this issue "just disappearing" without wider impact is if we find a different way to lower async fns/blocks that side-steps the issue and still delivers everything else we want from async fns. This seems unlikely to me, tbh.

If that doesn't happens, stabilizing async/await doesn't simply mean we forever barred from re-enabling noalias emission for a cheap codegen improvements once relevant LLVM bugs are fixed, we're actually affecting the language's (MIR's) memory model, specifically the aliasing rules. For example, one "hotfix" discussed in rust-lang/unsafe-code-guidelines#194 (comment) would be to give Pin<&mut T> looser aliasing rules than &mut T. This has repercussions beyond just async/await. That may be okay, or desirable for other reasons, but it needs deliberate consideration, which based on your comment the lang team apparently didn't do.

@Centril
Copy link
Contributor Author

Centril commented Aug 21, 2019

@rkruppe Thanks for a detailed write-up.

@Centril I think this understanding is flawed. As already laid out in #63209 (comment), -Z mutable-alias is unstable & off by default (indeed, only exists in the first place) solely to work around long-standing LLVM optimizer bugs. If those bugs weren't there, we'd be happily emitting noalias and would have an live unsoundness at our hands. That's because this is not just some random LLVM IR generation experiment we've tried on a whim: Rust-the-language puts aliasing restrictions on mutable references (and other pointers) to allow more aggressive optimizations and better reasoning about code. So we are really talking about aliasing restrictions at the language/MIR level, noalias in LLVM IR is just one specific way those may be exploited. And it's by no means the only way these aliasing restrictions show up -- consider miri checking for UB, MIR optimizations, and other non-LLVM optimizers (or different formats for communicating Rust's aliasing restrictions to LLVM).

This is my understanding as well and I did take the time to read up on the history of removing the flag before stabilizing.

If that doesn't happens, stabilizing async/await means not only are we forever barred from re-enabling noalias emission for a cheap codegen improvements once relevant LLVM bugs are fixed, we're actually affecting the language's (MIR's) memory model, specifically the aliasing rules.

Yes, I agree and it would be very unfortunate not to have the desired optimizations outside async fn / generators and whatnot.

For example, one "hotfix" discussed in rust-lang/unsafe-code-guidelines#194 (comment) would be to give Pin<&mut T> looser aliasing rules than &mut T.

Agreed. And at least Pin is already a language item and a bit special (although not in a fundamental way at the moment) so it does not seem outrageous to extend that albeit in very major ways. I'm thinking specifically of @RalfJung's notes in rust-lang/unsafe-code-guidelines#148 (comment) wrt. our options although this will substantially complicate our operational semantics. And I agree with Ralf that...

at the end of the day, we miss the PinMut abstraction

We might... :/

A &pin T type is looking increasingly good right now. However, ultimately there is little we can do today as far as I understand it. The Future trait is stable and uses the current formulation of Pin so it seems our hands are tied. We are faced with unfortunate choices but it seems clear that the desire for async_await is strong so I don't think people will accept waiting for us to figure out -Z mutable-alias.

I should also note that @cramertj made a comment re. removing Freeze from generators although they would be better placed to elaborate on that. cc also @nikomatsakis

@eddyb
Copy link
Member

eddyb commented Aug 21, 2019

T: Freeze only means anything for &T (just like T: Sync means &T: Send), but all operations on generators need &mut T, so I wouldn't expect Freeze to matter there.

@vorot93
Copy link

vorot93 commented Aug 21, 2019

@Centril Should have been expected that std::future::Future is not really stable until polished async/await lands. If anything, it's better to change it now, rather than after it together with async/await go live in the ecosystem.

@Centril
Copy link
Contributor Author

Centril commented Aug 21, 2019

If anything, it's better to change it now, rather than after it together with async/await go live in the ecosystem.

We have a commitment to stability and cannot break folks who use Pin and Future as encoded today.

@nicoburns
Copy link

Should have been expected that std::future::Future is not really stable until polished async/await lands.

I thought it was bizarre that std::Future was stabilised before the design/implementation of async-await was completed. It seemed like a bad idea for exactly this kind of reason. I guess the Rust teams hand was a bit forced by Tokio's refusal to implement against unstable futures though.

We have a commitment to stability and cannot break folks who use Pin and Future as encoded today.

Soundness bugs are an exception, right? Unintentionally altering the languages memory model would seem to be in a similar category to me. It's unfortunate, but unless a workaround can found, I think breaking changes to std::Future should strongly be considered.

the desire for async_await is strong so I don't think people will accept waiting for us to figure out -Z mutable-alias.

The desire for async-await is strong. But there are also lot's of people who will never use this feature. For this feature to unexpectedly have a significant impact on non-async code generation is not something the community is likely to take well. Even as someone who is hotly anticipating async-await, I would be pretty concerned with that.


Regardless of the decision taken, I think this merits serious consideration rather just "It's stable: there's nothing we can do now".

@vorot93
Copy link

vorot93 commented Aug 21, 2019

@Centril I know that Rust team takes stability guarantees seriously. But realistically speaking, most (the only?) current std::future::Future users are the folks active in the same Discord. I believe it will stay that way until a compelling reason for transition surfaces (async/await).

It's better to propagate breaking changes across a very small community now rather than suffer when it will really be too late to fix.

@Centril
Copy link
Contributor Author

Centril commented Aug 21, 2019

@nicoburns

I thought it was bizarre that std::Future was stabilised before the design/implementation of async-await was completed. It seemed like a bad idea for exactly this kind of reason.

I agree it was not ideal from a process POV where the main issue was Pin<T> which in hindsight should not have been approved by the library team and should have also needed language team approval instead. Even so, that probably would not have changed anything.

Although, for what it's worth, I did ask some vague questions about exactly this at the time, #55766 (comment) which was replied to in #55766 (comment) which does not seem true today. Unfortunately, Ralf did not have a time machine.

In the end, it was just happenstance that we noticed this now which is mainly because the UCG got to this point. We could have easily stabilized all of pin, futures, async_await and found this out 2 months later. Mistakes where made, and we have to live with them.

I guess the Rust teams hand was a bit forced by Tokio's refusal to implement against unstable futures though.

Yes, that didn't help. In the future I think that we need to be clearer that stakeholders pushing for a feature need to be part of testing it out on nightly.

Soundness bugs are an exception, right?

They are, but we still have to be careful even in the even of a soundness bug to deal with the fallout in a graceful way.

Unintentionally altering the languages memory model would seem to be in a similar category to me. It's unfortunate, but unless a workaround can found, I think breaking changes to std::Future should strongly be considered.

@RalfJung's comments in rust-lang/unsafe-code-guidelines#148 (comment) suggests that a workaround can be found to recover noalias for code which does not care about Pin.

The desire for async-await is strong. But there are also lot's of people who will never use this feature.

Certainly; For example, I don't expect I will personally use this feature that much, at least not now.

For this feature to unexpectedly have a significant impact on non-async code generation is not something the community is likely to take well. Even as someone who is hotly anticipating async-await, I would be pretty concerned with that.

See above re. @RalfJung's notes. I believe we should be able to recover noalias at the cost of complicating our operational semantics. This cost will primarily be borne by 1) unsafe code authors who will have to account for this in their mental model, 2) Rust compiler developers (including prospective alternative compilers), and 3) PhDs and language specifiers like @RalfJung. On the other hand, having something like &pin T would have added type system complexity instead of costs in the operational semantics so there's a trade-off here and it's not so clear that complexity in the type system is better or worse than in the abstract machine.

Regardless of the decision taken, I think this merits serious consideration rather just "It's stable: there's nothing we can do now".

For what it's worth I have debated this internally and given it a non-trivial amount of thought.

@vorot93

It's better to propagate breaking changes across a very small community now rather than suffer when it will really be too late to fix.

Sure, it's always true that a breaking change is easier to propagate when fewer rely on them. However, I believe you are underestimating the disruption this would cause both to code and to trust in our leadership.

@golddranks
Copy link
Contributor

I'd like to point out that one of the foremost things that would prevent erosion of trust towards the Rust leadership and decision process, is to have someone with understanding of the matter as thorough as possible, to write a summarisation that considers the Rust memory model, what is being lost, what is not, what can be done, what are the possibilities for action considering the leadership/release/stabilisation model et cetera. It's FUD that is the poison, and information from knowledgeable and authoritative source is the best remedy. Things concerning Rust memory model and optimisation potential sound serious, so I think they should be taken quite seriously.

@RalfJung
Copy link
Member

RalfJung commented Aug 21, 2019

@Centril

It was considered on a language team meeting and our understanding matches @XAMPPRocky's re. -Z mutable-alias.

Like (I think) @rkruppe, I read this as saying that "soundness bugs on the Rust side with -Z mutable-noalias are not actually soundness bugs", which would be very concerning. @Centril can you clarify the lang team's stanza on this?

I believe we should be able to recover noalias at the cost of complicating our operational semantics.

So, that note is my plan for how to approach the problem, when/if me or someone else gets around to actually do that. map_unchecked_mut would have to still be deprecated (in favor of a method that works with *mut T instead of &mut T), and all other methods on Pin<&mut T> also could not use the DerefMut instance. Then I hope we can avoid having any mutable reference covering the Future, and we can also avoid actually accessing it fields, so we should be good.

Note that e.g. #[derive(Debug)] on futures still inherently cannot work. One key point of mutable references is to allow the compiler to move around reads and writes, including across unknown function calls and yield points -- and such optimizations would certainly change the behavior of a program that debug-prints the environment of a Future.

Btw, Pin is not the only place where we have too much noalias currently.


Things concerning Rust memory model and optimisation potential sound serious, so I think they should be taken quite seriously.

I think they are. The trouble is, we are still very early in exploring the design space for Rust memory models. It's basically impossible to say anything precise.

@gnzlbg
Copy link
Contributor

gnzlbg commented Aug 21, 2019

There are currently no known miscompilations, likely because the use of thread-local storage confuses LLVM enough that it cannot analyze the code well enough to exploit this UB.

I have a prototype here (https://github.com/gnzlbg/break_aw) that does not use thread-local storage and that allows LLVM to optimize across the yield point. I haven't been able to cause a misoptimization yet.

seanmonstar pushed a commit to hyperium/hyper that referenced this pull request Aug 21, 2019
@Aaron1011
Copy link
Member

Aaron1011 commented Aug 21, 2019

@RalfJung:

Note that e.g. #[derive(Debug)] on futures still inherently cannot work.

Is this a limitation of your current approach to resolving this issue, which could eventuantally removed? Or is this a fundamental restriction imposed the interaction of Pin, self-referential generators, and mutable references?

@Centril
Copy link
Contributor Author

Centril commented Aug 22, 2019

@RalfJung

Like (I think) @rkruppe, I read this as saying that "soundness bugs on the Rust side with -Z mutable-noalias are not actually soundness bugs", which would be very concerning. @Centril can you clarify the lang team's stanza on this?

I believe they still are soundness bugs especially in the cases where async-await, generators, and pin are not involved. At least this is my personal opinion and that of the language team historically (to my knowledge) and we haven't actively changed our minds here.

@jeffvandyke
Copy link

Related, Issue #54878 and Issue #45029 track -Zmutable-noalias - it looks like this is enabled by default for panic = abort, which doesn't unwind and cause an issue.

@hanna-kruppe
Copy link
Contributor

@jeffvandyke We did emit noalias for panic=abort for a while but had to revert that, see #54639

@RalfJung
Copy link
Member

RalfJung commented Aug 22, 2019

@Aaron1011

Is this a limitation of your current approach to resolving this issue, which could eventuantally removed? Or is this a fundamental restriction imposed the interaction of Pin, self-referential generators, and mutable references?

I think this is fundamental. Adding derive(Debug) makes the exact content of local variables at any yield point observable. Interesting optimizations based on mutable references being unique rely on the content mutable references point to to not be observable by any other alias.

@Mark-Simulacrum
Copy link
Member

Please direct further discussion to #63818.

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Nov 11, 2019
Pkgsrc changes:
 * Remove patch which no longer applies (but what about RPATH?)
 * Adapt a few patches to changed files upstream.

Upstream changes:

Version 1.39.0 (2019-11-07)
===========================

Language
--------
- [You can now create `async` functions and blocks with `async fn`,
  `async move {}`, and `async {}` respectively, and you can now call
  `.await` on async expressions.][63209]
- [You can now use certain attributes on function, closure, and function
  pointer parameters.][64010] These attributes include `cfg`, `cfg_attr`,
  `allow`, `warn`, `deny`, `forbid` as well as inert helper attributes used
  by procedural macro attributes applied to items. e.g.
  ```rust
  fn len(
      #[cfg(windows)] slice: &[u16],
      #[cfg(not(windows))] slice: &[u8],
  ) -> usize {
      slice.len()
  }
  ```
- [You can now take shared references to bind-by-move patterns in the
  `if` guards of `match` arms.][63118] e.g.
  ```rust
  fn main() {
      let array: Box<[u8; 4]> = Box::new([1, 2, 3, 4]);

      match array {
          nums
  //      ---- `nums` is bound by move.
              if nums.iter().sum::<u8>() == 10
  //                 ^------ `.iter()` implicitly takes a reference to `nums`.
          => {
              drop(nums);
  //          ----------- Legal as `nums` was bound by move and so we have ownership.
          }
          _ => unreachable!(),
      }
  }
  ```

Compiler
--------
- [Added tier 3\* support for the `i686-unknown-uefi` target.][64334]
- [Added tier 3 support for the `sparc64-unknown-openbsd` target.][63595]
- [rustc will now trim code snippets in diagnostics to fit in your terminal.]
  [63402] **Note** Cargo currently doesn't use this feature. Refer to
  [cargo#7315][cargo/7315] to track this feature's progress.
- [You can now pass `--show-output` argument to test binaries to print the
  output of successful tests.][62600]

\* Refer to Rust's [platform support page][forge-platform-support] for more
information on Rust's tiered platform support.

Libraries
---------
- [`Vec::new` and `String::new` are now `const` functions.][64028]
- [`LinkedList::new` is now a `const` function.][63684]
- [`str::len`, `[T]::len` and `str::as_bytes` are now `const` functions.][63770]
- [The `abs`, `wrapping_abs`, and `overflowing_abs` numeric functions are
  now `const`.][63786]

Stabilized APIs
---------------
- [`Pin::into_inner`]
- [`Instant::checked_duration_since`]
- [`Instant::saturating_duration_since`]

Cargo
-----
- [You can now publish git dependencies if supplied with a `version`.]
  [cargo/7237]
- [The `--all` flag has been renamed to `--workspace`.][cargo/7241] Using
  `--all` is now deprecated.

Misc
----
- [You can now pass `-Clinker` to rustdoc to control the linker used
  for compiling doctests.][63834]

Compatibility Notes
-------------------
- [Code that was previously accepted by the old borrow checker, but rejected by
  the NLL borrow checker is now a hard error in Rust 2018.][63565] This was
  previously a warning, and will also become a hard error in the Rust 2015
  edition in the 1.40.0 release.
- [`rustdoc` now requires `rustc` to be installed and in the same directory to
  run tests.][63827] This should improve performance when running a large
  amount of doctests.
- [The `try!` macro will now issue a deprecation warning.][62672] It is
  recommended to use the `?` operator instead.
- [`asinh(-0.0)` now correctly returns `-0.0`.][63698] Previously this
  returned `0.0`.

[62600]: rust-lang/rust#62600
[62672]: rust-lang/rust#62672
[63118]: rust-lang/rust#63118
[63209]: rust-lang/rust#63209
[63402]: rust-lang/rust#63402
[63565]: rust-lang/rust#63565
[63595]: rust-lang/rust#63595
[63684]: rust-lang/rust#63684
[63698]: rust-lang/rust#63698
[63770]: rust-lang/rust#63770
[63786]: rust-lang/rust#63786
[63827]: rust-lang/rust#63827
[63834]: rust-lang/rust#63834
[63927]: rust-lang/rust#63927
[63933]: rust-lang/rust#63933
[63934]: rust-lang/rust#63934
[63938]: rust-lang/rust#63938
[63940]: rust-lang/rust#63940
[63941]: rust-lang/rust#63941
[63945]: rust-lang/rust#63945
[64010]: rust-lang/rust#64010
[64028]: rust-lang/rust#64028
[64334]: rust-lang/rust#64334
[cargo/7237]: rust-lang/cargo#7237
[cargo/7241]: rust-lang/cargo#7241
[cargo/7315]: rust-lang/cargo#7315
[`Pin::into_inner`]: https://doc.rust-lang.org/std/pin/struct.Pin.html#method.into_inner
[`Instant::checked_duration_since`]: https://doc.rust-lang.org/std/time/struct.Instant.html#method.checked_duration_since
[`Instant::saturating_duration_since`]: https://doc.rust-lang.org/std/time/struct.Instant.html#method.saturating_duration_since
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-async-await Area: Async & Await relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Stabilization] async/await MVP Tracking issue for async/await (RFC 2394)