From 00461fb5bdbb316cb0952edd957b2136763551b4 Mon Sep 17 00:00:00 2001
From: Jacob Hoffman-Andrews <github@hoffman-andrews.com>
Date: Mon, 22 Jun 2020 09:40:55 -0700
Subject: [PATCH] Only retry idempotent requests. (#80)

This also reverts a change to send_body that was originally added to
return the number of bytes written. It's no longer needed now that we
check the size of the reader in advance.

Fixes #76.
---
 src/body.rs         | 10 +++++-----
 src/request.rs      | 18 +++++++++++++++++-
 src/test/timeout.rs |  2 --
 src/unit.rs         | 23 ++++++++++++++++-------
 4 files changed, 38 insertions(+), 15 deletions(-)

diff --git a/src/body.rs b/src/body.rs
index 4d20a43..f87763e 100644
--- a/src/body.rs
+++ b/src/body.rs
@@ -166,12 +166,12 @@ pub(crate) fn send_body(
     mut body: SizedReader,
     do_chunk: bool,
     stream: &mut Stream,
-) -> IoResult<u64> {
-    let n = if do_chunk {
-        copy_chunked(&mut body.reader, stream)?
+) -> IoResult<()> {
+    if do_chunk {
+        copy_chunked(&mut body.reader, stream)?;
     } else {
-        copy(&mut body.reader, stream)?
+        copy(&mut body.reader, stream)?;
     };
 
-    Ok(n)
+    Ok(())
 }
diff --git a/src/request.rs b/src/request.rs
index 205850a..59e60b3 100644
--- a/src/request.rs
+++ b/src/request.rs
@@ -10,7 +10,7 @@ use url::{form_urlencoded, Url};
 use std::fmt;
 
 use crate::agent::{self, Agent, AgentState};
-use crate::body::Payload;
+use crate::body::{Payload, SizedReader};
 use crate::error::Error;
 use crate::header::{self, Header};
 use crate::pool;
@@ -605,6 +605,22 @@ impl Request {
         self.tls_config = Some(TLSClientConfig(tls_config));
         self
     }
+
+    // Returns true if this request, with the provided body, is retryable.
+    pub(crate) fn is_retryable(&self, body: &SizedReader) -> bool {
+        // Per https://tools.ietf.org/html/rfc7231#section-8.1.3
+        // these methods are idempotent.
+        let idempotent = match self.method.as_str() {
+            "DELETE" | "GET" | "HEAD" | "OPTIONS" | "PUT" | "TRACE" => true,
+            _ => false,
+        };
+        // Unsized bodies aren't retryable because we can't rewind the reader.
+        // Sized bodies are retryable only if they are zero-length because of
+        // coincidences of the current implementation - the function responsible
+        // for retries doesn't have a way to replay a Payload.
+        let no_body = body.size.is_none() || body.size.unwrap() > 0;
+        idempotent && no_body
+    }
 }
 
 #[cfg(feature = "tls")]
diff --git a/src/test/timeout.rs b/src/test/timeout.rs
index 1ee33e7..4e14df5 100644
--- a/src/test/timeout.rs
+++ b/src/test/timeout.rs
@@ -1,5 +1,3 @@
-
-use crate::test;
 use std::io::{self, BufRead, BufReader, Read, Write};
 use std::net::TcpStream;
 use std::thread;
diff --git a/src/unit.rs b/src/unit.rs
index 2cca994..cea0e05 100644
--- a/src/unit.rs
+++ b/src/unit.rs
@@ -155,21 +155,30 @@ pub(crate) fn connect(
             return Err(err.into());
         }
     }
+    let retryable = req.is_retryable(&body);
 
     // send the body (which can be empty now depending on redirects)
-    let body_bytes_sent = body::send_body(body, unit.is_chunked, &mut stream)?;
+    body::send_body(body, unit.is_chunked, &mut stream)?;
 
     // start reading the response to process cookies and redirects.
     let mut stream = stream::DeadlineStream::new(stream, unit.deadline);
     let mut resp = Response::from_read(&mut stream);
 
+    // https://tools.ietf.org/html/rfc7230#section-6.3.1
+    // When an inbound connection is closed prematurely, a client MAY
+    // open a new connection and automatically retransmit an aborted
+    // sequence of requests if all of those requests have idempotent
+    // methods.
+    //
+    // We choose to retry only once. To do that, we rely on is_recycled,
+    // the "one connection per hostname" police of the ConnectionPool,
+    // and the fact that connections with errors are dropped.
+    //
+    // 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() && 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.
+        if err.is_bad_status_read() && retryable && is_recycled {
             let empty = Payload::Empty.into_read();
             return connect(req, unit, false, redirect_count, empty, redir);
         }