Migrate Gotham to std Futures to support Async/Await

[kevinastone]

With the anticipated release of async-await in stable rust this week, I took an effort to migrate Gotham to run on std futures using the pre-release versions of the dependencies (tokio, hyper, etc).

NOTE: This doesn't attempt to introduce await or async fn into the codebase (there was a single unavoidable case due to an tokio::File::metadata API change). That is designed as a future activity.

This migration involved a few key efforts:

1. Convert from Futures 0.1 to Futures-preview 0.3 (mostly Future<Item=> to Future<Output=Result<>>).
2. Update dependencies to pre-release versions (tokio-0.2 and hyper-0.13). There's still other dependencies that are outstanding and blocking the full release.
3. Migrate Handler trait to a pinned box HandlerFuture.

This is a work-in-progress with a few blockers before this would be ready:

Gotham Dependencies:

• Update Futures from futures-preview to futures = 0.3 when the other dependencies (hyper, tokio, etc) update in concert.
• Update Tokio to 0.2 from alpha pre-releases
• Update Hyper to 0.13 from alpha pre-releases
• Update Tower-Service to 0.3 from alpha pre-releases. Hyper is migrating many of its traits to tower-service::Service and so is now a direct dependency.
• Released version of futures_rustls which is currently a branch of tokio-rustls ported to Futures-preview
• Released version of futures-tokio-compat or suggested tokio::compat library for bridging futures::AsyncRead and tokio::AsyncRead. See Use futures AsyncRead, AsyncWrite, ready! macro everywhere tokio-rs/tokio#1297
  and Compatibility with tokio? async-rs/async-std#54

Middleware Dependencies:

• Diesel - Requires updated release of tokio-threadpool
• JWT - Requires updated release of jsonwebtoken since it's dependency ring conflicts withs futures-rustls.

The following examples were not updated (and thus commented out in Cargo.toml) due to incompatible dependencies:

• hello_world_until is dependent on a compatible version of tokio-signal
• diesel is dependent on the Diesel middleware
• openssl is dependent on tokio-openssl
• websocket is dependent on tokio-tungstenite

[colinbankier]

Wow - thanks @kevinastone! This is a huge effort! This is definitely something that would have been inevitable soon but we hadn't managed to start yet. We'll see if we can divide and conquer the remaining items on your list 👍

[alsuren]

After applying this patch, will async fn be able to impl Handler directly, or will we need to write a normal function which contains an async block, and then box the resulting future for returning, like I have to do in #281 ?

[kevinastone]

Without async traits, I don't believe we can implement async fn handlers directly. I wrote a proc-macro in my own code (happy to upstream) to allow annotating your async fn to generate the boilerplate async-block/boxed future for you: https://github.com/kevinastone/httpbox/blob/master/gotham_async/src/lib.rs

Here's an example:
https://github.com/kevinastone/httpbox/blob/gotham-future/src/app/method/body.rs#L47-L53

[alsuren]

I have made a PoC of how I think we could make async fn ... impl Handler without any macros. I've published it as a PR against this branch: kevinastone#1 .

Unfortunately, it had to break support for the (State, T) convenience return type (due to E0119), so it needs a bit of an API design discussion if we want to go in that direction:

• If we want to support the convenience (State, T) return type, we might be able to do this with a new .to_simple() function in the router builder. We might even be able to add a .to_compat() function, which takes an old-style future.
• Alternatively, we could keep the existing .to() method, and Handler types using old-style futures, and make a new .to_async() function that accepts a new-style async fn.
  • This would make the upgrade a little bit less painful for our users. We can then deprecate .to() at our leisure.
• I haven't looked into how this would affect middleware.

[pksunkara]

I would vote for .to_async()

[pksunkara]

So, I started working on this branch to tick off items above the list, but I got stuck with the following:

error[E0277]: the trait bound `tokio_rustls::client::TlsStream<tokio::net::tcp::stream::TcpStream>: hyper::client::connect::Connection` is not satisfied
   --> gotham/src/tls/test.rs:170:5
    |
