From 17d7e147eba520d4236c471bcca18b0d16d4b22a Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Tue, 29 Sep 2020 01:55:34 -0700 Subject: [PATCH] Handle ConnectionReset+ConnectionAbort at any time (#168) Previously we had a special case for BadStatusRead that would happen only when we got a ConnectionAborted error reading the status line. However, sometimes we get ConnectionReset instead. Also the HTTP spec says that idempotent requests may be retried anytime a connection is closed prematurely. The change treats as retryable any ConnectionAborted OR ConnectionReset error while reading the status line and headers. It removes the special case BadStatusRead error. Fixes #165 (I think). --- src/error.rs | 16 +++++----------- src/response.rs | 5 +---- src/stream.rs | 5 +++-- src/unit.rs | 6 +----- 4 files changed, 10 insertions(+), 22 deletions(-) diff --git a/src/error.rs b/src/error.rs index 704ff76..2c5a736 100644 --- a/src/error.rs +++ b/src/error.rs @@ -1,5 +1,5 @@ use std::fmt; -use std::io; +use std::io::{self, ErrorKind}; /// Errors that are translated to ["synthetic" responses](struct.Response.html#method.synthetic). #[derive(Debug)] @@ -14,9 +14,6 @@ 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`. @@ -37,11 +34,11 @@ 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 { + // Return true iff the error was due to a connection closing. + pub(crate) fn connection_closed(&self) -> bool { match self { - Error::BadStatusRead => true, + Error::Io(e) if e.kind() == ErrorKind::ConnectionAborted => true, + Error::Io(e) if e.kind() == ErrorKind::ConnectionReset => true, _ => false, } } @@ -54,7 +51,6 @@ impl Error { Error::DnsFailed(_) => 400, Error::ConnectionFailed(_) => 500, Error::TooManyRedirects => 500, - Error::BadStatusRead => 500, Error::BadStatus => 500, Error::BadHeader => 500, Error::Io(_) => 500, @@ -75,7 +71,6 @@ 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", @@ -96,7 +91,6 @@ 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 54fae37..7c16e35 100644 --- a/src/response.rs +++ b/src/response.rs @@ -476,10 +476,7 @@ 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(|e| match e.kind() { - ErrorKind::ConnectionAborted => Error::BadStatusRead, - _ => Error::Io(e), - })?; + let status_line = read_next_line(&mut reader)?; let (index, status) = parse_status_line(status_line.as_str())?; diff --git a/src/stream.rs b/src/stream.rs index ff0faee..8b89c23 100644 --- a/src/stream.rs +++ b/src/stream.rs @@ -368,7 +368,9 @@ pub(crate) fn connect_https(unit: &Unit, hostname: &str) -> Result Error::TlsError(err), - _ => Error::BadStatusRead, + // The only other possibility is WouldBlock. Since we don't + // handle retries of WouldBlock, turn it into a generic error. + _ => Error::ConnectionFailed("TLS handshake unexpected error".to_string()), })?; Ok(Stream::Https(BufReader::new(stream))) @@ -380,7 +382,6 @@ pub(crate) fn connect_host(unit: &Unit, hostname: &str, port: u16) -> Result format!("{}:{}", proxy.server, proxy.port), None => format!("{}:{}", hostname, port), diff --git a/src/unit.rs b/src/unit.rs index 53ce754..4a28a14 100644 --- a/src/unit.rs +++ b/src/unit.rs @@ -189,12 +189,8 @@ pub(crate) fn connect( // from the ConnectionPool, since those are most likely to have // reached a server-side timeout. Note that this means we may do // up to N+1 total tries, where N is max_idle_connections_per_host. - // - // TODO: is_bad_status_read is too narrow since it covers only the - // first line. It's also allowable to retry requests that hit a - // closed connection during the sending or receiving of headers. if let Some(err) = resp.synthetic_error() { - if err.is_bad_status_read() && retryable && is_recycled { + if err.connection_closed() && retryable && is_recycled { debug!("retrying request {} {}", method, url); let empty = Payload::Empty.into_read(); return connect(req, unit, false, redirect_count, empty, redir);