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..2e5b149cd75e910cd97b2757a2bf75a3a470943a 100644
--- a/net/http/http_cache_transaction.cc
+++ b/net/http/http_cache_transaction.cc
@@ -170,6 +170,7 @@ HttpCache::Transaction::Transaction(RequestPriority priority, HttpCache* cache)
       new_entry_(NULL),
       new_response_(NULL),
       mode_(NONE),
+      original_mode_(NONE),
       reading_(false),
       invalid_range_(false),
       truncated_(false),
@@ -210,16 +211,12 @@ HttpCache::Transaction::~Transaction() {
 
   if (cache_) {
     if (entry_) {
-      bool cancel_request = reading_ && response_.headers.get();
-      if (cancel_request) {
-        if (partial_) {
-          entry_->disk_entry->CancelSparseIO();
-        } else {
-          cancel_request &= (response_.headers->response_code() == 200);
-        }
-      }
+      bool cancel_response = cache_->IsTransactionCurrentOrFutureWriter(
+          entry_, this, request_->method);
+      if (cancel_response && partial_)
+        entry_->disk_entry->CancelSparseIO();
 
-      cache_->DoneWithEntry(entry_, this, cancel_request);
+      cache_->DoneWithEntry(entry_, this, cancel_response);
     } else if (cache_pending_) {
       cache_->RemovePendingTransaction(this);
     }
@@ -579,6 +576,12 @@ void HttpCache::Transaction::GetConnectionAttempts(
               old_connection_attempts_.end());
 }
 
+void HttpCache::Transaction::SetValidatingCannotProceed() {
+  DCHECK(!reading_);
+  next_state_ = STATE_HEADERS_PHASE_CANNOT_PROCEED;
+  entry_ = nullptr;
+}
+
 size_t HttpCache::Transaction::EstimateMemoryUsage() const {
   // TODO(xunjieli): Consider improving the coverage. crbug.com/669108.
   return 0;
@@ -593,7 +596,7 @@ size_t HttpCache::Transaction::EstimateMemoryUsage() const {
 //   GetBackend* -> InitEntry -> OpenEntry* -> CreateEntry* -> AddToEntry* ->
 //   SendRequest* -> SuccessfulSendRequest -> OverwriteCachedResponse ->
 //   CacheWriteResponse* -> TruncateCachedData* -> TruncateCachedMetadata* ->
-//   PartialHeadersReceived
+//   PartialHeadersReceived -> FinishHeaders*
 //
 //   Read():
 //   NetworkRead* -> CacheWriteData*
@@ -602,7 +605,7 @@ size_t HttpCache::Transaction::EstimateMemoryUsage() const {
 //   Start():
 //   GetBackend* -> InitEntry -> OpenEntry* -> AddToEntry* -> CacheReadResponse*
 //   -> CacheDispatchValidation -> BeginPartialCacheValidation() ->
-//   BeginCacheValidation() -> SetupEntryForRead()
+//   BeginCacheValidation() -> SetupEntryForRead() -> FinishHeaders*
 //
 //   Read():
 //   CacheReadData*
@@ -614,7 +617,7 @@ size_t HttpCache::Transaction::EstimateMemoryUsage() const {
 //   BeginCacheValidation() -> SendRequest* -> SuccessfulSendRequest ->
 //   UpdateCachedResponse -> CacheWriteUpdatedResponse* ->
 //   UpdateCachedResponseComplete -> OverwriteCachedResponse ->
-//   PartialHeadersReceived
+//   PartialHeadersReceived -> FinishHeaders*
 //
 //   Read():
 //   CacheReadData*
@@ -625,7 +628,7 @@ size_t HttpCache::Transaction::EstimateMemoryUsage() const {
 //   -> CacheDispatchValidation -> BeginPartialCacheValidation() ->
 //   BeginCacheValidation() -> SendRequest* -> SuccessfulSendRequest ->
 //   OverwriteCachedResponse -> CacheWriteResponse* -> DoTruncateCachedData* ->
-//   TruncateCachedMetadata* -> PartialHeadersReceived
+//   TruncateCachedMetadata* -> PartialHeadersReceived -> FinishHeaders*
 //
 //   Read():
 //   NetworkRead* -> CacheWriteData*
@@ -639,7 +642,7 @@ size_t HttpCache::Transaction::EstimateMemoryUsage() const {
 //   BeginCacheValidation() -> SendRequest* -> SuccessfulSendRequest ->
 //   UpdateCachedResponse -> CacheWriteUpdatedResponse* ->
 //   UpdateCachedResponseComplete -> OverwriteCachedResponse ->
-//   PartialHeadersReceived
+//   PartialHeadersReceived -> FinishHeaders*
 //
 //   Read() 1:
 //   NetworkRead* -> CacheWriteData*
@@ -680,7 +683,7 @@ size_t HttpCache::Transaction::EstimateMemoryUsage() const {
 //   GetBackend* -> InitEntry -> OpenEntry* -> AddToEntry* -> CacheReadResponse*
 //   -> CacheDispatchValidation -> BeginPartialCacheValidation() ->
 //   BeginCacheValidation() -> SendRequest* -> SuccessfulSendRequest ->
-//   OverwriteCachedResponse
+//   OverwriteCachedResponse -> FinishHeaders*
 //
 // 10. HEAD. Sparse entry, partially cached:
 //   Serve the request from the cache, as long as it doesn't require
@@ -705,7 +708,7 @@ size_t HttpCache::Transaction::EstimateMemoryUsage() const {
 //   GetBackend* -> InitEntry -> OpenEntry* -> AddToEntry* -> CacheReadResponse*
 //   -> CacheToggleUnusedSincePrefetch* -> CacheDispatchValidation ->
 //   BeginPartialCacheValidation() -> BeginCacheValidation() ->
-//   SetupEntryForRead()
+//   SetupEntryForRead() -> FinishHeaders*
 //
 //   Read():
 //   CacheReadData*
@@ -719,8 +722,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 +861,15 @@ int HttpCache::Transaction::DoLoop(int result) {
       case STATE_CACHE_READ_METADATA_COMPLETE:
         rv = DoCacheReadMetadataComplete(rv);
         break;
+      case STATE_HEADERS_PHASE_CANNOT_PROCEED:
+        rv = DoHeadersPhaseCannotProceed();
+        break;
+      case STATE_FINISH_HEADERS:
+        rv = DoFinishHeaders(rv);
+        break;
+      case STATE_FINISH_HEADERS_COMPLETE:
+        rv = DoFinishHeadersComplete(rv);
+        break;
       case STATE_NETWORK_READ:
         DCHECK_EQ(OK, rv);
         rv = DoNetworkRead();
@@ -893,8 +906,13 @@ int HttpCache::Transaction::DoLoop(int result) {
 
   } while (rv != ERR_IO_PENDING && next_state_ != STATE_NONE);
 
+  // Assert Start() state machine's allowed last state in successful cases when
+  // caching is happening.
+  DCHECK(reading_ || rv != OK || !entry_ ||
+         state == STATE_FINISH_HEADERS_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);
   }
 
@@ -921,7 +939,7 @@ int HttpCache::Transaction::DoGetBackendComplete(int result) {
     if (effective_load_flags_ & LOAD_ONLY_FROM_CACHE) {
       if (effective_load_flags_ & LOAD_BYPASS_CACHE) {
         // The client has asked for nonsense.
-        TransitionToState(STATE_NONE);
+        TransitionToState(STATE_FINISH_HEADERS);
         return ERR_CACHE_MISS;
       }
       mode_ = READ;
@@ -960,7 +978,7 @@ int HttpCache::Transaction::DoGetBackendComplete(int result) {
   // If must use cache, then we must fail.  This can happen for back/forward
   // navigations to a page generated via a form post.
   if (!(mode_ & READ) && effective_load_flags_ & LOAD_ONLY_FROM_CACHE) {
-    TransitionToState(STATE_NONE);
+    TransitionToState(STATE_FINISH_HEADERS);
     return ERR_CACHE_MISS;
   }
 
@@ -977,6 +995,9 @@ int HttpCache::Transaction::DoGetBackendComplete(int result) {
   // This is only set if we have something to do with the response.
   range_requested_ = (partial_.get() != NULL);
 
+  // mode_ may change later, save the initial mode in case we need to restart
+  // this request.
+  original_mode_ = mode_;
   return OK;
 }
 
@@ -985,7 +1006,7 @@ int HttpCache::Transaction::DoInitEntry() {
   DCHECK(!new_entry_);
 
   if (!cache_.get()) {
-    TransitionToState(STATE_NONE);
+    TransitionToState(STATE_FINISH_HEADERS);
     return ERR_UNEXPECTED;
   }
 
@@ -1022,7 +1043,7 @@ int HttpCache::Transaction::DoOpenEntryComplete(int result) {
   }
 
   if (result == ERR_CACHE_RACE) {
-    TransitionToState(STATE_INIT_ENTRY);
+    TransitionToState(STATE_HEADERS_PHASE_CANNOT_PROCEED);
     return OK;
   }
 
@@ -1048,7 +1069,7 @@ int HttpCache::Transaction::DoOpenEntryComplete(int result) {
 
   // The entry does not exist, and we are not permitted to create a new entry,
   // so we must fail.
-  TransitionToState(STATE_NONE);
+  TransitionToState(STATE_FINISH_HEADERS);
   return ERR_CACHE_MISS;
 }
 
@@ -1067,8 +1088,9 @@ int HttpCache::Transaction::DoDoomEntryComplete(int result) {
   net_log_.EndEventWithNetErrorCode(NetLogEventType::HTTP_CACHE_DOOM_ENTRY,
                                     result);
   cache_pending_ = false;
-  TransitionToState(result == ERR_CACHE_RACE ? STATE_INIT_ENTRY
-                                             : STATE_CREATE_ENTRY);
+  TransitionToState(result == ERR_CACHE_RACE
+                        ? STATE_HEADERS_PHASE_CANNOT_PROCEED
+                        : STATE_CREATE_ENTRY);
   return OK;
 }
 
@@ -1095,7 +1117,7 @@ int HttpCache::Transaction::DoCreateEntryComplete(int result) {
       break;
 
     case ERR_CACHE_RACE:
-      TransitionToState(STATE_INIT_ENTRY);
+      TransitionToState(STATE_HEADERS_PHASE_CANNOT_PROCEED);
       break;
 
     default:
@@ -1176,13 +1198,13 @@ int HttpCache::Transaction::DoAddToEntryComplete(int result) {
   new_entry_ = NULL;
 
   if (result == ERR_CACHE_RACE) {
-    TransitionToState(STATE_INIT_ENTRY);
+    TransitionToState(STATE_HEADERS_PHASE_CANNOT_PROCEED);
     return OK;
   }
 
   if (result == ERR_CACHE_LOCK_TIMEOUT) {
     if (mode_ == READ) {
-      TransitionToState(STATE_NONE);
+      TransitionToState(STATE_FINISH_HEADERS);
       return ERR_CACHE_MISS;
     }
 
@@ -1201,7 +1223,7 @@ int HttpCache::Transaction::DoAddToEntryComplete(int result) {
   // TODO(jkarlin): We should either handle the case or DCHECK.
   if (result != OK) {
     NOTREACHED();
-    TransitionToState(STATE_NONE);
+    TransitionToState(STATE_FINISH_HEADERS);
     return result;
   }
 
@@ -1344,7 +1366,7 @@ int HttpCache::Transaction::DoCacheQueryData() {
 int HttpCache::Transaction::DoCacheQueryDataComplete(int result) {
   DCHECK_EQ(OK, result);
   if (!cache_.get()) {
-    TransitionToState(STATE_NONE);
+    TransitionToState(STATE_FINISH_HEADERS);
     return ERR_UNEXPECTED;
   }
 
@@ -1354,7 +1376,7 @@ 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);
+    TransitionToState(STATE_FINISH_HEADERS);
     return OK;
   }
 
@@ -1371,12 +1393,12 @@ int HttpCache::Transaction::DoCompletePartialCacheValidation(int result) {
       cache_->DoneReadingFromEntry(entry_, this);
       entry_ = NULL;
     }
-    TransitionToState(STATE_NONE);
+    TransitionToState(STATE_FINISH_HEADERS);
     return result;
   }
 
   if (result < 0) {
-    TransitionToState(STATE_NONE);
+    TransitionToState(STATE_FINISH_HEADERS);
     return result;
   }
 
@@ -1402,7 +1424,7 @@ int HttpCache::Transaction::DoSendRequest() {
   int rv =
       cache_->network_layer_->CreateTransaction(priority_, &network_trans_);
   if (rv != OK) {
-    TransitionToState(STATE_NONE);
+    TransitionToState(STATE_FINISH_HEADERS);
     return rv;
   }
   network_trans_->SetBeforeNetworkStartCallback(before_network_start_callback_);
@@ -1424,7 +1446,7 @@ int HttpCache::Transaction::DoSendRequest() {
 int HttpCache::Transaction::DoSendRequestComplete(int result) {
   TRACE_EVENT0("io", "HttpCacheTransaction::DoSendRequestComplete");
   if (!cache_.get()) {
-    TransitionToState(STATE_NONE);
+    TransitionToState(STATE_FINISH_HEADERS);
     return ERR_UNEXPECTED;
   }
 
@@ -1455,7 +1477,7 @@ int HttpCache::Transaction::DoSendRequestComplete(int result) {
     DoneWritingToEntry(true);
   }
 
-  TransitionToState(STATE_NONE);
+  TransitionToState(STATE_FINISH_HEADERS);
   return result;
 }
 
@@ -1469,7 +1491,7 @@ int HttpCache::Transaction::DoSuccessfulSendRequest() {
       new_response->headers->response_code() == 407) {
     SetAuthResponse(*new_response);
     if (!reading_) {
-      TransitionToState(STATE_NONE);
+      TransitionToState(STATE_FINISH_HEADERS);
       return OK;
     }
 
@@ -1494,7 +1516,7 @@ int HttpCache::Transaction::DoSuccessfulSendRequest() {
     mode_ = NONE;
     partial_.reset();
     ResetNetworkTransaction();
-    TransitionToState(STATE_NONE);
+    TransitionToState(STATE_FINISH_HEADERS);
     return ERR_CACHE_AUTH_FAILURE_AFTER_READ;
   }
 
@@ -1532,7 +1554,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 +1572,7 @@ 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);
+    TransitionToState(STATE_FINISH_HEADERS);
     return OK;
   }
 
@@ -1566,6 +1588,20 @@ 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 &&
+      !cache_->IsTransactionCurrentOrFutureWriter(entry_, this,

[jkarlin, 2017/04/04 16:49:55]

I don't think the HCT should be aware of ActiveEntry states (e.g., writers,
readers, or pending headers). I'm trying to avoid a combinatorial explosion of
states (e.g., having to think about all 3 ActiveEntry states for each HCT
state). 

As such, HC::IsTransactionCurrentOrFutureWriter is too specific. I'd prefer
something like, "HC::CanTransactionWriteToEntry". That function should be called
before first updating/overwriting headers or first writing body bytes and it
should be DCHECKED before each call to entry_->disk_entry->WriteData.

WDYT? Would something like that work?

[shivanisha, 2017/04/04 20:33:15]

Created CanTransactionWriteResponseHeaders for this scenario and it will not be
invoked here but in DoCacheWriteResponse before attempting to write headers to
the entry.
Renamed IsTransactionCurrentOrFutureWriter to CanTransactionWriteResponseBody to
be invoked in the destructor as it is done now and invoked with a DCHECK in
WriteToEntry everytime it is about to write data to entry.

+                                                  request_->method)) {
+    cache_->DoneWritingToEntry(entry_, false, this);
+    entry_ = nullptr;
+    mode_ = NONE;
+  }
+
   return OK;
 }
 
@@ -1638,7 +1674,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.
@@ -1676,7 +1711,7 @@ int HttpCache::Transaction::DoOverwriteCachedResponse() {
     DoneWritingToEntry(false);
     mode_ = NONE;
     new_response_ = NULL;
-    TransitionToState(STATE_NONE);
+    TransitionToState(STATE_FINISH_HEADERS);
     return OK;
   }
 
@@ -1758,10 +1793,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_FINISH_HEADERS);
+    }
     return OK;
   }
 
@@ -1775,13 +1812,60 @@ 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_FINISH_HEADERS);
   } else {
-    TransitionToState(STATE_NONE);
+    TransitionToState(STATE_FINISH_HEADERS);
   }
   return OK;
 }
 
+int HttpCache::Transaction::DoHeadersPhaseCannotProceed() {
+  // If its the Start state machine and it cannot proceed due to a failure,
+  // restart this transaction.
+  DCHECK(!reading_);
+  TransitionToState(STATE_INIT_ENTRY);
+  cache_entry_status_ = CacheEntryStatus::ENTRY_UNDEFINED;
+  entry_ = nullptr;
+  mode_ = original_mode_;
+  if (network_trans_)
+    network_trans_.reset();
+  return OK;
+}
+
+int HttpCache::Transaction::DoFinishHeaders(int result) {
+  if (!entry_ || result != OK) {
+    TransitionToState(STATE_NONE);
+    return result;
+  }
+
+  TransitionToState(STATE_FINISH_HEADERS_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;
+
+  return cache_->DoneWithResponseHeaders(entry_, this);
+}
+
+int HttpCache::Transaction::DoFinishHeadersComplete(int rv) {
+  if (rv == ERR_CACHE_RACE) {
+    TransitionToState(STATE_HEADERS_PHASE_CANNOT_PROCEED);
+    return OK;
+  }
+
+  TransitionToState(STATE_NONE);
+  return rv;
+}
+
 int HttpCache::Transaction::DoCacheReadMetadata() {
   TRACE_EVENT0("io", "HttpCacheTransaction::DoCacheReadMetadata");
   DCHECK(entry_);
@@ -1804,7 +1888,8 @@ int HttpCache::Transaction::DoCacheReadMetadataComplete(int result) {
                                     result);
   if (result != response_.metadata->size())
     return OnCacheReadError(result, false);
-  TransitionToState(STATE_NONE);
+
+  TransitionToState(STATE_FINISH_HEADERS);
   return OK;
 }
 
@@ -2101,18 +2186,18 @@ int HttpCache::Transaction::BeginCacheRead() {
   // TODO(jkarlin): Either handle this case or DCHECK.
   if (response_.headers->response_code() == 206 || partial_) {
     NOTREACHED();
-    TransitionToState(STATE_NONE);
+    TransitionToState(STATE_FINISH_HEADERS);
     return ERR_CACHE_MISS;
   }
 
   // We don't have the whole resource.
   if (truncated_) {
-    TransitionToState(STATE_NONE);
+    TransitionToState(STATE_FINISH_HEADERS);
     return ERR_CACHE_MISS;
   }
 
   if (RequiresValidation() != VALIDATION_NONE) {
-    TransitionToState(STATE_NONE);
+    TransitionToState(STATE_FINISH_HEADERS);
     return ERR_CACHE_MISS;
   }
 
@@ -2122,7 +2207,7 @@ int HttpCache::Transaction::BeginCacheRead() {
   if (entry_->disk_entry->GetDataSize(kMetadataIndex))
     TransitionToState(STATE_CACHE_READ_METADATA);
   else
-    TransitionToState(STATE_NONE);
+    TransitionToState(STATE_FINISH_HEADERS);
 
   return OK;
 }
@@ -2635,7 +2720,7 @@ int HttpCache::Transaction::SetupEntryForRead() {
       partial_.reset();
     }
   }
-  cache_->ConvertWriterToReader(entry_);
+
   mode_ = READ;
 
   if (request_->method == "HEAD")
@@ -2644,7 +2729,7 @@ int HttpCache::Transaction::SetupEntryForRead() {
   if (entry_->disk_entry->GetDataSize(kMetadataIndex))
     TransitionToState(STATE_CACHE_READ_METADATA);
   else
-    TransitionToState(STATE_NONE);
+    TransitionToState(STATE_FINISH_HEADERS);
   return OK;
 }
 
@@ -2723,7 +2808,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
 }