HttpCache::Transaction layer allowing parallel validation

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 5f870c6cdd56503b606817e410f4c0b9205bbb2c..a02a4ea8d5ed3a4d846aab57f2343b7c65b55f56 100644
--- a/net/http/http_cache_transaction.cc
+++ b/net/http/http_cache_transaction.cc
@@ -168,8 +168,10 @@ HttpCache::Transaction::Transaction(RequestPriority priority, HttpCache* cache)
       cache_(cache->GetWeakPtr()),
       entry_(NULL),
       new_entry_(NULL),
+      created_entry_(false),
       new_response_(NULL),
       mode_(NONE),
+      original_mode_(NONE),
       reading_(false),
       invalid_range_(false),
       truncated_(false),
@@ -182,6 +184,7 @@ HttpCache::Transaction::Transaction(RequestPriority priority, HttpCache* cache)
       couldnt_conditionalize_request_(false),
       bypass_lock_for_test_(false),
       fail_conditionalization_for_test_(false),
+      validating_cannot_proceed_(false),
       io_buf_len_(0),
       read_offset_(0),
       effective_load_flags_(0),
@@ -210,7 +213,11 @@ HttpCache::Transaction::~Transaction() {
 
   if (cache_) {
     if (entry_) {
-      bool cancel_request = reading_ && response_.headers.get();
+      // TODO(shivanisha): A lot of tests invoke Read on a HEAD request which
+      // should not be allowed in the Read() method. Fix those tests and then
+      // the check for method below need not exist.
+      bool cancel_request = reading_ && response_.headers.get() &&
+                            (request_ && request_->method != "HEAD");
       if (cancel_request) {
         if (partial_) {
           entry_->disk_entry->CancelSparseIO();
@@ -219,6 +226,12 @@ HttpCache::Transaction::~Transaction() {
         }
       }
 
+      // If this transaction created the entry and not yet started the response
+      // body phase, then it is a cancellation.
+      if (!cancel_request)

[jkarlin, 2017/03/23 18:37:38]

Looks like cancel_request can't be true if !reading_ at this point, so how
about:

cancel_request |= !reading && created_entry_ && (...) and removing the
conditional if (!cancel_request)?

[shivanisha, 2017/03/29 03:39:30]

N/A after the changes

+        cancel_request = !reading_ && created_entry_ &&

[jkarlin, 2017/03/23 18:37:38]

I find cancel_request hinging on whether we created an entry or not to be very
strange. Is there any way we can be more consistent about cance_requests meaning
instead of a specific subset of requests that canceled before finishing?

[shivanisha, 2017/03/29 03:39:30]

Removed cancel_request and changed the conditional. Either the transaction is a
current writer or its an auth/416 case where it is not added to the
done_headers_queue.

[shivanisha, 2017/03/29 03:41:42]

*Removed created_request ...

+                         (request_ && request_->method != "HEAD");
+
       cache_->DoneWithEntry(entry_, this, cancel_request);
     } else if (cache_pending_) {
       cache_->RemovePendingTransaction(this);
@@ -579,6 +592,11 @@ void HttpCache::Transaction::GetConnectionAttempts(
               old_connection_attempts_.end());
 }
 
+void HttpCache::Transaction::SetValidatingCannotProceed() {
+  validating_cannot_proceed_ = true;
+  entry_ = nullptr;
+}
+
 size_t HttpCache::Transaction::EstimateMemoryUsage() const {
   // TODO(xunjieli): Consider improving the coverage. crbug.com/669108.
   return 0;
@@ -602,7 +620,7 @@ size_t HttpCache::Transaction::EstimateMemoryUsage() const {
 //   Start():
 //   GetBackend* -> InitEntry -> OpenEntry* -> AddToEntry* -> CacheReadResponse*
 //   -> CacheDispatchValidation -> BeginPartialCacheValidation() ->
-//   BeginCacheValidation() -> SetupEntryForRead()
+//   BeginCacheValidation() -> SetupEntryForRead() -> WaitBeforeRead*
 //
 //   Read():
 //   CacheReadData*
@@ -614,7 +632,7 @@ size_t HttpCache::Transaction::EstimateMemoryUsage() const {
 //   BeginCacheValidation() -> SendRequest* -> SuccessfulSendRequest ->
 //   UpdateCachedResponse -> CacheWriteUpdatedResponse* ->
 //   UpdateCachedResponseComplete -> OverwriteCachedResponse ->
-//   PartialHeadersReceived
+//   PartialHeadersReceived -> WaitBeforeRead*
 //
 //   Read():
 //   CacheReadData*
@@ -719,8 +737,9 @@ int HttpCache::Transaction::DoLoop(int result) {
   DCHECK(!in_do_loop_);
 
   int rv = result;
+  State state = next_state_;
   do {
-    State state = next_state_;
+    state = next_state_;
     next_state_ = STATE_UNSET;
     base::AutoReset<bool> scoped_in_do_loop(&in_do_loop_, true);
 
@@ -857,6 +876,13 @@ int HttpCache::Transaction::DoLoop(int result) {
       case STATE_CACHE_READ_METADATA_COMPLETE:
         rv = DoCacheReadMetadataComplete(rv);
         break;
+      case STATE_WAIT_BEFORE_READ:
+        DCHECK_EQ(OK, rv);
+        rv = DoWaitBeforeRead();
+        break;
+      case STATE_WAIT_BEFORE_READ_COMPLETE:
+        rv = DoWaitBeforeReadComplete(rv);
+        break;
       case STATE_NETWORK_READ:
         DCHECK_EQ(OK, rv);
         rv = DoNetworkRead();
@@ -893,8 +919,13 @@ int HttpCache::Transaction::DoLoop(int result) {
 
   } while (rv != ERR_IO_PENDING && next_state_ != STATE_NONE);
 
+  // Assert Start() state machine allowed exit state.
+  if (!reading_ && rv == OK) {
+    DCHECK(!entry_ || state == STATE_WAIT_BEFORE_READ_COMPLETE);
+  }
+
   if (rv != ERR_IO_PENDING && !callback_.is_null()) {
-    read_buf_ = NULL;  // Release the buffer before invoking the callback.
+    read_buf_ = nullptr;  // Release the buffer before invoking the callback.
     base::ResetAndReturn(&callback_).Run(rv);
   }
 
@@ -976,7 +1007,7 @@ int HttpCache::Transaction::DoGetBackendComplete(int result) {
 
   // This is only set if we have something to do with the response.
   range_requested_ = (partial_.get() != NULL);
-
+  original_mode_ = mode_;
   return OK;
 }
 
@@ -1091,6 +1122,7 @@ int HttpCache::Transaction::DoCreateEntryComplete(int result) {
   cache_pending_ = false;
   switch (result) {
     case OK:
+      created_entry_ = true;
       TransitionToState(STATE_ADD_TO_ENTRY);
       break;
 
@@ -1354,7 +1386,10 @@ int HttpCache::Transaction::DoCacheQueryDataComplete(int result) {
 // We may end up here multiple times for a given request.
 int HttpCache::Transaction::DoStartPartialCacheValidation() {
   if (mode_ == NONE) {
-    TransitionToState(STATE_NONE);
+    if (!reading_)
+      TransitionToState(STATE_WAIT_BEFORE_READ);
+    else
+      TransitionToState(STATE_NONE);
     return OK;
   }
 
@@ -1469,7 +1504,7 @@ int HttpCache::Transaction::DoSuccessfulSendRequest() {
       new_response->headers->response_code() == 407) {
     SetAuthResponse(*new_response);
     if (!reading_) {
-      TransitionToState(STATE_NONE);
+      TransitionToState(STATE_WAIT_BEFORE_READ);
       return OK;
     }
 
@@ -1532,7 +1567,7 @@ int HttpCache::Transaction::DoSuccessfulSendRequest() {
       int ret = cache_->DoomEntry(cache_key_, NULL);
       DCHECK_EQ(OK, ret);
     }
-    cache_->DoneWritingToEntry(entry_, true);
+    cache_->DoneWritingToEntry(entry_, true, this);
     entry_ = NULL;
     mode_ = NONE;
   }
@@ -1550,7 +1585,10 @@ int HttpCache::Transaction::DoSuccessfulSendRequest() {
       (request_->method == "GET" || request_->method == "POST")) {
     // If there is an active entry it may be destroyed with this transaction.
     SetResponse(*new_response_);
-    TransitionToState(STATE_NONE);
+    if (!reading_)

[jkarlin, 2017/03/23 18:37:38]

prefer ternay here and the other cases like this:

TransitionToState(reading_ ? STATE_NONE : STATE_WAIT_BEFORE_READ)

[shivanisha, 2017/03/29 03:39:30]

Done

+      TransitionToState(STATE_WAIT_BEFORE_READ);
+    else
+      TransitionToState(STATE_NONE);
     return OK;
   }
 
@@ -1566,6 +1604,21 @@ int HttpCache::Transaction::DoSuccessfulSendRequest() {
   }
 
   TransitionToState(STATE_OVERWRITE_CACHED_RESPONSE);
+
+  if (!entry_)
+    return OK;
+
+  // Invalidate any current entry with a successful response if this transaction
+  // cannot write to this entry.
+  if (new_response->headers->response_code() != 304 &&

[jkarlin, 2017/03/23 18:37:38]

How do we know this transaction can't write to this entry? The conditional isn't
obvious to me.

[shivanisha, 2017/03/29 03:39:30]

It cannot write to the entry because either a writer or readers exist for the
current entry and this validation is a no match case.

+      (entry_->writer || !entry_->readers.empty())) {
+    DCHECK_EQ(entry_->headers_transaction, this);
+    cache_->DoneResponseHeaders(entry_, this, false);
+    entry_ = nullptr;
+    mode_ = NONE;
+    return OK;
+  }
+
   return OK;
 }
 
@@ -1638,7 +1691,6 @@ int HttpCache::Transaction::DoUpdateCachedResponseComplete(int result) {
   } else if (entry_ && !handling_206_) {
     DCHECK_EQ(READ_WRITE, mode_);
     if (!partial_ || partial_->IsLastRange()) {
-      cache_->ConvertWriterToReader(entry_);
       mode_ = READ;
     }
     // We no longer need the network transaction, so destroy it.
@@ -1758,10 +1810,12 @@ int HttpCache::Transaction::DoPartialHeadersReceived() {
   new_response_ = NULL;
 
   if (!partial_) {
-    if (entry_ && entry_->disk_entry->GetDataSize(kMetadataIndex))
+    if (entry_ && entry_->disk_entry->GetDataSize(kMetadataIndex)) {
       TransitionToState(STATE_CACHE_READ_METADATA);
-    else
-      TransitionToState(STATE_NONE);
+    } else {
+      DCHECK(!reading_);
+      TransitionToState(STATE_WAIT_BEFORE_READ);
+    }
     return OK;
   }
 
@@ -1775,13 +1829,88 @@ int HttpCache::Transaction::DoPartialHeadersReceived() {
     // We are about to return the headers for a byte-range request to the user,
     // so let's fix them.
     partial_->FixResponseHeaders(response_.headers.get(), true);
-    TransitionToState(STATE_NONE);
+    TransitionToState(STATE_WAIT_BEFORE_READ);
   } else {
+    TransitionToState(STATE_WAIT_BEFORE_READ);
+  }
+  return OK;
+}
+
+int HttpCache::Transaction::DoWaitBeforeRead() {
+  if (!entry_) {
     TransitionToState(STATE_NONE);
+    return OK;
+  }
+
+  // A transaction comes here between the completion of headers phase and the
+  // start of response body phase, thus it should never be the active writer at
+  // this point.
+  DCHECK_NE(entry_->writer, this);
+
+  TransitionToState(STATE_WAIT_BEFORE_READ_COMPLETE);
+
+  // If it was an auth failure or 416, this transaction should continue to be
+  // headers_transaction till consumer takes an action, so no need to do
+  // anything now.
+  if (auth_response_.headers.get() ||
+      (new_response_ && new_response_->headers &&
+       new_response_->headers->response_code() == 416))
+    return OK;
+
+  // If there is no response body to be written or read, it does not need to
+  // wait.
+  if (request_->method == "HEAD")
+    return OK;
+
+  // Proceed only if |this| completed the headers phase. For read-only
+  // transactions since they do not have a headers phase, they would have
+  // already been added to entry_->readers, so do nothing.
+  if (entry_->headers_transaction == this)
+    return cache_->DoneResponseHeaders(entry_, this, true);
+
+  return OK;
+}
+
+int HttpCache::Transaction::DoWaitBeforeReadComplete(int rv) {
+  if (rv == ERR_CACHE_RACE) {
+    RestartAfterValidationStarted();
+    return OK;
+  }
+
+  TransitionToState(STATE_NONE);
+  if (rv != OK)
+    return rv;
+
+  // If successful then this must be either be the writer or a reader as the
+  // response must have completely been written to the cache.
+  if (entry_->writer == this)
+    return OK;
+
+  // If it was an auth failure or 416, this transaction should continue to be
+  // headers_transaction, so no need to do anything now.
+  if (auth_response_.headers.get() ||
+      (new_response_ && new_response_->headers &&
+       new_response_->headers->response_code() == 416)) {
+    DCHECK_EQ(entry_->headers_transaction, this);
+    return OK;
   }
+
+  mode_ = READ;
+  if (network_trans_)
+    ResetNetworkTransaction();
   return OK;
 }
 
+void HttpCache::Transaction::RestartAfterValidationStarted() {
+  DCHECK(!reading_);
+  next_state_ = STATE_INIT_ENTRY;
+  cache_entry_status_ = CacheEntryStatus::ENTRY_UNDEFINED;
+  entry_ = nullptr;
+  mode_ = original_mode_;
+  if (network_trans_)
+    network_trans_.reset();
+}
+
 int HttpCache::Transaction::DoCacheReadMetadata() {
   TRACE_EVENT0("io", "HttpCacheTransaction::DoCacheReadMetadata");
   DCHECK(entry_);
@@ -1804,7 +1933,11 @@ int HttpCache::Transaction::DoCacheReadMetadataComplete(int result) {
                                     result);
   if (result != response_.metadata->size())
     return OnCacheReadError(result, false);
-  TransitionToState(STATE_NONE);
+
+  if (!reading_)
+    TransitionToState(STATE_WAIT_BEFORE_READ);
+  else
+    TransitionToState(STATE_NONE);
   return OK;
 }
 
@@ -2121,6 +2254,8 @@ int HttpCache::Transaction::BeginCacheRead() {
 
   if (entry_->disk_entry->GetDataSize(kMetadataIndex))
     TransitionToState(STATE_CACHE_READ_METADATA);
+  else if (!reading_)
+    TransitionToState(STATE_WAIT_BEFORE_READ);
   else
     TransitionToState(STATE_NONE);
 
@@ -2635,7 +2770,10 @@ int HttpCache::Transaction::SetupEntryForRead() {
       partial_.reset();
     }
   }
-  cache_->ConvertWriterToReader(entry_);
+
+  // This might or might not be able to add it as an active reader based on
+  // whether this is the active writer or not. If not, it will be added to a
+  // wait queue in DoWaitBeforeRead.
   mode_ = READ;
 
   if (request_->method == "HEAD")
@@ -2643,6 +2781,8 @@ int HttpCache::Transaction::SetupEntryForRead() {
 
   if (entry_->disk_entry->GetDataSize(kMetadataIndex))
     TransitionToState(STATE_CACHE_READ_METADATA);
+  else if (!reading_)
+    TransitionToState(STATE_WAIT_BEFORE_READ);
   else
     TransitionToState(STATE_NONE);
   return OK;
@@ -2723,7 +2863,7 @@ void HttpCache::Transaction::DoneWritingToEntry(bool success) {
 
   RecordHistograms();
 
-  cache_->DoneWritingToEntry(entry_, success);
+  cache_->DoneWritingToEntry(entry_, success, this);
   entry_ = NULL;
   mode_ = NONE;  // switch to 'pass through' mode
 }
@@ -3080,6 +3220,13 @@ void HttpCache::Transaction::RecordHistograms() {
 }
 
 void HttpCache::Transaction::OnIOComplete(int result) {
+  // If its the Start state machine and it cannot proceed due to failure by
+  // another transaction in writing the response, restart this transaction.
+  if (!reading_ && validating_cannot_proceed_) {
+    validating_cannot_proceed_ = false;
+    RestartAfterValidationStarted();
+  }
+
   DoLoop(result);
 }