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

@@ -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),

src/response.rs

@@ -476,10 +476,7 @@ impl Response {
     fn do_from_read(mut reader: impl Read) -> Result<Response, Error> {
         //
         // 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())?;
 

src/stream.rs

@@ -368,7 +368,9 @@ pub(crate) fn connect_https(unit: &Unit, hostname: &str) -> Result<Stream, Error
         .connect(&hostname.trim_matches(|c| c == '[' || c == ']'), sock)
         .map_err(|e| match e {
             HandshakeError::Failure(err) => 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<Tcp
     } else {
         unit.deadline
     };
-  
     let netloc = match unit.req.proxy {
         Some(ref proxy) => format!("{}:{}", proxy.server, proxy.port),
         None => format!("{}:{}", hostname, port),

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);