Hyper: Support accepting connections on task

[LucioFranco]

Currently, poll_accept gets called on the main tcp accept task but in certain cases we want to run another future on each accept where we perform a TLS handshake. Ideally, we'd like to perform this per-connection workload on the connection task.

Currently, the closet solution to this is to poll the tls handshake future from within the AsyncRead/AsyncWrite implementation on the IO type. This works well if you just want your connection to have TLS but does not work with the MakeService pattern where you might want to fetch peer certificates. This is because we only poll the io type after the MakeService is called.

The main goal here is to let users fetch the io type in the MakeService and extract any _post_ handshake information like peer certs.

Related issue https://github.com/hyperium/tonic/issues/325

Relevant code:

https://github.com/hyperium/hyper/blob/master/src/server/conn.rs#L726

cc @seanmonstar I am not sure exactly what the best solution is here but it would most likely be breaking.

[LucioFranco]

One idea here might be to drop the Accept trait for a MakeConnection version which would allow us to return a future that can do the accept work.

[seanmonstar]

Yea, in Linkerd2, to prevent 1 slow TLS handshake from holding up others, we used buffered_unordered().

I do like the idea of MakeConnection, but I think it has some other issues, which is that Service typically assumes multiple futures can be resolving at the same time, but something like TcpListener::accept() may not, because of lifetimes.

[seanmonstar]

Thinking a little more, I suppose the TcpListener case could be handled by basically accepting the TcpStream in poll_ready(), and just returning future::ok(socket) in call()... Is that crazy?

[LucioFranco]

I think that is valid but I do worry about it being much more complex than needed? Though the benefit is we are pushing more tower.

[ajpauwels]

Hey all, very interested in tackling this. I've been reviewing the warp and hyper code for the past couple of days and trying to wrap my head around the suggested fix. Are you saying we should modify the poll_ready and make_connection methods inside the MakeConnection implementation for services?

[LucioFranco]

@ajpauwels I don't think we have a clear solution yet. but basically we need some sort of way to execute some future on the connection task to do things like tls handshakes.

[fdubois1]

Hi all, maybe it is off-topic, but my question is about the tls and hyper server so maybe related with what I read in that thread. I'm using hyper 0.12. My code looks like this :

             let incoming = websocket_listener.incoming().and_then(move |conn| {
                ws_tls_acceptor
                    .accept(conn)
                    .map_err(|e| io::Error::new(io::ErrorKind::Other, e))
            });
            Either::B(hyper::Server::builder(incoming).serve(make_service_fn(                           remote_addr,
                move |stream: &tokio_rustls::server::TlsStream<tokio::net::tcp::TcpStream>| {                       )
                    let remote_addr = stream.get_ref().0.peer_addr().ok();                  })
                    let mut ws_serve = websocket_service.clone();           })),
                    service_fn(move |req| ws_serve.handle(req, remote_addr.clone()))    
                },  
            ))) 
            .map_err(closure)

The problem with that approach is that if the ws_tls_acceptor.accept(conn) fails, it will stop completely my server because the server is polling for a new connection as long as it doesn't receive an error. Of course, if a ws_tls_acceptor.accept(conn) fails, I just want to ignore that connection and wait for other connection.

We changed our code to create a stream that will do the accept first and handle the connection after with something like this :

let http = hyper::server::conn::Http::new();

let websocket_server = incoming.then(|conn_res| Ok(conn_res)).for_each(move |conn_res| {
        match conn_res {
            Ok((conn, remote_addr)) => {
                let mut ws_serve = websocket_service.clone();
                let srvc = service_fn(move |req| ws_serve.handle(req, remote_addr));
                websocket_service
                    .executor_handle
                    .spawn(http.serve_connection(conn, srvc).map_err(|_| ()));
            }
            Err(e) => {
                warn!("incoming connection encountered an error: {}", e);
            }
        }

Now the problem is that we use websocket and it fails when we do req.on_upgrade() with the error upgrade expected but low level API in use. From what I understood, hyper::server::conn::Http::new() is the low level api so we will not be able to handle ws easily with that.

My question is : How can I handle tls with hyper server ? Since we use ws/on_upgrade, we have to use a server and not the low level api connection. And to start a server, there is two choices : hyper::Server::builder(incoming) or hyper::Server::bind(addr). Am I right? We have issue if we do the tls in the incoming future and if I do only a bind, I can't do the tls after since I can't get the tcp stream. Any idea? Thanks a lot

[max-heller]

Any updates on this?

[LucioFranco]

@max-heller not yet, we will be tackling this soon.

[Geobert]

Any progress? I'm working on client auth and need to retrieve the client certificate for that :)

[LucioFranco]

Proposal for this has been posted here https://github.com/hyperium/hyper/issues/2321, feedback would be really appreciated :)

[seanmonstar]

The expected plan now is to do as proposed in #2321, removing the "higher-level" Accept stuff, and recommending that users just make their simple accept loops manually, which is far easier now with async/await. So I'll close this in favor of #2321.