[net] Better StopCaching() handling for HttpCache::Transaction.

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 245 matching lines @@
       new_entry_(NULL),
       new_response_(NULL),
       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_writing_(false),
       vary_mismatch_(false),
       couldnt_conditionalize_request_(false),
+      stopped_caching_(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),
       transaction_pattern_(PATTERN_UNDEFINED),
       total_received_bytes_(0),
       websocket_handshake_stream_base_create_helper_(NULL),
       weak_factory_(this) {
@@ skipping 14 matching lines @@
     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);
         }
       }
 
-      cache_->DoneWithEntry(entry_, this, cancel_request);
+      DoneWithEntry(cancel_request);
     } else if (cache_pending_) {
       cache_->RemovePendingTransaction(this);
     }
   }
 }
 
 int HttpCache::Transaction::WriteMetadata(IOBuffer* buf, int buf_len,
                                           const CompletionCallback& callback) {
   DCHECK(buf);
   DCHECK_GT(buf_len, 0);
@@ skipping 13 matching lines @@
   DCHECK(mode_ & WRITE || mode_ == NONE);
 
   // Don't set the flag for sparse entries.
   if (is_sparse_)
     return true;
 
   if (!CanResume(true))
     return false;
 
   // We may have received the whole resource already.
-  if (done_reading_)
+  if (done_writing_)
     return true;
 
   truncated_ = true;
   next_state_ = STATE_CACHE_WRITE_TRUNCATED_RESPONSE;
   DoLoop(OK);
   return true;
 }
 
 LoadState HttpCache::Transaction::GetWriterLoadState() const {
   if (network_trans_.get())
@@ skipping 11 matching lines @@
                                   const CompletionCallback& callback,
                                   const BoundNetLog& net_log) {
   DCHECK(request);
   DCHECK(!callback.is_null());
 
   // Ensure that we only have one asynchronous call at a time.
   DCHECK(callback_.is_null());
   DCHECK(!reading_);
   DCHECK(!network_trans_.get());
   DCHECK(!entry_);
-  DCHECK_EQ(next_state_, STATE_NONE);
+  DCHECK_EQ(STATE_NONE, next_state_);

[rvargas (doing something else), 2015/07/27 22:25:12]

Don't invert the arguments. DCHECKs should read like an if, not like an EXPECT

[asanka, 2015/08/18 22:46:59]

Done.

 
   if (!cache_.get())
     return ERR_UNEXPECTED;
 
   SetRequest(net_log, request);
 
   // We have to wait until the backend is initialized so we start the SM.
   next_state_ = STATE_GET_BACKEND;
   int rv = DoLoop(OK);
 
   // Setting this here allows us to check for the existence of a callback_ to
   // determine if we are still inside Start.
   if (rv == ERR_IO_PENDING)
     callback_ = callback;
 
   return rv;
 }
 
 int HttpCache::Transaction::RestartIgnoringLastError(
     const CompletionCallback& callback) {
   DCHECK(!callback.is_null());
 
   // Ensure that we only have one asynchronous call at a time.
   DCHECK(callback_.is_null());
+  DCHECK_EQ(STATE_NONE, next_state_);
 
   if (!cache_.get())
     return ERR_UNEXPECTED;
 
   int rv = RestartNetworkRequest();
 
   if (rv == ERR_IO_PENDING)
     callback_ = callback;
 
   return rv;
 }
 
 int HttpCache::Transaction::RestartWithCertificate(
     X509Certificate* client_cert,
     const CompletionCallback& callback) {
   DCHECK(!callback.is_null());
 
   // Ensure that we only have one asynchronous call at a time.
   DCHECK(callback_.is_null());
+  DCHECK_EQ(STATE_NONE, next_state_);
 
   if (!cache_.get())
     return ERR_UNEXPECTED;
 
   int rv = RestartNetworkRequestWithCertificate(client_cert);
 
   if (rv == ERR_IO_PENDING)
     callback_ = callback;
 
   return rv;
 }
 
 int HttpCache::Transaction::RestartWithAuth(
     const AuthCredentials& credentials,
     const CompletionCallback& callback) {
   DCHECK(auth_response_.headers.get());
   DCHECK(!callback.is_null());
 
   // Ensure that we only have one asynchronous call at a time.
   DCHECK(callback_.is_null());
+  DCHECK_EQ(STATE_NONE, next_state_);
 
   if (!cache_.get())
     return ERR_UNEXPECTED;
 
   // Clear the intermediate response since we are going to start over.
   auth_response_ = HttpResponseInfo();
 
   int rv = RestartNetworkRequestWithAuth(credentials);
 
   if (rv == ERR_IO_PENDING)
     callback_ = callback;
 
   return rv;
 }
 
 bool HttpCache::Transaction::IsReadyToRestartForAuth() {
   if (!network_trans_.get())
     return false;
   return network_trans_->IsReadyToRestartForAuth();
 }
 
 int HttpCache::Transaction::Read(IOBuffer* buf, int buf_len,
                                  const CompletionCallback& callback) {
-  DCHECK_EQ(next_state_, STATE_NONE);
   DCHECK(buf);
   DCHECK_GT(buf_len, 0);
   DCHECK(!callback.is_null());
 
+  // Ensure that we only have one asynchronous call at a time.
   DCHECK(callback_.is_null());
+  DCHECK_EQ(next_state_, STATE_NONE);
 
   if (!cache_.get())
     return ERR_UNEXPECTED;
 
   // If we have an intermediate auth response at this point, then it means the
   // user wishes to read the network response (the error page).  If there is a
   // previous response in the cache then we should leave it intact.
   if (auth_response_.headers.get() && mode_ != NONE) {
     UpdateTransactionPattern(PATTERN_NOT_COVERED);
     DCHECK(mode_ & WRITE);
@@ skipping 16 matching lines @@
   int rv = DoLoop(OK);
 
   if (rv == ERR_IO_PENDING) {
     DCHECK(callback_.is_null());
     callback_ = callback;
   }
   return rv;
 }
 
 void HttpCache::Transaction::StopCaching() {
-  // We really don't know where we are now. Hopefully there is no operation in
-  // progress, but nothing really prevents this method to be called after we
-  // returned ERR_IO_PENDING. We cannot attempt to truncate the entry at this
-  // point because we need the state machine for that (and even if we are really
-  // free, that would be an asynchronous operation). In other words, keep the
-  // entry how it is (it will be marked as truncated at destruction), and let
-  // the next piece of code that executes know that we are now reading directly
-  // from the net.
-  // TODO(mmenke):  This doesn't release the lock on the cache entry, so a
-  //                future request for the resource will be blocked on this one.
-  //                Fix this.
-  if (cache_.get() && entry_ && (mode_ & WRITE) && network_trans_.get() &&
-      !is_sparse_ && !range_requested_) {
-    mode_ = NONE;
-  }
+  stopped_caching_ = true;
+  UpdateTransactionPattern(PATTERN_NOT_COVERED);
 }
 
 bool HttpCache::Transaction::GetFullRequestHeaders(
     HttpRequestHeaders* headers) const {
   if (network_trans_)
     return network_trans_->GetFullRequestHeaders(headers);
 
   // TODO(ttuttle): Read headers from cache.
   return false;
 }
 
 int64 HttpCache::Transaction::GetTotalReceivedBytes() const {
   int64 total_received_bytes = total_received_bytes_;
   if (network_trans_)
     total_received_bytes += network_trans_->GetTotalReceivedBytes();
   return total_received_bytes;
 }
 
 void HttpCache::Transaction::DoneReading() {
-  if (cache_.get() && entry_) {
-    DCHECK_NE(mode_, UPDATE);
-    if (mode_ & WRITE) {
-      DoneWritingToEntry(true);
-    } else if (mode_ & READ) {
-      // It is necessary to check mode_ & READ because it is possible
-      // for mode_ to be NONE and entry_ non-NULL with a write entry
-      // if StopCaching was called.
-      cache_->DoneReadingFromEntry(entry_, this);
-      entry_ = NULL;
-    }
-  }
+  DCHECK_NE(mode_, UPDATE);
+  DoneWithRequest();
 }
 
 const HttpResponseInfo* HttpCache::Transaction::GetResponseInfo() const {
   // Null headers means we encountered an error or haven't a response yet
   if (auth_response_.headers.get())
     return &auth_response_;
   return &response_;
 }
 
 LoadState HttpCache::Transaction::GetLoadState() const {
@@ skipping 792 matching lines @@
 // We may end up here multiple times for a given request.
 int HttpCache::Transaction::DoStartPartialCacheValidation() {
   if (mode_ == NONE)
     return OK;
 
   next_state_ = STATE_COMPLETE_PARTIAL_CACHE_VALIDATION;
   return partial_->ShouldValidateCache(entry_->disk_entry, io_callback_);
 }
 
 int HttpCache::Transaction::DoCompletePartialCacheValidation(int result) {
-  if (!result) {
-    // This is the end of the request.
-    if (mode_ & WRITE) {
-      DoneWritingToEntry(true);
-    } else {
-      cache_->DoneReadingFromEntry(entry_, this);
-      entry_ = NULL;
-    }
-    return result;
-  }
+  // This is the end of the request.
+  if (!result)
+    return DoneWithRequest();
 
   if (result < 0)
     return result;
 
   partial_->PrepareCacheValidation(entry_->disk_entry,
                                    &custom_request_->extra_headers);
 
   if (reading_ && partial_->IsCurrentRangeCached()) {
     next_state_ = STATE_CACHE_READ_DATA;
     return OK;
@@ skipping 125 matching lines @@
     UpdateTransactionPattern(PATTERN_ENTRY_NOT_CACHED);
   }
 
   // Invalidate any cached GET with a successful PUT or DELETE.
   if (mode_ == WRITE &&
       (request_->method == "PUT" || request_->method == "DELETE")) {
     if (NonErrorResponse(new_response->headers->response_code())) {
       int ret = cache_->DoomEntry(cache_key_, NULL);
       DCHECK_EQ(OK, ret);
     }
-    cache_->DoneWritingToEntry(entry_, true);
-    entry_ = NULL;
-    mode_ = NONE;
+    DoneWritingToEntry(true);
   }
 
   // Invalidate any cached GET with a successful POST.
   if (!(effective_load_flags_ & LOAD_DISABLE_CACHE) &&
       request_->method == "POST" &&
       NonErrorResponse(new_response->headers->response_code())) {
     cache_->DoomMainEntryForUrl(request_->url);
   }
 
   RecordNoStoreHeaderHistogram(request_->load_flags, new_response);
@@ skipping 114 matching lines @@
 
   // We change the value of Content-Length for partial content.
   if (handling_206_ && partial_)
     partial_->FixContentLength(new_response_->headers.get());
 
   response_ = *new_response_;
 
   if (request_->method == "HEAD") {
     // This response is replacing the cached one.
     DoneWritingToEntry(false);
-    mode_ = NONE;
     new_response_ = NULL;
     return OK;
   }
 
   if (handling_206_ && !CanResume(false)) {
     // There is no point in storing this resource because it will never be used.
     // This may change if we support LOAD_ONLY_FROM_CACHE with sparse entries.
     DoneWritingToEntry(false);
     if (partial_)
       partial_->FixResponseHeaders(response_.headers.get(), true);
@@ skipping 112 matching lines @@
   next_state_ = STATE_NETWORK_READ_COMPLETE;
   return network_trans_->Read(read_buf_.get(), io_buf_len_, io_callback_);
 }
 
 int HttpCache::Transaction::DoNetworkReadComplete(int result) {
   DCHECK(mode_ & WRITE || mode_ == NONE);
 
   if (!cache_.get())
     return ERR_UNEXPECTED;
 
-  // If there is an error or we aren't saving the data, we are done; just wait
-  // until the destructor runs to see if we can keep the data.
-  if (mode_ == NONE || result < 0)
+  if (result < 0)
     return result;
 
+  // Skip writing if StopCaching() has been called.
+  //
+  // For partial requests the transaction still needs to update its state. If
+  // the network transaction being read was conditionalized, the cache
+  // transaction may need to issue additional network or cache reads to fulfil
+  // the original request.
+  if (stopped_caching_ && partial_)
+    return DoPartialNetworkReadCompleted(result);
+
+  if (mode_ == NONE || stopped_caching_)
+    return DoReadComplete(result);
+
   next_state_ = STATE_CACHE_WRITE_DATA;
   return result;
 }
 
 int HttpCache::Transaction::DoCacheReadData() {
   if (request_->method == "HEAD")
     return 0;
 
   DCHECK(entry_);
   next_state_ = STATE_CACHE_READ_DATA_COMPLETE;
@@ skipping 48 matching lines @@
 
   if (!entry_ || !num_bytes)
     return num_bytes;
 
   int current_size = entry_->disk_entry->GetDataSize(kResponseContentIndex);
   return WriteToEntry(kResponseContentIndex, current_size, read_buf_.get(),
                       num_bytes, io_callback_);
 }
 
 int HttpCache::Transaction::DoCacheWriteDataComplete(int result) {
-  if (entry_) {
-    if (net_log_.IsCapturing()) {
-      net_log_.EndEventWithNetErrorCode(NetLog::TYPE_HTTP_CACHE_WRITE_DATA,
-                                        result);
-    }
-  }
+  if (entry_ && net_log_.IsCapturing())

[rvargas (doing something else), 2015/07/27 22:25:12]

needs {}

[asanka, 2015/08/18 22:46:59]

Done.

+    net_log_.EndEventWithNetErrorCode(NetLog::TYPE_HTTP_CACHE_WRITE_DATA,
+                                      result);
   if (!cache_.get())
     return ERR_UNEXPECTED;
 
   if (result != write_len_) {
     DLOG(ERROR) << "failed to write response data to cache";
     DoneWritingToEntry(false);
 
     // We want to ignore errors writing to disk and just keep reading from
     // the network.
     result = write_len_;
-  } else if (!done_reading_ && entry_) {
+  } else if (!done_writing_ && entry_) {
     int current_size = entry_->disk_entry->GetDataSize(kResponseContentIndex);
     int64 body_size = response_.headers->GetContentLength();
     if (body_size >= 0 && body_size <= current_size)
-      done_reading_ = true;
+      done_writing_ = true;

[rvargas (doing something else), 2015/07/27 22:25:12]

We should probably keep the old name. So far, it still means that the user is
still reading from the entry, not that we are not writing to the cache anymore.

[asanka, 2015/08/18 22:46:59]

Changed to the old name.

   }
 
   if (partial_) {
     // This may be the last request.
     if (result != 0 || truncated_ ||
         !(partial_->IsLastRange() || mode_ == WRITE)) {
       return DoPartialNetworkReadCompleted(result);
     }
   }
 
   if (result == 0) {
     // End of file. This may be the result of a connection problem so see if we
     // have to keep the entry around to be flagged as truncated later on.
-    if (done_reading_ || !entry_ || partial_ ||
-        response_.headers->GetContentLength() <= 0) {
+    if (done_writing_ || !entry_ || partial_ ||
+        response_.headers->GetContentLength() <= 0)

[rvargas (doing something else), 2015/07/27 22:25:12]

needs {}

[asanka, 2015/08/18 22:46:59]

Done.

       DoneWritingToEntry(true);
-    }
   }
 
   return result;
 }
 
 int HttpCache::Transaction::DoCacheWriteTruncatedResponse() {
   next_state_ = STATE_CACHE_WRITE_TRUNCATED_RESPONSE_COMPLETE;
   return WriteResponseInfoToEntry(true);
 }
 
@@ skipping 764 matching lines @@
                                       result);
   }
 
   if (result != io_buf_len_) {
     DLOG(ERROR) << "failed to write response info to cache";
     DoneWritingToEntry(false);
   }
   return OK;
 }
 
