HTTP retry support.

net/http/http_network_transaction.cc

Index: net/http/http_network_transaction.cc
diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc
index f4df073b4bba5ad905631653d2cba46820c0daa7..76122234df3a422323cafff2c9c68bf663a4c42d 100644
--- a/net/http/http_network_transaction.cc
+++ b/net/http/http_network_transaction.cc
@@ -75,6 +75,13 @@ namespace net {
 
 namespace {
 
+// Array of all network error codes.  Needed for histograms.

[mmenke, 2014/07/22 14:57:41]

Not needed.  See GetAllErrorCodesForUma (declared in net_errors.h)

[jonnyr, 2014/07/23 08:43:04]

Done.

+const int kAllNetErrorCodes[] = {
+#define NET_ERROR(label, value) - (value),
+#include "net/base/net_error_list.h"
+#undef NET_ERROR
+};
+
 void ProcessAlternateProtocol(
     HttpNetworkSession* session,
     const HttpResponseHeaders& headers,
@@ -142,6 +149,9 @@ HttpNetworkTransaction::HttpNetworkTransaction(RequestPriority priority,
       request_headers_(),
       read_buf_len_(0),
       total_received_bytes_(0),
+      retry_attempt_(0),
+      offset_(0),
+      previous_content_length_(0),
       next_state_(STATE_NONE),
       establishing_tunnel_(false),
       websocket_handshake_stream_base_create_helper_(NULL) {
@@ -778,6 +788,8 @@ int HttpNetworkTransaction::DoInitStream() {
 
 int HttpNetworkTransaction::DoInitStreamComplete(int result) {
   if (result == OK) {
+    if (offset_)
+      stream_->SetRestartInfo(offset_, hash_, sizeof(hash_));
     next_state_ = STATE_GENERATE_PROXY_AUTH_TOKEN;
   } else {
     if (result < 0)
@@ -1000,6 +1012,20 @@ int HttpNetworkTransaction::DoReadHeadersComplete(int result) {
 
   DCHECK(response_.headers.get());
 
+  if ((response_.headers->response_code() == 200 ||
+       response_.headers->response_code() == 206) && offset_) {
+    std::string etag, last_modified;
+    response_.headers->EnumerateHeader(NULL, "last-modified", &last_modified);
+    if (response_.headers->GetHttpVersion() >= HttpVersion(1, 1))
+       response_.headers->EnumerateHeader(NULL, "etag", &etag);
+
+    // Return an error if content was updated on a retry.
+    if ((previous_content_length_ != response_.headers->GetContentLength() &&
+         response_.headers->response_code() == 200) ||
+        previous_etag_ != etag || previous_last_modified_ != last_modified)
+      return HandleIOError(ERR_RETRY_CONTENT_UPDATED);
+  }
+
   // On a 408 response from the server ("Request Timeout") on a stale socket,
   // retry the request.
   // Headers can be NULL because of http://crbug.com/384554.
@@ -1061,6 +1087,9 @@ int HttpNetworkTransaction::DoReadHeadersComplete(int result) {
 
   headers_valid_ = true;
 
+  if (retry_attempt_ && read_buf_.get())
+    next_state_ = STATE_READ_BODY;
+
   if (session_->huffman_aggregator()) {
     session_->huffman_aggregator()->AggregateTransactionCharacterCounts(
         *request_,
@@ -1086,6 +1115,8 @@ int HttpNetworkTransaction::DoReadBodyComplete(int result) {
   bool done = false;
   if (result <= 0) {
     DCHECK_NE(ERR_IO_PENDING, result);
+    if (result < 0)
+      return HandleIOError(result);
     done = true;
   }
 
@@ -1227,6 +1258,22 @@ void HttpNetworkTransaction::LogTransactionMetrics() const {
   }
 }
 
+void HttpNetworkTransaction::LogTransactionRetryMetrics(int error) const {
+  UMA_HISTOGRAM_CUSTOM_COUNTS("Net.HttpRetry.Count", retry_attempt_, 1, 5, 5);
+  if (request_->load_flags & LOAD_MAIN_FRAME)

[mmenke, 2014/07/22 14:57:41]

Use braces when the body of an if takes more than one line.

[jonnyr, 2014/07/23 08:43:04]

Done.

+    UMA_HISTOGRAM_CUSTOM_ENUMERATION(
+    "Net.HttpRetry.Cause.MainFrame",
+    -error,
+    base::CustomHistogram::ArrayToCustomRanges(
+    kAllNetErrorCodes, arraysize(kAllNetErrorCodes)));

[mmenke, 2014/07/22 14:57:41]

Should indent lines that continue statements by 4 lines.  SHould be something
like:

UMA_HISTOGRAM_CUSTOM_ENUMERATION(
    "Net.HttpRetry.Cause.MainFrame",
    -error,
    base::CustomHistogram::ArrayToCustomRanges(
        kAllNetErrorCodes, arraysize(kAllNetErrorCodes)));

[jonnyr, 2014/07/23 08:43:04]

Done.

+  else
+    UMA_HISTOGRAM_CUSTOM_ENUMERATION(
+    "Net.HttpRetry.Cause.Subresource",
+    -error,
+    base::CustomHistogram::ArrayToCustomRanges(
+    kAllNetErrorCodes, arraysize(kAllNetErrorCodes)));
+}
+
 int HttpNetworkTransaction::HandleCertificateRequest(int error) {
   // There are two paths through which the server can request a certificate
   // from us.  The first is during the initial handshake, the second is
@@ -1421,6 +1468,8 @@ int HttpNetworkTransaction::HandleIOError(int error) {
       if (ShouldResendRequest()) {
         net_log_.AddEventWithNetErrorCode(
             NetLog::TYPE_HTTP_TRANSACTION_RESTART_AFTER_ERROR, error);
+        retry_attempt_++;
+        LogTransactionRetryMetrics(error);
         ResetConnectionAndRequestForResend();
         error = OK;
       }
@@ -1462,27 +1511,90 @@ HttpResponseHeaders* HttpNetworkTransaction::GetResponseHeaders() const {
 }
 
 bool HttpNetworkTransaction::ShouldResendRequest() const {
-  bool connection_is_proven = stream_->IsConnectionReused();
-  bool has_received_headers = GetResponseHeaders() != NULL;
+  if (request_->method != "GET" || retry_attempt_ > 3)

[mmenke, 2014/07/22 14:57:41]

The 3 should be a constant at the top of this file.

[mmenke, 2014/07/22 14:57:41]

Not retrying on POSTs when we received a connection error on a reused socket is
a bad idea.

Servers may timeout idle sockets at any time.  So if we ever want to be able to
reuse sockets for POSTs at all, we have to retry POSTs.

[jonnyr, 2014/07/23 08:43:04]

Done.

[jonnyr, 2014/07/23 08:43:04]

Done.

[jonnyr, 2014/07/23 08:43:04]

Done.

[jonnyr, 2014/07/23 08:43:04]

Done.

+    return false;
 
-  // NOTE: we resend a request only if we reused a keep-alive connection.
-  // This automatically prevents an infinite resend loop because we'll run
-  // out of the cached keep-alive connections eventually.
-  if (connection_is_proven && !has_received_headers)
-    return true;
-  return false;
-}
+  if (headers_valid_) {

[mmenke, 2014/07/22 14:57:41]

Your should have some sort of time check here.  If we get a timeout after
waiting for 30 seconds without receiving data, we probably don't want to retry,
unless perhaps it was a stale socket.

+    // Do not retry automatically if resource is larger than 1 Mb until byte
+    // range support is enabled.
+    if (response_.headers->GetContentLength() > 1000000)
+      return false;
 
-void HttpNetworkTransaction::ResetConnectionAndRequestForResend() {
-  if (stream_.get()) {
-    stream_->Close(true);
-    stream_.reset();
+    // If etag or last-modified is set we can always retry since the responce

[mmenke, 2014/07/22 14:57:41]

response

[jonnyr, 2014/07/23 08:43:04]

Done.

+    // can be verified.
+    if (response_.headers->HasHeader("etag") ||
+        response_.headers->HasHeader("Last-Modified"))
+      return true;

[mmenke, 2014/07/22 14:57:41]

Use braces when the condition of an if statement takes multiple times.

[jonnyr, 2014/07/23 08:43:04]

Done.

+
+    if (response_.headers->HasHeaderValue("cache-control", "no-cache") ||
+        response_.headers->HasHeaderValue("pragma", "no-cache"))
+      return false;

[mmenke, 2014/07/22 14:57:41]

Not a big fan of having all this cache logic duplicated here and in the caching
code.  Seems ugly and regression prone.  You're missing "no-store", for
instance.

[jonnyr, 2014/07/23 08:43:04]

I did not like it either. I also missed must-revalidate I guess.

But I can not reuse the cache logic directly, since the behavior here is
slightly different. Suggestions for how to handle it better would be
appreciated.

[mmenke, 2014/07/23 15:08:40]

I'm not at all familiar with caching semantics or the cache code.  Maybe rope in
rvargas?

+
+    TimeDelta max_age_value;
+    if (response_.headers->GetMaxAgeValue(&max_age_value))
+      return max_age_value.InSeconds() > 60;
+
+    // If there is no Date header, then assume that the server response was
+    // generated at the time when we received the response, hence do not retry.
+    Time date_value;
+    if (!response_.headers->GetDateValue(&date_value))
+      return false;
+
+    Time expires_value;
+    if (response_.headers->GetExpiresValue(&expires_value)) {
+      max_age_value = expires_value - date_value;
+      if (max_age_value.InSeconds() > 60)
+        return true;
+    }
+    return false;
   }
+  return true;
+}
 
+void HttpNetworkTransaction::ResetConnectionAndRequestForResend() {
   // We need to clear request_headers_ because it contains the real request
   // headers, but we may need to resend the CONNECT request first to recreate
   // the SSL tunnel.
   request_headers_.Clear();
+  headers_valid_ = false;
+
+  if (stream_) {
+    total_received_bytes_ += stream_->GetTotalReceivedBytes();
+    if (stream_->GetReceivedBodyLength() > offset_) {
+      offset_ = stream_->GetReceivedBodyLength();
+      stream_->GetHash(hash_, sizeof(hash_));
+    }
+  }
+
+  HttpResponseHeaders* headers = GetResponseHeaders();
+  if (offset_ && headers) {
+    previous_content_length_ = headers->GetContentLength();
+    headers->EnumerateHeader(NULL, "last-modified",
+                             &previous_last_modified_);
+    if (headers->GetHttpVersion() >= HttpVersion(1, 1))
+      headers->EnumerateHeader(NULL, "etag", &previous_etag_);
+
+    // Disable until we have statistics that show that retrying does not
+    // result in too many ERR_RETRY_HASH_MISMATCH.
+    if (0 && previous_content_length_ &&
+        headers->HasHeaderValue("Accept-Ranges", "bytes")) {
+      // Try resending using a range request if supported.
+      request_headers_.SetHeader(HttpRequestHeaders::kRange,
+          "bytes=" + base::Uint64ToString(offset_) + "-");
+      if (!previous_last_modified_.empty())
+        request_headers_.SetHeader("If-Unmodified-Since",
+            previous_last_modified_);
+      if (!previous_etag_.empty())
+        request_headers_.SetHeader("If-Match", previous_etag_);
+    }
+  }
+
+  response_ = HttpResponseInfo();
+  establishing_tunnel_ = false;
+  if (stream_.get()) {
+    stream_->Close(true);
+    stream_.reset();
+  }
   next_state_ = STATE_CREATE_STREAM;  // Resend the request.
 }