rfc: Promote low-level server API as the main API

[LucioFranco]

Context

Currently, the hyper server module has two major components. A high-level server API and a low level connection based API. The most common usages of hyper for its server come from the high-level abstraction, as it is mostly plug and play. That said, there is a major flaw with the high-level API.

Problem

The high-level API takes some MakeService<T, R> and will call MakeService::make_service with a &IO where IO is some AsyncRead + AsyncWrite. This then allows the user to basically await on a single future for the entire server. Accepting new connections and passing them to the MakeService is all done on the root server future. This means that the Server can continue to use a &mut MakeService reference, calling poll_ready to apply back-pressure without the need for synchronization. Once a connection has been accepted and MakeService returns the per-connection M::Service, hyper will then spawn a connection task that will then handle the connection from that point on. This processing happens off of the root accept task.

This works great when you just want a one liner server and it can support the occasional user that may want to inspect the IO type before processing the connection. For example, it is possible to pull the remote address from the &IO type and give each connection this context in the returned service. When just using a bare TcpStream/TcpListener this works great. This is due to the fact that all processing of the connection happens during the accept phase and not after. When introducing TLS we must continue to an additional per connection processing (handshake). This processing can take some time and thus if done during the accept phase, it may potentially lead to other connections stalling during their accept/connect phase. To solve this, one would need to process the TLS handshake on the connection task, see #2175 for more information on this specifically.

The problem comes up when you want to use the MakeService with each IO type without any synchronization. Ideally, we'd like to accept the tcp connection, then process the handshake in an async manner, then somehow call MakeService::make_service with the TlsStream<TcpStream>. This would allow users to extract the remote address and peer certs. By unfortunately this style is not really compatible with how MakeService::poll_ready is stateless.

Solutions

The solution I would like to propose is to continue to treat hyper as a low level http implementation. This would mean remove the need for the MakeService abstraction but instead promote the low level conn module as the main way to work with servers in hyper. This provides a few advantages, the API is simpler and easier to discover, people can work with async/await instead of poll fn and 'static futures.

Examples

Simple one liners:

serve_fn("0.0.0.0:0", |req| async move { response(req).await }).await;

// Or with a `tower::Service`
serve("0.0.0.0:0", svc).await;

More complex with fetching the local addr from the bind:

let http = Http::new()
	.bind(addr)
	.await
	.unwrap();

let local_addr = http.local_addr();

http.serve_fn(|req| //...).await;

// Or with a `tower::Service`
http.serve(svc).await;

This style, then would allow us in hyper-tls to expand and provide nice and easy abstractions to implement similar on-top of the selected TLS implementation.

A more complex example would be a manual implementation with the Http::service_connection API.

let listener = TcpListener::bind(addr).await?;

let http = Http::new();

let svc = MyHandlerService::new();

loop {
	let (conn, addr) = listener.accept().await?;

	let http = http.clone();
	tokio::spawn(async move {
		// Now we can accept TLS/do other fun things here!

		http.serve_connection(conn, svc.clone()).await.unwrap();
	});
}

This change would allow us to remove the need for MakeService and its confusing abstraction while still leveraging the more powerful aspects of tower.

Rename Http to Server

Now that we are nixing the Server type I'd like for us to rename Http into Server. This would allow us to treat Server as a configuration that is easily clonable. It can then produce a Serve type via Server::bind. The Serve type would handle the default implementation of the server and expose methods to serve a serve or a fn via serve_fn.

Other solutions

We could continue to keep our current implementation and use something like a FuturesUnordered + tokio::spawn but this to me feels like continuing down a hacky path instead of backing out and looking at the bigger picture.

cc @seanmonstar @hawkw @davidbarsky

[Geobert]

I don't use hyper directly (only through warp) so it's hard to give a feedback that I'm confident in. But after reading, the proposal seems sensible to me :)

[seanmonstar]

At a high level, I support making hyper simpler, lower-level. I think there's a lot of confusion with MakeService, and isn't required at hyper's level, but instead can be provided by some hyper-tower crate (or users can make similar abstractions with their own accept loops). So, I agree, let's make things simple!

I think it'd be really helpful in this issue to define more exactly the plan. Some disjointed thoughts about the plan:

• The type hyper::Server and hyper::server::conn::Http would merge? What would the stubs of the methods that remain be? (Besides the config methods, of course).
• Graceful shutdown is important. Maybe we provide a utility in hyper to slot in Connections? Or maybe that stuff can be moved to some imaginary hyper-util crate. But whatever the end result, it'd be good to know the plan.
• I believe we should keep the hyper::upgrade API, which is a much nicer way to deal with CONNECT and websockets and what-not.

[djc]

Judging from this post, this isn't entirely what I hoped it would be. What I'd like is to make the requirements on implementations a much clearer interface. This seems to still depend on closures. Can you explain the desired interface in terms of traits (or trait bounds)?

[LucioFranco]

@seanmonstar yeah, I think a hyper-tower or we could re-introduce tower-hyper. This will make the path to 1.0 easier as well.

As for the changes, I can stub out the API a bit more but basically expose a lot of the components but in a more composable manner than the monolithic Server type we have now.

@djc Right, you can still use Service we just provide an easy way to use a closure. What were you expecting? Would be nice to hear some other thoughts :)

[djc]

It would be nice to have a full example of setting up a server using only the requisite Service impls (and making sure there are no private trait definitions involved) in the documentation itself.

[seanmonstar]

I was thinking about how to separate the graceful shutdown type into a separate tool, and I think the API could look like this:

let listener = TcpListener::bind("0.0.0.0:0").await?;

let server = Server::new();

let graceful = GracefulShutdown::new();
let ctrl_c = tokio::signal::ctrl_c();

loop {
	tokio::select! {
		conn = listener.accept() => {
			let server = server.clone();
			let graceful.clone();
			task::spawn(async move {
				// maybe TLS accept here...

				let conn = server.serve_connection(conn);
				let conn = graceful.watch(conn);

				if let Err(e) = conn.await {
					eprintln!("http server conn error: {}", e);
				}
			});
		},

		_ = ctrl_c => {
			eprintln!("Ctrl-C received, starting shutdown");
			break;
		}
	}
}

tokio::select! {
	_ = graceful.shutdown() => {
		eprintln!("Gracefully shutdown!");
	},
	_ = tokio::time::sleep(10.seconds()) => {
		eprintln!("Waited 10 seconds for graceful shutdown, aborting...");
	}
}

[LucioFranco]

@seanmonstar yeah I think that could totally work.

[seanmonstar]

Punting this to next milestone, since it's not ready yet. An API outline would still be appreciated before the actual code/PR is done.