+int HttpCache::Transaction::DoneWithRequest() {
+  DCHECK_EQ(STATE_NONE, next_state_);
+
+  if (!cache_ || !entry_)
+    return OK;
+
+  if (!(mode_ & WRITE)) {
+    cache_->DoneReadingFromEntry(entry_, this);
+    entry_ = nullptr;
+    return OK;
+  }
+
+  if (stopped_caching_ && !is_sparse_ && !done_writing_ && !entry_->doomed) {
+    if (CanResume(true)) {
+      truncated_ = true;
+      return DoCacheWriteTruncatedResponse();

[rvargas (doing something else), 2015/07/27 22:25:12]

This may return pending and that is not expected by the callers. (aka, we should
not enter the sm here).

[asanka, 2015/08/18 22:46:59]

This code was removed in the latest patch set.

+    }
+    int ret = cache_->DoomEntry(cache_key_, nullptr);
+    DCHECK_EQ(OK, ret);
+  }
+
+  DoneWritingToEntry(true);
+  return OK;
+}
+
 void HttpCache::Transaction::DoneWritingToEntry(bool success) {
   if (!entry_)
     return;
 
   RecordHistograms();
 
   cache_->DoneWritingToEntry(entry_, success);
   entry_ = NULL;
   mode_ = NONE;  // switch to 'pass through' mode
 }
 
