Cache failover to LOAD_PREFERRING_CACHE if network response suggests offline.

net/http/http_cache_transaction.cc

Index: net/http/http_cache_transaction.cc
diff --git a/net/http/http_cache_transaction.cc b/net/http/http_cache_transaction.cc
index 3c9953930bcac27131eb50894d2fec174f78b26f..5ed99e9a55ae56568b090573d57d33bba210e5bd 100644
--- a/net/http/http_cache_transaction.cc
+++ b/net/http/http_cache_transaction.cc
@@ -140,6 +140,7 @@ HttpCache::Transaction::Transaction(
       cache_pending_(false),
       done_reading_(false),
       vary_mismatch_(false),
+      couldnt_conditionalize_request_(false),
       io_buf_len_(0),
       read_offset_(0),
       effective_load_flags_(0),
@@ -810,6 +811,39 @@ int HttpCache::Transaction::DoSendRequestComplete(int result) {
   if (!cache_)
     return ERR_UNEXPECTED;
 
+  // If requested, and we have a readable cache entry, and we have
+  // an error indicating that we're offline as opposed to in contact
+  // with a bad server, read from cache anyway.
+  if (effective_load_flags_  & LOAD_CACHE_RETURN_IF_OFFLINE &&

[rvargas (doing something else), 2013/03/05 02:58:19]

nit: single space before &... and add () around this one for clarity

[Randy Smith (Not in Mondays), 2013/03/05 23:16:14]

Whoops; done.

+      mode_ == READ_WRITE && entry_ && !partial_ && result != OK) {

[rvargas (doing something else), 2013/03/05 02:58:19]

nit: move result != OK to the beginning of this line

[rvargas (doing something else), 2013/03/05 02:58:19]

it would be nice to make this work with byte ranges.

[Randy Smith (Not in Mondays), 2013/03/05 23:16:14]

Done.

[Randy Smith (Not in Mondays), 2013/03/05 23:16:14]

I'm happy to take a swing at that, but I'm not completely sure looking at the
code how it should be done.  According to the comments at PartialData
declaration, there can be many ranges specified by partial_.  I'm inclined to
think that the first offline network error (here) would result in switching over
to offline mode.  But it's not clear to me that we will have read in all the
cache entries associated with all the ranges described by partial_ at this
point, so we might need to loop more times through the state diagram to bring
everything in.  Is that about right?  Any more guidance as to how I should
implement this?

[rvargas (doing something else), 2013/03/06 03:11:48]

I don't think we need to do that on the first version. The general idea would be
that if the current logic says that the reason for revalidating a resource is
because of http rules (as opposed of being truncated or part of the range not
being available), then we can treat that as any other request and return cached
data when offline.

It is true that by the time we see a byte range request we should be already in
offline mode, but that concept (of switching for the rest of the page) is beyond
the scope of net code, so we should assume that seeking a video can be the first
signal to detect offline.

[Randy Smith (Not in Mondays), 2013/03/06 22:55:55]

Ok, sounds good.

+    switch (result) {

[rvargas (doing something else), 2013/03/05 02:58:19]

if (IsOffline(result)) ?

[Randy Smith (Not in Mondays), 2013/03/05 23:16:14]

Done.  Note that I took the opportunity to get rid of the result!=OK check
(since error == OK is not an offline result :-}); let me know if you find that
makes the code less understandable. 

+      // Errors that mean we didn't have a connection to the host.
+      // Go ahead and read from the cache anyway.
+      case ERR_NAME_NOT_RESOLVED:
+      case ERR_INTERNET_DISCONNECTED:
+      case ERR_ADDRESS_UNREACHABLE:
+      case ERR_CONNECTION_TIMED_OUT:
+        network_trans_.reset();
+        UpdateTransactionPattern(PATTERN_NOT_COVERED);
+
+        cache_->ConvertWriterToReader(entry_);

[rvargas (doing something else), 2013/03/05 02:58:19]

maybe extract this and 1800-1818 into a new method.

[Randy Smith (Not in Mondays), 2013/03/05 23:16:14]

Say more?  I don't follow the suggestion, probably because I don't grok the
cache architecture well.  BeginCacheValidation() was called noticeably earlier
in the state diagram (when we were deciding whether to send the network request
or not), so I'm not sure how we combine code between the two locations.  Could
you sketch out in more detail what you're thinking?

[rvargas (doing something else), 2013/03/06 03:11:48]

I just meant something like:

if (effective_flags...) {
  UpdateTxPattern(not_covered);
  was_override = true;
  return SetupEntryForRead();
}

int SetupEntryForRead() {
  network_trans_.reset();
  if (partial_.get()) {
      if (truncated_ || is_sparse_ || !invalid_range_) {
        // We are going to return the saved response headers to the caller, so
        // we may need to adjust them first.
        next_state_ = STATE_PARTIAL_HEADERS_RECEIVED;
        return OK;
      } else {
        partial_.reset();
      }
    }
    cache_->ConvertWriterToReader(entry_);
    mode_ = READ;

    if (entry_->disk_entry->GetDataSize(kMetadataIndex))
      next_state_ = STATE_CACHE_READ_METADATA;

    return OK;
}
...

if (skip_validation) {
  UpdateTxPattern(entry_used);
  return SetupEntryForRead();
}

[Randy Smith (Not in Mondays), 2013/03/06 22:55:55]

Ah, gotcha.  I'm not used to thinking of things this way, so I'm relying on you
and the tests to make sure I got it right.  Done.

+        mode_ = READ;
+        response_.was_cache_override = true;
+
+        // If there is metadata, read it.  Otherwise, we're done.
+        if (entry_->disk_entry->GetDataSize(kMetadataIndex))
+          next_state_ = STATE_CACHE_READ_METADATA;
+        return OK;
+      default:
+        break;
+    }
+  }
+
+  // If we tried to conditionalize the request and failed, we know
+  // we won't be reading from the cache after this point.
+  if (couldnt_conditionalize_request_)
+    mode_ = WRITE;
+
   if (result == OK) {
     next_state_ = STATE_SUCCESSFUL_SEND_REQUEST;
     return OK;
@@ -1772,6 +1806,10 @@ int HttpCache::Transaction::BeginCacheValidation() {
       } else {
         partial_.reset();
       }
+    } else {
+      // Override possible flag set in RequiresValidation(); we're
+      // validating and did not override from cache.
+      response_.was_cache_override = false;

[rvargas (doing something else), 2013/03/05 02:58:19]

remove? (see other comments)

[Randy Smith (Not in Mondays), 2013/03/05 23:16:14]

As noted in other comment, I don't yet follow.

     }
     cache_->ConvertWriterToReader(entry_);
     mode_ = READ;
@@ -1784,12 +1822,16 @@ int HttpCache::Transaction::BeginCacheValidation() {
     // Our mode remains READ_WRITE for a conditional request.  We'll switch to
     // either READ or WRITE mode once we hear back from the server.
     if (!ConditionalizeRequest()) {
+      couldnt_conditionalize_request_ = true;
       UpdateTransactionPattern(PATTERN_ENTRY_CANT_CONDITIONALIZE);
       if (partial_.get())
         return DoRestartPartialRequest();
 
       DCHECK_NE(206, response_.headers->response_code());
-      mode_ = WRITE;
+      // We don't want to switch mode to WRITE here as in offline

[rvargas (doing something else), 2013/03/05 02:58:19]

update the comment at 1822 instead of adding a new one here?

[Randy Smith (Not in Mondays), 2013/03/05 23:16:14]

Done.  Note that I changed the comment to only refer to switching mode to WRITE
because I don't see how the old code would have switched to READ, so you should
evaluate the new comment too.

+      // mode we may still need to read from the cache.  Instead
+      // we record and test on whether we could conditionalize the
+      // request.
     }
     next_state_ = STATE_SEND_REQUEST;
   }
@@ -1925,8 +1967,10 @@ bool HttpCache::Transaction::RequiresValidation() {
     return true;
   }
 
-  if (effective_load_flags_ & LOAD_PREFERRING_CACHE)
+  if (effective_load_flags_ & LOAD_PREFERRING_CACHE) {
+    response_.was_cache_override = true;

[rvargas (doing something else), 2013/03/05 02:58:19]

Feels wrong to do this, especially on this method.

[Randy Smith (Not in Mondays), 2013/03/05 23:16:14]

ETA: One of your later comments made it clear your opinion on the question I ask
below, so I've removed this code (option (a)).  I left my comment in for
context.

Fair.  I can see two alternatives: a) Don't set this flag for
LOAD_PREFERRING_CACHE; the caller can pretty much assume the value was from
cache in that case.  They'll be wrong sometimes, but not most of the time. 
*And* I don't currently see a use for that functionality, it just "felt right"
to me to set the output flag for LOAD_PREFERRING_CACHE if the cache was used. 
b) Re-plumb the logic so that was_cache_override is set at a higher level.  I'm
not sure the right way to do (b); maybe return another variable from this
function indicating that the cache override path was taken (so the function
stays non-side-effecting) and do the assignment at a higher level.  

Do you have a preference between those alternatives?

     return false;
+  }
 
   if (effective_load_flags_ & LOAD_VALIDATE_CACHE)
     return true;