Doom and create new entry when validation is not a match

net/http/http_cache_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_cache_transaction.h"
 
 #include "build/build_config.h"  // For OS_POSIX
 
 #if defined(OS_POSIX)
 #include <unistd.h>
@@ skipping 147 matching lines @@
       mode_(NONE),
       original_mode_(NONE),
       reading_(false),
       invalid_range_(false),
       truncated_(false),
       is_sparse_(false),
       range_requested_(false),
       handling_206_(false),
       cache_pending_(false),
       done_reading_(false),
+      done_headers_create_new_entry_(false),
       vary_mismatch_(false),
       couldnt_conditionalize_request_(false),
       bypass_lock_for_test_(false),
       fail_conditionalization_for_test_(false),
       io_buf_len_(0),
       read_offset_(0),
       effective_load_flags_(0),
       write_len_(0),
       cache_entry_status_(CacheEntryStatus::ENTRY_UNDEFINED),
       validation_cause_(VALIDATION_CAUSE_UNDEFINED),
@@ skipping 569 matching lines @@
       case STATE_CREATE_ENTRY_COMPLETE:
         rv = DoCreateEntryComplete(rv);
         break;
       case STATE_ADD_TO_ENTRY:
         DCHECK_EQ(OK, rv);
         rv = DoAddToEntry();
         break;
       case STATE_ADD_TO_ENTRY_COMPLETE:
         rv = DoAddToEntryComplete(rv);
         break;
+      case STATE_DONE_HEADERS_ADD_TO_ENTRY_COMPLETE:
+        rv = DoDoneHeadersAddToEntryComplete(rv);
+        break;
       case STATE_CACHE_READ_RESPONSE:
         DCHECK_EQ(OK, rv);
         rv = DoCacheReadResponse();
         break;
       case STATE_CACHE_READ_RESPONSE_COMPLETE:
         rv = DoCacheReadResponseComplete(rv);
         break;
       case STATE_TOGGLE_UNUSED_SINCE_PREFETCH:
         DCHECK_EQ(OK, rv);
         rv = DoCacheToggleUnusedSincePrefetch();
@@ skipping 338 matching lines @@
     case ERR_CACHE_RACE:
       TransitionToState(STATE_HEADERS_PHASE_CANNOT_PROCEED);
       break;
 
     default:
       // We have a race here: Maybe we failed to open the entry and decided to
       // create one, but by the time we called create, another transaction
       // already created the entry. If we want to eliminate this issue, we
       // need an atomic OpenOrCreate() method exposed by the disk cache.
       DLOG(WARNING) << "Unable to create cache entry";
+      // Change the mode to not write anything to the cache.

[Randy Smith (Not in Mondays), 2017/04/11 19:41:07]

This comment doesn't really add anything to the code; that's what "mode_ =
NONE;" means.  "Switching into passing through data directly from the network,
avoiding the cache entry" was more what I had in mind.

[shivanisha, 2017/04/24 17:23:46]

Done

       mode_ = NONE;
