Use the new tokio 0.1

[lschmierer]

Use the new simplified tokio.

https://tokio.rs/blog/2018-02-tokio-reform-shipped/

[seanmonstar]

Yes, I know of it, thanks! Switching to use it will be a breaking change, and since futures will release 0.2 soon, the change will be made together, in hyper 0.12.0. I'll tag this for that milestone.

[srijs]

I'm keen to attempt doing this! @seanmonstar, would you be open to accepting a PR against the 0.12.x branch?

[srijs]

FYI, I started spiking this out of curiosity, and opened tokio-rs/tokio#217 to follow up on a few unclarities.

[seanmonstar]

Awesome! Yea, if you want to take a stab at it, that'd be fantastic!

• I think some things we'd want to do is have "default" constructors, that just assume the global reactor, and constructors that take a Handle. So, for instance, there is Http::serve_addr_handle, and I think something like that would stay, and we'd also have Http::serve_addr.

• For places where the Handle is currently used as an executor, that will eventually be replaced by futures 0.2's Context::spawn.

• I wonder if it makes sense to change the HttpConnector to stop exposing the TcpStream directly, and instead expose an HttpStream, kind of like how in the server, there is AddrStream. I think this might allow for upgrade tokio-tcp versions without being a breaking change?

[srijs]

Okay, so far I have:

• replaced use of tokio-core by tokio (this will use the sub-crates as soon as Move tokio::net module into tokio tcp/udp crates tokio-rs/tokio#224 lands)
• introduced Http::serve_addr (using Handle::current)
• changed Server::run and Server::run_until to return a Run future rather than a Result
• use tokio_executor::spawn in places where Handle::spawn was used (this will move to task::Context::spawn with futures 0.2 as you mentioned)

If you think that's enough, I can open a PR against 0.12.x and we can hash out the details there, or otherwise I'm happy to make more changes.

[srijs]

Looking through my changes again, I'm wondering if we should change the API for Server to work better with tokio::run.

Currently w/o any major changes to the API, integrating hyper::Server with tokio::run looks something like this:

let http = Http::new();

tokio::run(lazy(move || {
    let mut server = http.bind(addr, || { ... }).unwrap();
    server.shutdown_timeout(Duration::from_secs(60));
    server.run().map_err(|err| println!("error: {}", err)
}))

I think it'd be neat if it looked something like this instead:

let http = Http::new();

let server = http.server()
    .with_shutdown_timeout(Duration::from_secs(60))
    .with_error_handler(|err| println!("error {}", err))
    .build(addr, || { ... });

tokio::run(server)

This would require a bunch of changes, effectively delaying all I/O until the server future gets polled (so that we're accessing the right task-local handle and executor), and invoking an error handler for all errors we encounter (since tokio::run expects the future to be infallible).

However maybe it makes sense to postpone this refactor until we have the basic changes on the 0.12.x branch, and then we can bikeshed on the API.

[aturon]

Note that futures 0.2 integration is now landing in tokio, so we'll want to coordinate work here with work in #1448.

To that end, getting a PR up and merged soon would be helpful, so that we can parallelize some of these API questions with the work on futures 0.2 integration.

[srijs]

Understood! Just polishing up a few things, then this should be ready for PR!

[seanmonstar]

To your question about the API: I do think things should probably change up a bit!

• The Http type was introduced to be a tokio_proto::ServerProto implementation. Now that it isn't, it might just be confusing. It's kind of a config builder, and also the starting point for both the lower-level connection API and higher-level server API.
• Some things started to appear, like Serve, which allowed a server-like interface that didn't require owning a Core. Now that Server doesn't need to own one, I wonder if Serve itself should go away...
• I also wonder if it'd be nice for Server to allow running entirely without importing tokio, if people just want the defaults...

[srijs]

Quick update here, I'm stuck on fixing up the integration tests, which has proven more difficult than I thought. Will try to push further tomorrow!

[seanmonstar]

This has been merged into the 0.12.x branch, here: 603c3e4

We can open new issues around specific API changes.