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..b3f7391a059421382eb600d37713d8e6c38e61a1 100644
--- a/net/http/http_network_transaction.cc
+++ b/net/http/http_network_transaction.cc
@@ -75,6 +75,8 @@ namespace net {
 
 namespace {
 
+const int kNumRetries = 3;
+
 void ProcessAlternateProtocol(
     HttpNetworkSession* session,
     const HttpResponseHeaders& headers,
@@ -142,6 +144,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 +783,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 +1007,21 @@ 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 +1083,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 +1111,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 +1254,21 @@ 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) {
+    UMA_HISTOGRAM_CUSTOM_ENUMERATION(
+    "Net.HttpRetry.Cause.MainFrame",
+    -error,
+    GetAllErrorCodesForUma());
+  } else {
+    UMA_HISTOGRAM_CUSTOM_ENUMERATION(
+    "Net.HttpRetry.Cause.Subresource",
+    -error,
+    GetAllErrorCodesForUma());
+  }
+}
+
 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 +1463,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 +1506,101 @@ HttpResponseHeaders* HttpNetworkTransaction::GetResponseHeaders() const {
 }
 
 bool HttpNetworkTransaction::ShouldResendRequest() const {
-  bool connection_is_proven = stream_->IsConnectionReused();
-  bool has_received_headers = GetResponseHeaders() != NULL;
+  if (retry_attempt_ > kNumRetries)
+    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/23 20:17:42]

If there a reason the old GetResponseHeaders() != NULL check doesn't work?

[jonnyr, 2014/07/25 13:41:59]

Done.

+    // Do not attempt a retry on anything but GET request if headers are valid.

[rvargas (doing something else), 2014/07/24 19:09:47]

How do we know that nothing actually reached the server when headers_valid_ is
false?

[jonnyr, 2014/07/25 13:41:59]

Right, this will have to be removed.

+    if (request_->method != "GET")
+      return false;
 
-void HttpNetworkTransaction::ResetConnectionAndRequestForResend() {
-  if (stream_.get()) {
-    stream_->Close(true);
-    stream_.reset();
+    TimeDelta time_delta = base::TimeTicks::Now() - send_start_time_;
+    if (time_delta.InSeconds() > 30)
+      return true;
+
+    // Do not retry automatically if resource is larger than 1 Mb until byte
+    // range support is enabled. This is temporary.
+    if (response_.headers->GetContentLength() > 1000000)

[rvargas (doing something else), 2014/07/24 19:09:47]

Seems like this should go before the 30 seconds rule.

[jonnyr, 2014/07/25 13:41:59]

Done.

+      return false;
+
+    // If etag or last-modified is set we can always retry since the response
+    // can be verified.
+    if (response_.headers->HasHeader("etag") ||

[rvargas (doing something else), 2014/07/24 19:09:47]

use HasStrongValidators()

[jonnyr, 2014/07/25 13:41:59]

Done.

+        response_.headers->HasHeader("Last-Modified")) {
+      return true;
+    }
+
+    if (response_.headers->HasHeaderValue("cache-control", "no-cache") ||

[rvargas (doing something else), 2014/07/24 19:09:47]

why fail if no-cache or must-revalidate?

[jonnyr, 2014/07/25 13:41:59]

What I am trying to avoid is that we glue together two responses 
that have been modified between retry attempts. I am using the 
cache headers as an indication that the resource could be modified 
in between two requests in an attempt to be as careful as possible.

no-cache is used to force intermediate caches to obtain a new copy 
from the origin server, and must-revalidate to force the client to 
revalidate with server if stale. I am thinking that developers use 
this to get the client to check with original content servers often 
since content is changed all the time.

An alternative to being this strict is to cross-check content, and 
save and match x number of bytes in the response before glueing 
together the result. Currently everything is hashed, but the 
intention is to remove the hashing once we see that we do not get 
too many ERR_RETRY_HASH_MISMATCH errors.

[rvargas (doing something else), 2014/07/26 00:30:33]

hmm... I don't think it is safe to remove the hash unless we have zero
ERR_RETRY_HASH_MISMATCH, at which point something is probably broken an not
reporting that error :)

Even if statistically speaking the error rate is insignificant, the fact that we
would be mixing responses seems unacceptable to me.

And if there is a hash in place, the only reason I could think of for being
extra conservative would be to try to avoid extra latency before actually
failing a request, but that should probably come out of an experiment (I mean,
we are assuming that the user is better served by waiting more and actually
seeing the page than a fast failure)

I think there's a lot of no-cache content that doesn't really change that often
but people still feel the need to force revalidations to avoid showing stale
content occasionally. (That is to say, choosing an expiration date is not an
easy thing).

+        response_.headers->HasHeaderValue("cache-control", "no-store") ||
+        response_.headers->HasHeaderValue("cache-control", "must-revalidate") ||
+        response_.headers->HasHeaderValue("pragma", "no-cache")) {
+      return false;
+    }
+
+    if (response_.headers->GetMaxAgeValue(&time_delta))
+      return time_delta.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.

[rvargas (doing something else), 2014/07/24 19:09:48]

why not? If it actually differs, we'll catch that by the hash.

[jonnyr, 2014/07/25 13:41:59]

The intention is to remove the hash.

+    Time date_value;
+    if (!response_.headers->GetDateValue(&date_value))
+      return false;
+
+    Time expires_value;
+    if (response_.headers->GetExpiresValue(&expires_value)) {
+      time_delta = expires_value - date_value;
+      if (time_delta.InSeconds() > 60)
+        return true;
+    }

[rvargas (doing something else), 2014/07/24 19:09:47]

Most of this logic should probably be moved to a method of HttpResponseHeaders.

[jonnyr, 2014/07/25 13:41:59]

That I can do. Do you think the overall strategy as mentioned above makes sense?

[rvargas (doing something else), 2014/07/26 00:30:33]

Yeah, I think that avoid merging by using a hash makes sense, and what I've seen
seems appropriate.

+    return false;
   }
+  return true;

[mmenke, 2014/07/23 20:17:42]

Out of paranoia, think we should keep the only retry non-GETs (And maybe
non-HEADs) if the connection was also reused.

[jonnyr, 2014/07/25 13:41:59]

Agreed.

+}
 
+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())

[rvargas (doing something else), 2014/07/24 19:09:47]

I suggest checking this one before last-modified and not adding both headers if
this one is present.

[jonnyr, 2014/07/25 13:41:59]

Done.

+        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.
 }