-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
rfc: Promote low-level server API as the main API #2321
Comments
I don't use |
At a high level, I support making hyper simpler, lower-level. I think there's a lot of confusion with I think it'd be really helpful in this issue to define more exactly the plan. Some disjointed thoughts about the plan:
|
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)? |
@seanmonstar yeah, I think a 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 @djc Right, you can still use |
It would be nice to have a full example of setting up a server using only the requisite |
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...");
}
} |
@seanmonstar yeah I think that could totally work. |
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. |
`hyper` [will eventually depreciate](hyperium/hyper#2321) `hyper::Server` and `hyper::service::make_service_fn`, in favor of lower-level API constructs like `hyper::server::conn::Http`. This commit makes the example more idiomatic in that regard. Note that `hyper`'s future API is not yet stabilized, and that `hyper::server::conn::Http` itself will be replaced by something else, but that change should be very lightweight in comparison, i.e., ```rust let mut http = hyper::server::conn::Http::new(); http.http1_only(true); http.http1_keep_alive(true); ``` will eventually have to initialize an object that follows the new API; as of now, that would be ```rust let mut http = hyper::server::conn::http1::Builder::new() ```
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 callMakeService::make_service
with a&IO
whereIO
is someAsyncRead + AsyncWrite
. This then allows the user to basically await on a single future for the entire server. Accepting new connections and passing them to theMakeService
is all done on the root server future. This means that theServer
can continue to use a&mut MakeService
reference, callingpoll_ready
to apply back-pressure without the need for synchronization. Once a connection has been accepted andMakeService
returns the per-connectionM::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 bareTcpStream
/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 introducingTLS
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 callMakeService::make_service
with theTlsStream<TcpStream>
. This would allow users to extract the remote address and peer certs. By unfortunately this style is not really compatible with howMakeService::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 levelconn
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 ofpoll
fn and'static
futures.Examples
Simple one liners:
More complex with fetching the local addr from the bind:
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.This change would allow us to remove the need for
MakeService
and its confusing abstraction while still leveraging the more powerful aspects oftower
.Rename
Http
toServer
Now that we are nixing the
Server
type I'd like for us to renameHttp
intoServer
. This would allow us to treatServer
as a configuration that is easily clonable. It can then produce aServe
type viaServer::bind
. TheServe
type would handle the default implementation of the server and expose methods toserve
a serve or a fn viaserve_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
The text was updated successfully, but these errors were encountered: