Update hyper to 1.x and integrate with hyper-util #232
Conversation
|
I marked this as ready for review but since I still didn't hear back regarding the future of the acceptor API, it isn't really "ready". If it were up to me, I'd completely remove the acceptor API, but I'm not the one to make that decision so I'd really prefer hearing from the maintainers. This PR is blocking a lot of upgrades to hyper 1.0 in the ecosystem, I'd really appreciate hearing back so this PR can be reviewed properly. |
|
Thanks for the PR. This is a substantial update and I'm not as familiar with the guts of hyper-rustls as I am with other parts of the Rustls ecosystem. Spinning up on the Hyper 1.0 API changes and the work in this branch will take time that I haven't been able to find yet. Your patience is appreciated.
Have you looked at any downstream consumers of this crate to see if they are using the acceptor API, and what they would need to do in place of it were it removed? |
Let me demonstrate what a hyper server looks like without TLS in 0.14 and 1.x so I can better explain the changes to the accept process. // hyper 0.14
use std::convert::Infallible;
use std::net::SocketAddr;
use hyper::{Body, Request, Response, Server};
use hyper::service::{make_service_fn, service_fn};
async fn handle(_req: Request<Body>) -> Result<Response<Body>, Infallible> {
Ok(Response::new(Body::from("Hello World")))
}
#[tokio::main]
async fn main() {
// Construct our SocketAddr to listen on...
let addr = SocketAddr::from(([127, 0, 0, 1], 3000));
// And a MakeService to handle each connection...
let make_service = make_service_fn(|_conn| async {
Ok::<_, Infallible>(service_fn(handle))
});
// Then bind and serve...
let server = Server::bind(&addr).serve(make_service);
// And run forever...
if let Err(e) = server.await {
eprintln!("server error: {}", e);
}
}And here's the hyper 1.x equivalent: // Hyper 1.x
use http_body_util::Full;
use hyper::body::{Bytes, Incoming};
use hyper::service::service_fn;
use hyper::{Request, Response};
use hyper_util::rt::{TokioExecutor, TokioIo};
use hyper_util::server::conn::auto;
use std::convert::Infallible;
use std::net::SocketAddr;
use tokio::net::TcpListener;
async fn handle(_req: Request<Incoming>) -> Result<Response<Full<Bytes>>, Infallible> {
Ok(Response::new(Full::from("Hello World")))
}
#[tokio::main]
async fn main() {
// Construct our SocketAddr to listen on...
let addr = SocketAddr::from(([127, 0, 0, 1], 3000));
// Bind to the address
let listener = TcpListener::bind(addr).await.unwrap();
// Serve incoming connections
loop {
let (stream, _) = listener.accept().await.unwrap();
// Note that we have to drive the connections ourselves and also take care of the accepting process
tokio::spawn(async move {
auto::Builder::new(TokioExecutor::new())
.serve_connection(TokioIo::new(stream), service_fn(handle))
.await
.unwrap();
});
}
}You'll notice that the accepting of connections is now no longer up to hyper's now-removed All that the hyper-rustls I hope that somewhat explains why I think this is pointless now. Any "new" implementation of |
|
@Gelbpunkt Want to say thank you for putting the PR together. I'm chiming in to confirm, I've got tons of code that is blocked on upgrading http=1.0 (and axum=0.7, etc) because this isn't merged, and I'm looking forward to seeing it happen. I'm not that familiar with the guts of |
Thank you, this was helpful 👍
I'm convinced. I think you should go ahead and remove it from this branch (and update the PR description). |
|
One issue that arises with removing the server example is that there's no longer a test that runs the client example with custom certificates. I assume the best way forward is to inline a server that does this into the tests? I'm thinking that it seems a bit weird that the server and the client were testing "each other" previously, since a misimplementation might go unnoticed if it's in hyper-rustls. Wouldn't it be better to test the client's custom certificate with a completely different server implementation (similar to how the server was tested with curl)? |
|
Looking good :-)
Could this issue and @Arnavion's feedback be addressed at the same time by adding an example server that doesn't use the now-removed
I think it's fair to think about this as follow-up work and not block the hyper update on improving the tests like this.
If you want to update the |
|
One note, tower-service 0.3 depends on http 0.2. This fact doesn't block this work at all, but it does indicate that, to drop the http 0.2 dependency, we need either an updated hyper-util that depends on hyper::Service rather than tower-service::Service (I'm guessing this is preferable, but I don't know if it's feasible), or an updated tower-service that depends on http=1.0. EDIT: Apologies. As pointed out below, http 0.2 is a dev-dependency of tower-service, thus not relevant. |
That is a dev-dependency and therefore not relevant for us. |
Gelbpunkt
changed the title
Should be done. Earlier CI was failing because of the missing required-features on the revamped server example, that should now hopefully also pass. If not, I'll have to add some |
This updates the entire dependency stack to hyper 1.x and http 1.x. The tokio-runtime feature was removed since it no longer serves a purpose and tokio is a hard dependency. Since hyper 1.x removed the high-level server API that supported customizing the accept process of the server via an Accept trait, the acceptor in hyper-rustls is removed. The server example however was updated to showcase how rustls can be integrated with the low-level hyper 1.x API. Signed-off-by: Jens Reidel <adrian@travitia.xyz>
Signed-off-by: Jens Reidel <adrian@travitia.xyz>
|
Thanks again @Gelbpunkt 🙇 |
|
Tracking a release with this work in #250 |
Since hyper 1.x leaves control of the accepting process to the user (explanation), I removed the acceptor API and server examples.
The tokio feature in hyper-util is required for the IO adapter traits, so the need for a tokio-runtime feature is gone (it was already a hard dependency before).