170 | /     pub fn client(&self) -> TestClient<Self, TestConnect> {
171 | |         self.client_with_address(SocketAddr::new(IpAddr::from([127, 0, 0, 1]), 10000))
172 | |     }
    | |_____^ the trait `hyper::client::connect::Connection` is not implemented for `tokio_rustls::client::TlsStream<tokio::net::tcp::stream::TcpStream>`
    | 
   ::: gotham/src/test.rs:83:1
    |
83  |   pub struct TestClient<TS: Server, C: Connect> {
    |   --------------------------------------------- required by `test::TestClient`
    |
    = note: required because of the requirements on the impl of `hyper::client::connect::sealed::Connect` for `tls::test::TestConnect`

I can't implement the above hyper trait for a tokio_rustls struct.

My branch is at: https://github.com/reign-rs/gotham/tree/await

[kevinastone]

It was a considerable amount of work to port to tokio=0.2 and hyper=0.13. Here's a commit with my WIP changes:

d6c11a6

It seems to run correctly, I just haven't got all the doctests to run successfully. I'd also like to split up the commits into something more manageable.

[colinbankier]

Thanks for the efforts here so far @kevinastone @pksunkara .
Since this is a BIG change, with breaking api changes, I'd certainly want @whitfin and @nyarly to weigh in with their thoughts.

[nyarly]

I'll queue up a review by EOW.

[kevinastone]

I pushed up one big commit for the final Tokio-0.2 and hyper-0.13 migration to unblock others. I haven't had the chance to do any desirable refactoring and investigate some of the test failures (or really, tests that never complete).

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@1446956). Click here to learn what that means.
The diff coverage is 78.7%.

@@            Coverage Diff            @@
##             master     #370   +/-   ##
=========================================
  Coverage          ?   79.51%           
=========================================
  Files             ?      105           
  Lines             ?     4964           
  Branches          ?        0           
=========================================
  Hits              ?     3947           
  Misses            ?     1017           
  Partials          ?        0

Impacted Files	Coverage Δ
gotham/src/router/builder/single.rs	95% <ø> (ø)
gotham/src/extractor/path.rs	66.66% <ø> (ø)
gotham/src/extractor/query_string.rs	66.66% <ø> (ø)
gotham/src/router/response/extender.rs	25% <ø> (ø)
gotham/src/test/request.rs	84.61% <ø> (ø)
examples/hello_world_tls/src/main.rs	37.03% <ø> (ø)
gotham/src/handler/error.rs	33.33% <ø> (ø)
gotham/src/middleware/logger/mod.rs	0% <0%> (ø)
gotham/src/middleware/security/mod.rs	0% <0%> (ø)
gotham/src/middleware/timer/mod.rs	0% <0%> (ø)
... and 37 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1446956...e79eae7. Read the comment docs.

[kevinastone]

The JWT middleware is still blocked on an updated version of jsonwebtoken due to a conflicting dependency on ring. It doesn't seem to be maintained: https://github.com/Keats/jsonwebtoken#help-wanted-for-v7

Tokio-openssl (required by "examples/openssl") has been parked and expected to be integrated into rust-openssl, but no timeline is available: tokio-rs/tokio-openssl#12

Tokio-tungstenite (required by "examples/websocket") has not been updated to tokio-0.2 yet: snapview/tokio-tungstenite#64. It's last commit is 6 months old, so it might be a while before a compatible update is available.

Besides these middleware/examples above, the rest of the outstanding items for this PR are complete.

[kevinastone]

@ping @nyarly and @whitfin for review.

[pksunkara]

I have reviewed it pretty extensively. I don't see any issues with the code.

[whitfin]

I haven't had time to review fully, but things seem good to me so far. I've left a few comments but they're just mainly for my benefit rather than any specific issues.

I'll try take another pass, when I have time to actually stare at it for a while, but if others give approval for merging I'm not going to stand in the way. Seems like great work!

[colinbankier]

Great work @kevinastone - thanks, this is a huge amount of effort!
I'm going to take the bold move of merging this in :)
Since it's necessarily such a large PR, if there are any omissions or improvements, we can deal with them in separate PRs. This way we can keep things integrating and get some wider testing on master before doing the next release.
It's a shame about the dependency issues - I'll create issues for those and we can work out what to do about them later.

[pksunkara]

Thanks for the merge. Now we need to get #281 done so we can properly use async/await.