feat(server): Accepting futures::Streams of connections, and turning the server into a future

[P-E-Meunier]

• The commit messages match the guidelines in https://github.com/hyperium/hyper/blob/master/CONTRIBUTING.md#git-commit-guidelines

This PR is maybe not meant to be accepted as such, but solves a number of problems for me:

• I can now run my own event loop, and configure my socket as I want before, without having to resort to low-level functions. This was already possible before, at the price of code duplication between Hyper and my crate. This solves issues Http::new(core: &'a Core) #1075 and Http::bind does not scale to uses where a Handle is needed #1263.

• I can now use the latest developments of rustls (for example their recent addition of SNI): since the server now takes an arbitrary stream of connections, one example use is to negotiate a TLS connection and passing it as a stream, like so:

let mut core = Core::new().unwrap();
let http = hyper::server::Http::new();
let listener = tokio_core::net::TcpListener::bind(addr, core.handle())
  .incoming().from_err()
  .and_then(move |(socket, addr)| {
      https_config
        .accept_async(socket)
        .map(move |socket| (socket, addr))
        .from_err()
  });
http.from_listener(listener, | | Ok(MyServer { … }).unwrap()
  .future_run(core.handle())
  .from_err()

Of course, it work fine on plain TcpListeners, and that's actually what Http::bind does.

Also, this PR does not change the existing API, except for:

• local_addr, which is now impossible to implement with the more general setting of arbitrary streams of connections instead of just Incoming.
• handle, but this is now useless: if you need a handle, you might as well run your own event loop.

[seanmonstar]

Hey there, thanks for starting this up! I do indeed want to add the ability to use any stream of connections, without having to use the tokio-proto stack, so this is definitely the right idea.

I recently outlined in #1322 that shows a proposed API for using Http/Server with a Handle, instead of owning the Core. Importantly, #1322 defines a way to keep backwards compatibility, which would be needed to merge this into 0.11. Would you like to adjust this PR to match what is proposed there?

Additionally, while we would eventually want to get rid of the local_addr method, since it doesn't belong in hyper, we can't remove it in 0.11. So, a SocketAddr would need to be provided with the from_listener method, so it can be set in the Server. We could deprecate Server::local_addr also (maybe now, maybe later) to hint that it would go away in 0.12.

[P-E-Meunier]

Hi! As I figured out with my second commit, the tests currently in master do not pass with my proposed change, so I guess we'll have to wait for 0.12.
This is not a giant problem for me, as long as 0.12 comes in the next few weeks (at least before the next big change in rustls/*ring*).

By the way, I like how the server code is structured. It's really short and clean.

[seanmonstar]

so I guess we'll have to wait for 0.12. This is not a giant problem for me, as long as 0.12 comes in the next few weeks.

Not likely, 0.12 will probably be a little while, there's still some upstream changes to figure out with futures and tokio.

However! We don't need to wait for 0.12. As I said, we can morph this into the API in #1322, and be able to release it in a 0.11.x release. What do you think of trying to do that?

[P-E-Meunier]

Sure. So since the API are necessarily different (some have their own Core, some don't), that would mean duplicating some code for a while, marking it as deprecated (as you said before, but not just for local_addr).

The core field of the struct could be an Option, right?

Another question I had was, why did you require anything to be Send + Sync? This seem a little odd for stuff supposed to be run in an event loop.

[seanmonstar]

So since the API are necessarily different (some have their own Core, some don't), that would mean duplicating some code for a while? The core field of the struct could be an Option, right?

I believe it can be done without duplicating too much code. For instance:

• Server.core could become Server.reactor, with an enum Reactor { Core(Core), Handle(Handle) }.
• Server.run/Server.run_until consume the Server, so the Core can be taken out, a Handle grabbed, and then the Server can be reconstructed the same, just with a Reactor::Handle(handle), and then because it implements Future, run can just call core.run(server).

Another question I had was, why did you require anything to be Send + Sync? This seem a little odd for stuff supposed to be run in an event loop.

This was because we hoped to soon move the internal Server::run thing to use tokio-proto's TcpServer, which can run on multiple threads. I'm not sure this is going to actually happen, but I'd worry about that in a different issue (in fact, there is #1275 that removes it, and I'll be looking at merging that PR soon too).

[kamalmarhubi]

An added benefit of this approach is making it possible to implement HTTP over unix domain sockets. Having TCP baked in as it is currently would make that rather annoying!

[seanmonstar]

Master has seen some updates, adding a Serve type that may eventually be the replacement of Server (perhaps it takes its name in 0.12). We should add a method to Http that can create a Serve from any stream of incoming connections, which should accomplish the rest of the goal of this PR. Frankly, was just stuck on naming...

Thank you so much for exploring this! This helped figure out that Server both owning a Core and possibly being a Future was awkward.

[sanmai-NL]

@kamalmarhubi: interesting, yet I personally have no idea how user clients could browse a website via Unix domain sockets. What would the purpose of such a feature be?

[P-E-Meunier]

Here's a really direct application of allowing arbitrary streams: tests! Maybe you don't can't open ports on you machine, and you want to run the protocol inside buffers (or on unix domain sockets if you want to benchmark what gets sent to the kernel).