HTTP retry support.

net/http/http_network_transaction.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/http/http_network_transaction.h"
 
 #include <set>
 #include <vector>
 
 #include "base/bind.h"
@@ skipping 57 matching lines @@
 #endif
 
 
 using base::Time;
 using base::TimeDelta;
 
 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,
     const HostPortPair& http_host_port_pair) {
   if (!headers.HasHeader(kAlternateProtocolHeader))
     return;
 
   std::vector<std::string> alternate_protocol_values;
   void* iter = NULL;
   std::string alternate_protocol_str;
@@ skipping 47 matching lines @@
                               base::Unretained(this))),
       session_(session),
       request_(NULL),
       priority_(priority),
       headers_valid_(false),
       logged_response_time_(false),
       fallback_error_code_(ERR_SSL_INAPPROPRIATE_FALLBACK),
       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) {
   session->ssl_config_service()->GetSSLConfig(&server_ssl_config_);
   session->GetNextProtos(&server_ssl_config_.next_protos);
   proxy_ssl_config_ = server_ssl_config_;
 }
 
 HttpNetworkTransaction::~HttpNetworkTransaction() {
   if (stream_.get()) {
@@ skipping 616 matching lines @@
 }
 
 int HttpNetworkTransaction::DoInitStream() {
   DCHECK(stream_.get());
   next_state_ = STATE_INIT_STREAM_COMPLETE;
   return stream_->InitializeStream(request_, priority_, net_log_, io_callback_);
 }
 
 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)
       result = HandleIOError(result);
 
     // The stream initialization failed, so this stream will never be useful.
     if (stream_)
         total_received_bytes_ += stream_->GetTotalReceivedBytes();
     stream_.reset();
   }
