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

async/await #1654

Closed
seanmonstar opened this issue Sep 14, 2018 · 8 comments
Closed

async/await #1654

seanmonstar opened this issue Sep 14, 2018 · 8 comments
Labels
B-upstream Blocked: needs a change in a dependency or the compiler. C-feature Category: feature. This is adding a new feature.
Milestone

Comments

@seanmonstar
Copy link
Member

seanmonstar commented Sep 14, 2018

This is to track the status of being able to use async/await! both with and in hyper.

Relevant links:

@seanmonstar seanmonstar added B-upstream Blocked: needs a change in a dependency or the compiler. C-feature Category: feature. This is adding a new feature. labels Sep 14, 2018
@seanmonstar
Copy link
Member Author

Some concerns with simply using futures 0.3 directly in hyper:

  • Unstable (which is expected, but we can't suddenly make hyper nightly-only.)
  • New unsafe code should not be required in hyper.
  • Figuring out the Future vs TryFuture dance of bounds and impls.

@seanmonstar
Copy link
Member Author

I think this is a goal of the Futures (or wg-net?) team, as suggested by the 0.3 alpha announcement:

Tokio and Hyper support is not yet available for 0.3.0-alpha.1, but that’s the next area of work.

As of this writing, futures 0.3.0-alpha.1 is not integrated in any way with other libraries like Tokio or Hyper. Achieving such integration is a crucial part of getting real experience with the design and ultimately heading toward stabilization.

@Nemo157
Copy link

Nemo157 commented Sep 14, 2018

Here’s a WIP blog post on the compatibility layer available since futures-preview 0.3.0-alpha.3, it should be possible with this to use hyper in an async fn. In fact, rereading it now the final example is using hyper.

@seanmonstar
Copy link
Member Author

Experience Report

I started experimenting in a branch to see how it went, what follows is a report of my experience:

  • 🔴 Having async fn send_request(&self) -> Result<Response, Error> automatically make the future borrow &self is surprising. I wish the compiler could notice that I wasn't actually needing anything to be borrowed.
  • 🔴 Error messages are better than ever!
  • 🔴 Fixing the above issue by changing to fn send_request(&self) -> impl Future03<..> { async move { .. } } means lots of a functions that don't need to borrow any arguments have an extra level of indentation, just because.
  • 🔴 Many of those functions have several lines of copying config off self into local bindings at the top of the functions now. Feels gross, but it also clicked why if I think of async { similar to a closure.
  • 💚 It was neat to replace the RetryableSendReqest future into just a loop { match await!(fut) .. }.
  • 🔴 Tried changing Service::Future to use Future03, which lead to wanting to inject task::Context literally everywhere, which made me pause for now.

@seanmonstar
Copy link
Member Author

seanmonstar commented Sep 18, 2018

Report 2

  • 🔴 🔴 Seeing all the places that would need to be passed a task::Context argument, simply to then pass it along, is a lot of noise. Seeing as very few places actually need to grab the current waker, the previous task::current() was much nicer. (Grumbled about here).

  • Having switched Service to have a type Future: Future03<..> + Unpin;, it still means there's a paper cut when trying to use async/await! for a service. Consider:

    service_fn(|_| async {
        Ok(Response::new(BODY))
    })

    Since async is used, it uniformly has !Unpin on it, even though it isn't borrowing anything. So I'd need to use async { .. }.boxed() to get it to be Unpin. That requires importing Future03, and allocating on each request, which wasn't required with 0.1, and doesn't actually make it safer.

@aturon
Copy link
Contributor

aturon commented Sep 21, 2018

Thanks for checking this out @seanmonstar! Do you think you could push your branch somewhere? Some of this feedback is hard to understand without context.

When we chatted on Discord, we summarized these issues as:

  • The current async fn syntax makes some borrowing situations unergonomic to deal with
  • Because async fn/impl Trait cannot yet be used in traits, it's not possible for Hyper to completely move to the syntax yet (though existential types on nightly might be enough -- that still needs to be tried).
  • Because of the previous item, this conversion attempt still required a lot of manual futures, which means passing context arguments around.

More in-depth responses:

Having async fn send_request(&self) -> Result<Response, Error> automatically make the future borrow &self is surprising. I wish the compiler could notice that I wasn't actually needing anything to be borrowed.

Yep -- we talked about this some on Discord, but it's worth elaborating here. I think this is one of the core questions for the design of the async/await syntax, and I agree that the current setup has some pretty annoying downsides. Finalizing this syntax is one of the main blockers to stabilizing async/await.

I do want to note, however, that it's not an option to determine the borrowing structure by the body of the function -- it needs to be driven purely by the signature (with whatever elision rules we decide on).

I'll get a general issue started for discussing alternatives and ping here once that's ready.

Error messages are better than ever!

Is this related to futures 0.3? In general impl Future and async/await address this problem by making the involved types opaque. So presumably you're hitting this with some combinator-heavy code and would see similar things in 0.1?

Fixing the above issue by changing to fn send_request(&self) -> impl Future03<..> { async move { .. } } means lots of a functions that don't need to borrow any arguments have an extra level of indentation, just because.

Yep, the ergonomic "cliff" around different borrow modes here is suboptimal. This is directly connected to the first item, and is part of the design space there.

Many of those functions have several lines of copying config off self into local bindings at the top of the functions now. Feels gross, but it also clicked why if I think of async { similar to a closure.

This is one place where pointers to code would help -- it's hard to say much about this without context. In particular, it's worth trying to see whether borrowing in this context is possible.

But in general, yes async blocks are essentially unit-taking async closures.

Having switched Service to have a type Future: Future03<..> + Unpin;

This is another place where seeing the diff would be helpful.

@seanmonstar
Copy link
Member Author

There has been a new RFC to stabilize std::future, just linking here to keep track of things over time: rust-lang/rfcs#2592 (comment)

@seanmonstar
Copy link
Member Author

Closing as we don't really need to track the experience of using async/await, and #1805 tracks the update to std::future::Future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-upstream Blocked: needs a change in a dependency or the compiler. C-feature Category: feature. This is adding a new feature.
Projects
None yet
Development

No branches or pull requests

3 participants