+void HttpCache::Transaction::DoneWithEntry(bool cancel) {
+  cache_->DoneWithEntry(entry_, this, cancel);
+  entry_ = nullptr;
+  is_sparse_ = false;
+  truncated_ = false;
+}
+
 int HttpCache::Transaction::OnCacheReadError(int result, bool restart) {
   DLOG(ERROR) << "ReadData failed: " << result;
   const int result_for_histogram = std::max(0, -result);
   if (restart) {
     UMA_HISTOGRAM_SPARSE_SLOWLY("HttpCache.ReadErrorRestartable",
                                 result_for_histogram);
   } else {
     UMA_HISTOGRAM_SPARSE_SLOWLY("HttpCache.ReadErrorNonRestartable",
                                 result_for_histogram);
   }
 
   // Avoid using this entry in the future.
   if (cache_.get())
     cache_->DoomActiveEntry(cache_key_);
 
   if (restart) {
     DCHECK(!reading_);
     DCHECK(!network_trans_.get());
-    cache_->DoneWithEntry(entry_, this, false);
-    entry_ = NULL;
-    is_sparse_ = false;
+    DoneWithEntry(false);
     partial_.reset();
     next_state_ = STATE_GET_BACKEND;
     return OK;
   }
 
   return ERR_CACHE_READ_FAILURE;
 }
 
 void HttpCache::Transaction::OnAddToEntryTimeout(base::TimeTicks start_time) {
   if (entry_lock_waiting_since_ != start_time)
     return;
 
   DCHECK_EQ(next_state_, STATE_ADD_TO_ENTRY_COMPLETE);
 
   if (!cache_)
     return;
 
   cache_->RemovePendingTransaction(this);
   OnIOComplete(ERR_CACHE_LOCK_TIMEOUT);
 }
 
 void HttpCache::Transaction::DoomPartialEntry(bool delete_object) {
   DVLOG(2) << "DoomPartialEntry";
   int rv = cache_->DoomEntry(cache_key_, NULL);
   DCHECK_EQ(OK, rv);
-  cache_->DoneWithEntry(entry_, this, false);
-  entry_ = NULL;
-  is_sparse_ = false;
-  truncated_ = false;
+  DoneWithEntry(false);
   if (delete_object)
-    partial_.reset(NULL);
+    partial_.reset();
 }
 
 int HttpCache::Transaction::DoPartialNetworkReadCompleted(int result) {
   partial_->OnNetworkReadCompleted(result);
 
   if (result == 0) {
     // We need to move on to the next range.
     ResetNetworkTransaction();
     next_state_ = STATE_START_PARTIAL_CACHE_VALIDATION;
+    return OK;
   }
-  return result;
+  return DoReadComplete(result);
 }
 
 int HttpCache::Transaction::DoPartialCacheReadCompleted(int result) {
   partial_->OnCacheReadCompleted(result);
 
   if (result == 0 && mode_ == READ_WRITE) {
     // We need to move on to the next range.
     next_state_ = STATE_START_PARTIAL_CACHE_VALIDATION;
   } else if (result < 0) {
     return OnCacheReadError(result, false);
   }
   return result;
 }
 
 int HttpCache::Transaction::DoRestartPartialRequest() {
   // The stored data cannot be used. Get rid of it and restart this request.
   net_log_.AddEvent(NetLog::TYPE_HTTP_CACHE_RESTART_PARTIAL_REQUEST);
 
   // WRITE + Doom + STATE_INIT_ENTRY == STATE_CREATE_ENTRY (without an attempt
   // to Doom the entry again).
   mode_ = WRITE;
   ResetPartialState(!range_requested_);
   next_state_ = STATE_CREATE_ENTRY;
   return OK;
 }
 