@@ skipping 202 matching lines @@
   // bizarre for SPDY. Assuming this logic is useful at all.
   // TODO(davidben): Bubble the error code up so we do not cache?
   if (result == ERR_CONNECTION_CLOSED && response_.headers.get())
     result = OK;
 
   if (result < 0)
     return HandleIOError(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.
   if (response_.headers.get() && response_.headers->response_code() == 408 &&
       stream_->IsConnectionReused()) {
     net_log_.AddEventWithNetErrorCode(
         NetLog::TYPE_HTTP_TRANSACTION_RESTART_AFTER_ERROR,
         response_.headers->response_code());
     // This will close the socket - it would be weird to try and reuse it, even
     // if the server doesn't actually close it.
@@ skipping 41 matching lines @@
 
   int rv = HandleAuthChallenge();
   if (rv != OK)
     return rv;
 
   if (is_https_request())
     stream_->GetSSLInfo(&response_.ssl_info);
 
   headers_valid_ = true;
 
+  if (retry_attempt_ && read_buf_.get())
+    next_state_ = STATE_READ_BODY;
+
   if (session_->huffman_aggregator()) {
     session_->huffman_aggregator()->AggregateTransactionCharacterCounts(
         *request_,
         request_headers_,
         proxy_info_.proxy_server(),
         *response_.headers);
   }
   return OK;
 }
 
 int HttpNetworkTransaction::DoReadBody() {
   DCHECK(read_buf_.get());
   DCHECK_GT(read_buf_len_, 0);
   DCHECK(stream_ != NULL);
 
   next_state_ = STATE_READ_BODY_COMPLETE;
   return stream_->ReadResponseBody(
       read_buf_.get(), read_buf_len_, io_callback_);
 }
 
 int HttpNetworkTransaction::DoReadBodyComplete(int result) {
   // We are done with the Read call.
   bool done = false;
   if (result <= 0) {
     DCHECK_NE(ERR_IO_PENDING, result);
+    if (result < 0)
+      return HandleIOError(result);
     done = true;
   }
 
   bool keep_alive = false;
   if (stream_->IsResponseBodyComplete()) {
     // Note: Just because IsResponseBodyComplete is true, we're not
     // necessarily "done".  We're only "done" when it is the last
     // read on this HttpNetworkTransaction, which will be signified
     // by a zero-length read.
     // TODO(mbelshe): The keepalive property is really a property of
@@ skipping 121 matching lines @@
                              base::TimeDelta::FromMinutes(10), 100);
 
   if (!stream_->IsConnectionReused()) {
     UMA_HISTOGRAM_CUSTOM_TIMES(
         "Net.Transaction_Latency_Total_New_Connection",
         total_duration, base::TimeDelta::FromMilliseconds(1),
         base::TimeDelta::FromMinutes(10), 100);
   }
 }
 
+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
   // during SSL renegotiation.
   //
   // In both cases, we want to close the connection before proceeding.
   // We do this for two reasons:
   //   First, we don't want to keep the connection to the server hung for a
   //   long time while the user selects a certificate.
   //   Second, even if we did keep the connection open, NSS has a bug where
@@ skipping 170 matching lines @@
     // socket is still connected, receiving the FIN, and sending/reading data
     // on a reused socket.  If we receive the FIN between the connectedness
     // check and writing/reading from the socket, we may first learn the socket
     // is disconnected when we get a ERR_SOCKET_NOT_CONNECTED.  This will most
     // likely happen when trying to retrieve its IP address.
     // See http://crbug.com/105824 for more details.
     case ERR_SOCKET_NOT_CONNECTED:
     // If a socket is closed on its initial request, HttpStreamParser returns
     // ERR_EMPTY_RESPONSE. This may still be close/reuse race if the socket was
     // preconnected but failed to be used before the server timed it out.
     case ERR_EMPTY_RESPONSE:

[mmenke, 2014/07/22 15:01:41]

Note that this is a conservative list of all "expected" errors - errors that we
could legitimately get when trying to reuse a socket, even if everything's just
fine and dandy.

Depending on the expected use case of retrying more aggressively, may well want
to expand the list.

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

Agreed, ERR_CONTENT_LENGTH_MISMATCH should probably be added to the list, at
least if we have received less than expected data.

       if (ShouldResendRequest()) {
         net_log_.AddEventWithNetErrorCode(
             NetLog::TYPE_HTTP_TRANSACTION_RESTART_AFTER_ERROR, error);
+        retry_attempt_++;
+        LogTransactionRetryMetrics(error);
         ResetConnectionAndRequestForResend();
         error = OK;
       }
       break;
     case ERR_SPDY_PING_FAILED:
     case ERR_SPDY_SERVER_REFUSED_STREAM:
     case ERR_QUIC_HANDSHAKE_FAILED:
       net_log_.AddEventWithNetErrorCode(
           NetLog::TYPE_HTTP_TRANSACTION_RESTART_AFTER_ERROR, error);
       ResetConnectionAndRequestForResend();
@@ skipping 21 matching lines @@
   request_headers_.Clear();
   response_ = HttpResponseInfo();
   establishing_tunnel_ = false;
 }
 
 HttpResponseHeaders* HttpNetworkTransaction::GetResponseHeaders() const {
   return response_.headers.get();
 }
 
 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;
+
+    // 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();
   }
-
-  // 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();
   next_state_ = STATE_CREATE_STREAM;  // Resend the request.
 }
 
 bool HttpNetworkTransaction::ShouldApplyProxyAuth() const {
   return !is_https_request() &&
       (proxy_info_.is_https() || proxy_info_.is_http());
 }
 
 bool HttpNetworkTransaction::ShouldApplyServerAuth() const {
   return !(request_->load_flags & LOAD_DO_NOT_SEND_AUTH_DATA);
@@ skipping 88 matching lines @@
       description = base::StringPrintf("Unknown state 0x%08X (%u)", state,
                                        state);
       break;
   }
   return description;
 }
 
 #undef STATE_CASE
 
 }  // namespace net