+      if (done_headers_create_new_entry_) {

[Randy Smith (Not in Mondays), 2017/04/11 19:41:07]

Maybe a comment here something like "The headers to use to write the response
have already been received as a result of validation, triggering the doom of the
old entry.  So no network request needs to be sent."? 

That comment's not ideal because the setting of the STATE_SEND_REQUEST is a ways
away from the point of comment, so if you want to fix that (by changing the
comment or restructuring the code) I'm good with that.  But I'd like to indicate
in comment what (at a high level, not just repeating the information in the
code) is being done by the conditional.

[shivanisha, 2017/04/24 17:23:46]

Comment added and restructured the code.

+        done_headers_create_new_entry_ = false;
+        TransitionToState(STATE_CACHE_WRITE_RESPONSE);
+        return OK;
+      }
+
       if (partial_)
         partial_->RestoreHeaders(&custom_request_->extra_headers);
       TransitionToState(STATE_SEND_REQUEST);
   }
   return OK;
 }
 
 int HttpCache::Transaction::DoAddToEntry() {
   TRACE_EVENT0("io", "HttpCacheTransaction::DoAddToEntry");
   DCHECK(new_entry_);
   cache_pending_ = true;
-  TransitionToState(STATE_ADD_TO_ENTRY_COMPLETE);
   net_log_.BeginEvent(NetLogEventType::HTTP_CACHE_ADD_TO_ENTRY);
   DCHECK(entry_lock_waiting_since_.is_null());
+  int rv = cache_->AddTransactionToEntry(new_entry_, this);
+  DCHECK_EQ(rv, ERR_IO_PENDING);
+
+  if (done_headers_create_new_entry_) {
+    TransitionToState(STATE_DONE_HEADERS_ADD_TO_ENTRY_COMPLETE);
+    return rv;
+  }
+
+  TransitionToState(STATE_ADD_TO_ENTRY_COMPLETE);

[Randy Smith (Not in Mondays), 2017/04/11 19:41:07]

The naming makes it clear that these are parallel states, which is going to
raise the question in a code reader why the code within this state isn't
parallel.   Could you either make the code paths parallel (i.e. I'm not sure
there's harm in using the timeout path for the doom-and-create state, though we
know that the cache lock won't be held if we're creating a new entry) or put in
a comment about why they aren't parallel?

[shivanisha, 2017/04/24 17:23:46]

Added a comment in the new Do* function.

+
   entry_lock_waiting_since_ = TimeTicks::Now();
-  int rv = cache_->AddTransactionToEntry(new_entry_, this);
-  if (rv == ERR_IO_PENDING) {
-    if (bypass_lock_for_test_) {
-      base::ThreadTaskRunnerHandle::Get()->PostTask(
-          FROM_HERE,
-          base::Bind(&HttpCache::Transaction::OnAddToEntryTimeout,
-                     weak_factory_.GetWeakPtr(), entry_lock_waiting_since_));
-    } else {
-      int timeout_milliseconds = 20 * 1000;
-      if (partial_ && new_entry_->writer &&
-          new_entry_->writer->range_requested_) {
-        // Quickly timeout and bypass the cache if we're a range request and
-        // we're blocked by the reader/writer lock. Doing so eliminates a long
-        // running issue, http://crbug.com/31014, where two of the same media
-        // resources could not be played back simultaneously due to one locking
-        // the cache entry until the entire video was downloaded.
-        //
-        // Bypassing the cache is not ideal, as we are now ignoring the cache
-        // entirely for all range requests to a resource beyond the first. This
-        // is however a much more succinct solution than the alternatives, which
-        // would require somewhat significant changes to the http caching logic.
-        //
-        // Allow some timeout slack for the entry addition to complete in case
-        // the writer lock is imminently released; we want to avoid skipping
-        // the cache if at all possible. See http://crbug.com/408765
-        timeout_milliseconds = 25;
-      }
-      base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-          FROM_HERE,
-          base::Bind(&HttpCache::Transaction::OnAddToEntryTimeout,
-                     weak_factory_.GetWeakPtr(), entry_lock_waiting_since_),
-          TimeDelta::FromMilliseconds(timeout_milliseconds));
+  if (bypass_lock_for_test_) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(&HttpCache::Transaction::OnAddToEntryTimeout,
+                   weak_factory_.GetWeakPtr(), entry_lock_waiting_since_));
+  } else {
+    int timeout_milliseconds = 20 * 1000;
+    if (partial_ && new_entry_->writer &&
+        new_entry_->writer->range_requested_) {
+      // Quickly timeout and bypass the cache if we're a range request and
+      // we're blocked by the reader/writer lock. Doing so eliminates a long
+      // running issue, http://crbug.com/31014, where two of the same media
+      // resources could not be played back simultaneously due to one locking
+      // the cache entry until the entire video was downloaded.
+      //
+      // Bypassing the cache is not ideal, as we are now ignoring the cache
+      // entirely for all range requests to a resource beyond the first. This
+      // is however a much more succinct solution than the alternatives, which
+      // would require somewhat significant changes to the http caching logic.
+      //
+      // Allow some timeout slack for the entry addition to complete in case
+      // the writer lock is imminently released; we want to avoid skipping
+      // the cache if at all possible. See http://crbug.com/408765
+      timeout_milliseconds = 25;
     }
+    base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+        FROM_HERE,
+        base::Bind(&HttpCache::Transaction::OnAddToEntryTimeout,
+                   weak_factory_.GetWeakPtr(), entry_lock_waiting_since_),
+        TimeDelta::FromMilliseconds(timeout_milliseconds));