+int HttpCache::Transaction::DoReadComplete(int result) {
+  DCHECK(mode_ & WRITE || mode_ == NONE);
+  DCHECK(cache_);
+  DCHECK_LE(0, result);
+  DCHECK(reading_);
+
+  // If there is no partial data, or if this is the last range of, then all the
+  // remaining bytes of the response are going to be coming from the network.
+  // Hence we can release the cache entry and switch to passthrough mode.
+  if ((mode_ & WRITE) && stopped_caching_ &&
+      (!partial_ || partial_->IsLastRange()))

[rvargas (doing something else), 2015/07/27 22:25:12]

needs {}

[asanka, 2015/08/18 22:46:59]

Done.

+    return BeginCacheEntryRelease(result);
+  return result;
+}
+
+int HttpCache::Transaction::BeginCacheEntryRelease(int result_to_forward) {
+  DCHECK_EQ(STATE_NONE, next_state_);
+  DCHECK(stopped_caching_);
+
+  // Any errors from the cache truncation operation is going to be ignored.
+  // There's not much the caller can do about it.
+  callback_ = base::Bind(&Transaction::OnCacheReleaseComplete,

[rvargas (doing something else), 2015/07/27 22:25:12]

This is surprising. So far callback_ is always a caller supplied callback and
adding this hear makes that another axis to think about.

[asanka, 2015/08/18 22:46:59]

Yeah. This was removed.

+                         weak_factory_.GetWeakPtr(),
+                         base::Bind(callback_, result_to_forward));
+  return DoneWithRequest();
+}
+
+void HttpCache::Transaction::OnCacheReleaseComplete(

[rvargas (doing something else), 2015/07/27 22:25:12]

Why is this not part of the state machine?

[asanka, 2015/08/18 22:46:59]

Point taken, but this code was removed.

+    const base::Closure& closure,
+    int result) {
+  // TODO(asanka): Clean this up.
+  if (entry_) {
+    DCHECK(mode_ & WRITE);
+    DoneWritingToEntry(true);
+  }
+  mode_ = NONE;
+  stopped_caching_ = false;
+  if (!closure.is_null())
+    closure.Run();
+}
+
 void HttpCache::Transaction::ResetPartialState(bool delete_object) {
   partial_->RestoreHeaders(&custom_request_->extra_headers);
   DoomPartialEntry(delete_object);
 
   if (!delete_object) {
     // The simplest way to re-initialize partial_ is to create a new object.
     partial_.reset(new PartialData());
     if (partial_->Init(request_->extra_headers))
       partial_->SetHeaders(custom_request_->extra_headers);
     else
@@ skipping 132 matching lines @@
     default:
       NOTREACHED();
   }
 }
 
 void HttpCache::Transaction::OnIOComplete(int result) {
   DoLoop(result);
 }
 
 }  // namespace net