From 753d61b4546a49257bdef12119ca4e1d02209aba Mon Sep 17 00:00:00 2001 From: Martin Algesten Date: Sun, 20 Oct 2019 21:32:19 +0200 Subject: [PATCH] Retry some pooled connections failing when server closes. Close #10 This is not a perfect solution. It works as long as we are not sending any body bytes. We discover the error first when attempting to read the response status line. That means we discover the error after sending body bytes. To be able to re-send the body, we would need to introduce a buffer to be able to replay the body on the next request. We don't currently do that. --- src/body.rs | 12 ++++++------ src/error.rs | 15 +++++++++++++++ src/response.rs | 5 ++++- src/test/agent_test.rs | 26 ++++++++++++++++++++++++++ src/unit.rs | 14 +++++++++++++- 5 files changed, 64 insertions(+), 8 deletions(-) diff --git a/src/body.rs b/src/body.rs index 02db9fef6..7d389a2f6 100644 --- a/src/body.rs +++ b/src/body.rs @@ -105,13 +105,13 @@ pub(crate) fn send_body( mut body: SizedReader, do_chunk: bool, stream: &mut Stream, -) -> IoResult<()> { - if do_chunk { +) -> IoResult { + let n = if do_chunk { let mut chunker = chunked_transfer::Encoder::new(stream); - copy(&mut body.reader, &mut chunker)?; + copy(&mut body.reader, &mut chunker)? } else { - copy(&mut body.reader, stream)?; - } + copy(&mut body.reader, stream)? + }; - Ok(()) + Ok(n) } diff --git a/src/error.rs b/src/error.rs index 1ec8bfda3..093491473 100644 --- a/src/error.rs +++ b/src/error.rs @@ -14,6 +14,9 @@ pub enum Error { ConnectionFailed(String), /// Too many redirects. Synthetic error `500`. TooManyRedirects, + /// We fail to read the status line. This happens for pooled connections when + /// TLS fails and we don't notice until trying to read. + BadStatusRead, /// A status line we don't understand `HTTP/1.1 200 OK`. Synthetic error `500`. BadStatus, /// A header line that couldn't be parsed. Synthetic error `500`. @@ -23,6 +26,15 @@ pub enum Error { } impl Error { + // If the error is bad status read, which might happen if a TLS connections is + // closed and we only discover it when trying to read the status line from it. + pub(crate) fn is_bad_status_read(&self) -> bool { + match self { + Error::BadStatusRead => true, + _ => false, + } + } + /// For synthetic responses, this is the error code. pub fn status(&self) -> u16 { match self { @@ -31,6 +43,7 @@ impl Error { Error::DnsFailed(_) => 400, Error::ConnectionFailed(_) => 500, Error::TooManyRedirects => 500, + Error::BadStatusRead => 500, Error::BadStatus => 500, Error::BadHeader => 500, Error::Io(_) => 500, @@ -45,6 +58,7 @@ impl Error { Error::DnsFailed(_) => "Dns Failed", Error::ConnectionFailed(_) => "Connection Failed", Error::TooManyRedirects => "Too Many Redirects", + Error::BadStatusRead => "Failed to read status line", Error::BadStatus => "Bad Status", Error::BadHeader => "Bad Header", Error::Io(_) => "Network Error", @@ -59,6 +73,7 @@ impl Error { Error::DnsFailed(err) => format!("Dns Failed: {}", err), Error::ConnectionFailed(err) => format!("Connection Failed: {}", err), Error::TooManyRedirects => "Too Many Redirects".to_string(), + Error::BadStatusRead => "Failed to read status line".to_string(), Error::BadStatus => "Bad Status".to_string(), Error::BadHeader => "Bad Header".to_string(), Error::Io(ioe) => format!("Network Error: {}", ioe), diff --git a/src/response.rs b/src/response.rs index 08f0f70bb..005552622 100644 --- a/src/response.rs +++ b/src/response.rs @@ -418,7 +418,10 @@ impl Response { fn do_from_read(mut reader: impl Read) -> Result { // // HTTP/1.1 200 OK\r\n - let status_line = read_next_line(&mut reader).map_err(|_| Error::BadStatus)?; + let status_line = read_next_line(&mut reader).map_err(|e| match e.kind() { + ErrorKind::ConnectionAborted => Error::BadStatusRead, + _ => Error::BadStatus, + })?; let (index, status) = parse_status_line(status_line.as_str())?; diff --git a/src/test/agent_test.rs b/src/test/agent_test.rs index 405f7c329..0982b159f 100644 --- a/src/test/agent_test.rs +++ b/src/test/agent_test.rs @@ -51,3 +51,29 @@ fn agent_cookies() { agent.get("test://host/agent_cookies").call(); } + +#[test] +fn connection_reuse() { + use std::io::Read; + use std::time::Duration; + + let agent = Agent::default().build(); + let resp = agent.get("https://fau.xxx/").call(); + + // use up the connection so it gets returned to the pool + assert_eq!(resp.status(), 200); + resp.into_reader().read_to_end(&mut vec![]).unwrap(); + + // wait for the server to close the connection. fau.xxx has a + // 2 second connection keep-alive. then it closes. + std::thread::sleep(Duration::from_secs(3)); + + // try and make a new request on the pool. this fails + // when we discover that the TLS connection is dead + // first when attempting to read from it. + let resp = agent.get("https://fau.xxx/").call(); + if let Some(err) = resp.synthetic_error() { + panic!("Pooled connection failed! {:?}", err); + } + assert_eq!(resp.status(), 200); +} diff --git a/src/unit.rs b/src/unit.rs index ee194aea3..e1a222f41 100644 --- a/src/unit.rs +++ b/src/unit.rs @@ -144,11 +144,23 @@ pub(crate) fn connect( } // send the body (which can be empty now depending on redirects) - body::send_body(body, unit.is_chunked, &mut stream)?; + let body_bytes_sent = body::send_body(body, unit.is_chunked, &mut stream)?; // start reading the response to process cookies and redirects. let mut resp = Response::from_read(&mut stream); + if let Some(err) = resp.synthetic_error() { + if err.is_bad_status_read() && body_bytes_sent == 0 && is_recycled { + // We try open a new connection, this happens if the remote server + // hangs a pooled connection and we only discover when trying to + // read from it. It's however only possible if we didn't send any + // body bytes. This is because we currently don't want to buffer + // any body to be able to replay it. + let empty = Payload::Empty.into_read(); + return connect(req, unit, false, redirect_count, empty, redir); + } + } + // squirrel away cookies if cfg!(feature = "cookies") { save_cookies(&unit, &resp);