[Randy Smith (Not in Mondays), 2017/04/11 19:41:07]

Why is this pulled out of the conditional?  It looks like there'll be a double
post in the test case.

[shivanisha, 2017/04/24 17:23:46]

It is not out of the else conditional. This post task is part of the else but
out of the if (partial ...) case as in original code, so no double post.

I think the confusion arises because the diff view and that's because the
indentation changed from original as there isn't an if (rv == ERR_IO_PENDING)
outside the whole post-task logic.

   }
   return rv;
 }
 
 int HttpCache::Transaction::DoAddToEntryComplete(int result) {
   TRACE_EVENT0("io", "HttpCacheTransaction::DoAddToEntryComplete");
   net_log_.EndEventWithNetErrorCode(NetLogEventType::HTTP_CACHE_ADD_TO_ENTRY,
                                     result);
   const TimeDelta entry_lock_wait =
       TimeTicks::Now() - entry_lock_waiting_since_;
@@ skipping 44 matching lines @@
       partial_->RestoreHeaders(&custom_request_->extra_headers);
     TransitionToState(STATE_SEND_REQUEST);
   } else {
     // We have to read the headers from the cached entry.
     DCHECK(mode_ & READ_META);
     TransitionToState(STATE_CACHE_READ_RESPONSE);
   }
   return OK;
 }
 
+int HttpCache::Transaction::DoDoneHeadersAddToEntryComplete(int result) {

[Randy Smith (Not in Mondays), 2017/04/11 19:41:07]

Comment somewhere as to the context/motivation for the differences between this
and DoAddToEntryComplete?

[shivanisha, 2017/04/24 17:23:46]

done.

+  DCHECK_EQ(result, OK);
+  DCHECK(mode_ & WRITE);
+  DCHECK(new_entry_);
+  DCHECK(response_.headers);
+
+  cache_pending_ = false;
+  entry_ = new_entry_;
+  done_headers_create_new_entry_ = false;
+  DCHECK(cache_->CanTransactionWriteResponseHeaders(
+      entry_, this, response_.headers->response_code()));
+  TransitionToState(STATE_CACHE_WRITE_RESPONSE);
+  return OK;
+}
+
 int HttpCache::Transaction::DoCacheReadResponse() {
   TRACE_EVENT0("io", "HttpCacheTransaction::DoCacheReadResponse");
   DCHECK(entry_);
   TransitionToState(STATE_CACHE_READ_RESPONSE_COMPLETE);
 
   io_buf_len_ = entry_->disk_entry->GetDataSize(kResponseInfoIndex);
   read_buf_ = new IOBuffer(io_buf_len_);
 
   net_log_.BeginEvent(NetLogEventType::HTTP_CACHE_READ_INFO);
   return entry_->disk_entry->ReadData(kResponseInfoIndex, 0, read_buf_.get(),
@@ skipping 468 matching lines @@
   TransitionToState(STATE_CACHE_WRITE_RESPONSE_COMPLETE);
 
   // Invalidate any current entry with a successful response if this transaction
   // cannot write to this entry. This transaction then continues to read from
   // the network without writing to the backend.
   if (entry_ && response_.headers &&
       !cache_->CanTransactionWriteResponseHeaders(
           entry_, this, response_.headers->response_code())) {
     cache_->DoneWritingToEntry(entry_, false, this);
     entry_ = nullptr;
-    mode_ = NONE;
+    next_state_ = STATE_CREATE_ENTRY;
+    done_headers_create_new_entry_ = true;
     return OK;
   }
 
   return WriteResponseInfoToEntry(truncated_);
 }
 
 int HttpCache::Transaction::DoCacheWriteResponseComplete(int result) {
   TRACE_EVENT0("io", "HttpCacheTransaction::DoCacheWriteResponseComplete");
   TransitionToState(STATE_TRUNCATE_CACHED_DATA);
   return OnWriteResponseInfoToEntryComplete(result);
@@ skipping 1377 matching lines @@
 }
 
 void HttpCache::Transaction::TransitionToState(State state) {
   // Ensure that the state is only set once per Do* state.
   DCHECK(in_do_loop_);
   DCHECK_EQ(STATE_UNSET, next_state_) << "Next state is " << state;
   next_state_ = state;
 }
 
 }  // namespace net