fix(http1): add http10_disable_keep_alive() for server and client #3936
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
add http10_disable_keep_alive() for server and client to disable keep_alive connections for HTTP/1.0 requests and response
|
I don't expect to merge much more to 0.14.x in general... But even for 1.x, my initial feelings are that it's easier to do outside of hyper, simply with something like: if resp.version() == Version::HTTP_10 {
resp.headers.insert("connection", "close");
} |
@seanmonstar In certain cases you simply can’t force a connection to close outside of Hyper. For example, if you send an HTTP/1.1 keep-alive request but the server replies with HTTP/1.0 keep-alive, Hyper will put that connection back in the pool—and there’s no way to evict it, even if you add a I fully respect the back-porting policy. If upstream decides this patch isn’t suitable, we can leave it in our own branch. |
|
Ah, gotcha! Good point. For the 0.14 let captured = hyper::client::connect::capture_connection(&mut req);
let resp = client.request(req).await?;
if resp.version() == Version::HTTP_10 {
captured.connection_metadata().unwrap().poison();
}
For reference, the AWS SDK introduced this mechanism so they could evict connections based on custom criteria. |
|
Wow! Excellent! That's exactly what I am looking for! |
|
@seanmonstar I found something unexpected with poison(). Setupclient sent an HTTP1.0 request with "connection": "close", server responsed with HTTP1.1 response (connection header ommited, hyper treats it as keep-alive by default). I implemented a dumb server which intentionally NOT closing tcp connection even recv a "connection": "close" request. I observed that even with poison(), for the second request, conn evicted from pool, but conn stay connected for exactly 1min instead of closing immediately. It seems that with poison(), hyper relies on the idle checking task to regularly check for poisoned connections and reap them off, intead of respecting "connection": "close" header and close the socket once the response is fully received. If I use the patch in this PR, client closes the connection immediately every time as expected. POCsteps
Client (a proxy) // incomming req..
println!("request: {:?}", req);
if matches!(req.version(), Version::HTTP_10) {
// if http1.0 req, rewrite to connection: close
req.headers_mut().insert(CONNECTION, "close".parse().unwrap());
};
let req_version = req.version();
let result = client.request(req).await;
println!("response: {:?}", result);
result.map(|mut resp| {
if matches!(resp.version(), Version::HTTP_10) || matches!(req_version, Version::HTTP_10) {
// drop connection for http1.0
if captured.connection_metadata().as_ref().map(|conn| {
println!("poison connection");
conn.poison();
})
resp.headers_mut().insert(CONNECTION, "close".parse().unwrap());
};
resp
})Dumb server #[tokio::main]
async fn main() {
let listen: SocketAddr = "127.0.0.1:8084".parse().unwrap();
println!("server h11 {listen:?}");
let listener = TcpListener::bind(listen).await.unwrap();
loop {
let (mut stream, _) = listener.accept().await.unwrap();
println!("new stream");
tokio::spawn(async move {
let mut data = [0u8; 1024 * 32];
while let Ok(size) = stream.read(&mut data).await {
if size == 0 {
continue
}
let data = String::from_utf8_lossy(&data[..size]);
println!("[{}]", data);
if data.contains("HTTP/1.0") {
println!("HTTP/1.0 received, I can't handle it correctly");
stream.write_all(b"HTTP/1.1 400 Bad Request\r\nContent-Length: 0\r\n\r\n").await.unwrap();
} else {
stream.write_all(b"HTTP/1.1 200 OK\r\nconnection: keep-alive\r\nContent-Length: 0\r\n\r\n").await.unwrap();
}
}
});
}
}LOG |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
add http10_disable_keep_alive() for server and client to disable keep_alive connections for HTTP/1.0 requests and response
Server and Client can optionally disable HTTP/1.0 keepalive. If it's set, for http1.0 request and response, connection: keep-alive will be ignored and overridden with connection: close. Connections' state.keepalive is maintained accordingly.
@seanmonstar this is our vendored patch to fix following issues. Do you think if it is appropriate to merge into upstream?
fix #3588 and https://github.com/hyperium/hyper/security/advisories/GHSA-85g5-4q7m-6cgx