HttpCache::Transaction layer allowing parallel validation

net/http/http_cache.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.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "base/bind.h"
@@ skipping 78 matching lines @@
                                         true,
                                         thread_,
                                         net_log,
                                         backend,
                                         callback);
 }
 
 //-----------------------------------------------------------------------------
 
 HttpCache::ActiveEntry::ActiveEntry(disk_cache::Entry* entry)
-    : disk_entry(entry),
-      writer(NULL),
-      will_process_pending_queue(false),
-      doomed(false) {
-}
+    : disk_entry(entry) {}
 
 HttpCache::ActiveEntry::~ActiveEntry() {
   if (disk_entry) {
     disk_entry->Close();
-    disk_entry = NULL;
+    disk_entry = nullptr;
   }
 }
 
 size_t HttpCache::ActiveEntry::EstimateMemoryUsage() const {
   // Skip |disk_entry| which is tracked in simple_backend_impl; Skip |readers|
   // and |pending_queue| because the Transactions are owned by their respective
   // URLRequestHttpJobs.
   return 0;
 }
 
 bool HttpCache::ActiveEntry::HasNoTransactions() {
-  return !writer && readers.empty() && pending_queue.empty();
+  return !writer && readers.empty() && pending_queue.empty() &&
+         validated_queue.empty() && !validating_transaction;
+}
+
+bool HttpCache::ActiveEntry::IsReader(Transaction* transaction) {
+  return readers.count(transaction);
 }
 
 //-----------------------------------------------------------------------------
 
 // This structure keeps track of work items that are attempting to create or
 // open cache entries or the backend itself.
 struct HttpCache::PendingOp {
   PendingOp() : disk_entry(NULL) {}
   ~PendingOp() {}
 
@@ skipping 222 matching lines @@
     }
   }
 }
 
 HttpCache::~HttpCache() {
   // Transactions should see an invalid cache after this point; otherwise they
   // could see an inconsistent object (half destroyed).
   weak_factory_.InvalidateWeakPtrs();
 
   // If we have any active entries remaining, then we need to deactivate them.
-  // We may have some pending calls to OnProcessPendingQueue, but since those
-  // won't run (due to our destruction), we can simply ignore the corresponding
-  // will_process_pending_queue flag.
+  // We may have some pending tasks to process pending queue for reading or
+  // parallel validation, but since those won't run (due to our destruction),
+  // we can simply ignore the corresponding flags.
   while (!active_entries_.empty()) {
     ActiveEntry* entry = active_entries_.begin()->second.get();
     entry->will_process_pending_queue = false;
+    entry->will_process_parallel_validation = false;
     entry->pending_queue.clear();
     entry->readers.clear();
-    entry->writer = NULL;
+    entry->validated_queue.clear();
+    entry->validating_transaction = nullptr;
+    entry->writer = nullptr;
     DeactivateEntry(entry);
   }
 
   doomed_entries_.clear();
 
   // Before deleting pending_ops_, we have to make sure that the disk cache is
   // done with said operations, or it will attempt to use deleted data.
   disk_cache_.reset();
 
   for (auto pending_it = pending_ops_.begin(); pending_it != pending_ops_.end();
@@ skipping 238 matching lines @@
 
   // We keep track of doomed entries so that we can ensure that they are
   // cleaned up properly when the cache is destroyed.
   ActiveEntry* entry_ptr = entry.get();
   DCHECK_EQ(0u, doomed_entries_.count(entry_ptr));
   doomed_entries_[entry_ptr] = std::move(entry);
 
   entry_ptr->disk_entry->Doom();
   entry_ptr->doomed = true;
 
   DCHECK(entry_ptr->writer || !entry_ptr->readers.empty() ||

[Randy Smith (Not in Mondays), 2017/03/07 22:54:30]

Do you know what the purpose of this DCHECK is?  It seems to be saying "You can
only doom an entry if some transaction is using it", and I'm curious if that's
true and if so what the logic is behind that assertion.

[shivanisha, 2017/03/08 21:42:12]

DoomEntry's consumer is the HttpCache::Transaction (either directly or via
DoomActiveEntry or DoomMainEntryForUrl). There are various places where HCT
dooms the entry. In some places it is either sure that there is another consumer
for this entry (e.g. when it received this entry from active_entries_ map, it
dooms it in STATE_DOOM_ENTRY) or if its the only consumer for this entry (e.g.
for no-store requests). In many cases when its the only consumer it will also
call DoneWritingToEntry after dooming which will ensure the entry is destroyed.

This explanation helped me to correct the logic in DoneValidationWithEntry in
the new patch when its not a match. I have added a condition to check that if
this was the only active consumer then invoke
DestroyEntryRestartPendingTransactions else invoke
DoomEntryRestartPendingTransactions

-         entry_ptr->will_process_pending_queue);
+         !entry_ptr->validated_queue.empty() ||
+         entry_ptr->validating_transaction ||
+         entry_ptr->will_process_pending_queue ||
+         entry_ptr->will_process_parallel_validation);
   return OK;
 }
 
 int HttpCache::AsyncDoomEntry(const std::string& key, Transaction* trans) {
   std::unique_ptr<WorkItem> item =
       base::MakeUnique<WorkItem>(WI_DOOM_ENTRY, trans, nullptr);
   PendingOp* pending_op = GetPendingOp(key);
   if (pending_op->writer) {
     pending_op->pending_queue.push_back(std::move(item));
     return ERR_IO_PENDING;
@@ skipping 35 matching lines @@
   DCHECK(entry->doomed);
   DCHECK(entry->HasNoTransactions());
 
   auto it = doomed_entries_.find(entry);
   DCHECK(it != doomed_entries_.end());
   doomed_entries_.erase(it);
 }
 
 HttpCache::ActiveEntry* HttpCache::FindActiveEntry(const std::string& key) {
   auto it = active_entries_.find(key);
-  return it != active_entries_.end() ? it->second.get() : NULL;
+  return it != active_entries_.end() ? it->second.get() : nullptr;
+}
+
+bool HttpCache::IsEntryActive(const std::string& key,
+                              const ActiveEntry* entry) {
+  ActiveEntry* entry_alive = FindActiveEntry(key);
+  if (!entry_alive || entry != entry_alive)

[jkarlin, 2017/03/07 16:45:41]

return entry_alive && entry_alive == entry;

[shivanisha, 2017/03/08 21:42:11]

Done. Also renamed entry_alive to active_entry

+    return false;
+  return true;
 }
 
 HttpCache::ActiveEntry* HttpCache::ActivateEntry(
     disk_cache::Entry* disk_entry) {
   DCHECK(!FindActiveEntry(disk_entry->GetKey()));
   ActiveEntry* entry = new ActiveEntry(disk_entry);
   active_entries_[disk_entry->GetKey()] = base::WrapUnique(entry);
   return entry;
 }
 
 void HttpCache::DeactivateEntry(ActiveEntry* entry) {
-  DCHECK(!entry->will_process_pending_queue);

[Randy Smith (Not in Mondays), 2017/03/07 22:54:30]

Why?

[shivanisha, 2017/03/08 21:42:12]

Now that we have 2 types of tasks posted (OnProcessWaitingTransactions and
OnProcessParallelValidation), one of them can destroy the entry if there are no
transactions while the other one may be posted, thus removed this assert and
also added a IsEntryAlive check in both tasks.

(Note the names of functions here correspond to the new patch)

   DCHECK(!entry->doomed);
   DCHECK(entry->disk_entry);
   DCHECK(entry->HasNoTransactions());
 
   std::string key = entry->disk_entry->GetKey();
   if (key.empty())
     return SlowDeactivateEntry(entry);
 
   auto it = active_entries_.find(key);
   DCHECK(it != active_entries_.end());
@@ skipping 109 matching lines @@
 }
 
 void HttpCache::DestroyEntry(ActiveEntry* entry) {
   if (entry->doomed) {
     FinalizeDoomedEntry(entry);
   } else {
     DeactivateEntry(entry);
   }
 }
 
-int HttpCache::AddTransactionToEntry(ActiveEntry* entry, Transaction* trans) {
+int HttpCache::AddTransactionToEntry(ActiveEntry* entry,
+                                     Transaction* transaction,
+                                     bool validated_queue,

[jkarlin, 2017/03/07 16:45:41]

We discussed this F2F, but can you remove validated_queue and separate that
logic into a separate function? 

+                                     bool new_transaction) {

[jkarlin, 2017/03/07 16:45:40]

is new_transaction ever set to true? I don't see it from any callers from
HttpCache or HttpCacheTransaction

[shivanisha, 2017/03/08 21:42:11]

Refactored this function into 4 functions:

- Always called from HCT for a new transaction
AddTransactionToEntry()
so we do not need a boolean for new transaction.

This further calls AddTransactionToEntryImpl() which could also be called for
queued transactions.
This further calls either AddTransactionToEntryForRead() or
AddTransactionToEntryForWrite(). 

AddTransactionToEntryForRead() is also called directly for validated_queue since
they should only be processed for reading. 

   DCHECK(entry);
   DCHECK(entry->disk_entry);
 
-  // We implement a basic reader/writer lock for the disk cache entry.  If
-  // there is already a writer, then everyone has to wait for the writer to
-  // finish before they can access the cache entry.  There can be multiple
-  // readers.
-  //
-  // NOTE: If the transaction can only write, then the entry should not be in
-  // use (since any existing entry should have already been doomed).
+  // We implement a basic reader/writer lock for the disk cache entry. If there
+  // is a writer, then all read-only transactions must wait. Non read-only
+  // transactions can proceed to their validation phase. Validation is allowed
+  // for one transaction at a time so that we do not end up with wasted network
+  // requests.
 
-  if (entry->writer || entry->will_process_pending_queue) {
-    entry->pending_queue.push_back(trans);
+  // If a pending queue processing task is posted and this is invoked for a
+  // new transaction as opposed to a queued transaction, then this transaction
+  // should be added after the queue task is processed, to maintain FIFO
+  // ordering.
+  if (new_transaction && (entry->will_process_pending_queue ||
+                          entry->will_process_parallel_validation)) {
+    entry->pending_queue.push_back(transaction);
     return ERR_IO_PENDING;
   }
 
-  if (trans->mode() & Transaction::WRITE) {
-    // transaction needs exclusive access to the entry
-    if (entry->readers.empty()) {
-      entry->writer = trans;
-    } else {
-      entry->pending_queue.push_back(trans);
+  // A transaction that has the write mode and has not already passed the
+  // validation phase can become the next validating transaction. It can also
+  // become the writer if there is no active writer at this time.

[Randy Smith (Not in Mondays), 2017/03/07 22:54:30]

Is there a reason for it to take up the writer slot before validation is
complete?  I don't think we want to rule out another transaction coming in later
but finishing validation first and starting to write.

[shivanisha, 2017/03/08 21:42:12]

The writer slot works as a lock (in the existing code and in this CL). Presence
of the writer decides whether a new reader can be added or not. Similarly, since
we only allow one validating transaction at a time, validating_transaction acts
as a lock that no other transaction can start validating. Thus the scenario that
you mention : "another transaction coming in later but finishing validation
first and starting to write." should never happen.

+  if ((transaction->mode() & Transaction::WRITE) && !validated_queue) {
+    if (entry->validating_transaction) {
+      entry->pending_queue.push_back(transaction);
       return ERR_IO_PENDING;
     }
-  } else {
-    // transaction needs read access to the entry
-    entry->readers.insert(trans);
+    entry->validating_transaction = transaction;
+    if (!entry->writer && entry->readers.empty()) {
+      entry->writer = transaction;
+    }
+    if (!entry->writer && !entry->pending_queue.empty())
+      ProcessPendingQueue(entry);
+    return OK;
   }
 
-  // We do this before calling EntryAvailable to force any further calls to
-  // AddTransactionToEntry to add their transaction to the pending queue, which
-  // ensures FIFO ordering.
-  if (!entry->writer && !entry->pending_queue.empty())
+  // Read-only transactions.
+  if (entry->writer) {
+    entry->pending_queue.push_back(transaction);
+    return ERR_IO_PENDING;
+  }
+  entry->readers.insert(transaction);
+  if (!entry->pending_queue.empty())
     ProcessPendingQueue(entry);

[Randy Smith (Not in Mondays), 2017/03/07 22:54:30]

nit, suggestion: It feels like this routine might be clearer if (e.g.)
assignment in pending queue in these last several lines was unconditional, and
then in ProcessPendingQueue situations like the lack of writer and a non-empty
pending_queue were detected, and ProcessPendingQueue pushed the state along.  If
you agree with this suggestion, you might want to scan the other functions to
see if it applies; having a single or a small number of functions with state
advancing logic can make that state advancing logic much easier to understand.

(I suspect it's too much of a refactor, but if you want to and think it would be
a good idea, I'd supporting converting all such routines into an AdvanceState()
routine and really centralize the state logic.)

[shivanisha, 2017/03/08 21:42:12]

I have refactored this function quite a bit in the next patch. The current
AddTransactionToEntry() is now only called for new transactions and not queued
transactions before I saw this comment. PTAL.

-
   return OK;
 }
 
-void HttpCache::DoneWithEntry(ActiveEntry* entry, Transaction* trans,
+int HttpCache::OnValidationMatch(ActiveEntry* entry,
+                                 Transaction* transaction,
+                                 bool write_this_response) {
+  int rv = OK;
+  entry->validating_transaction = nullptr;
+
+  if (entry->writer != transaction) {
+    entry->validated_queue.push_back(transaction);
+    rv = ERR_IO_PENDING;
+  } else if (!write_this_response) {
+    entry->writer = nullptr;
+    entry->readers.insert(transaction);

[jkarlin, 2017/03/07 16:45:40]

We're adding it to readers, but I don't think the transactions mode is set to
READ yet is it?

[shivanisha, 2017/03/08 21:42:11]

Actually this should never happen. ConvertWriterToReader should have already
done that and if not, it will be added to validated_queue here. Removing this
case and adding comments to explain this in the new refactored code.

+  }
+  ProcessParallelValidation(entry);
+  return rv;
+}
+
+void HttpCache::OnValidationNoMatch(ActiveEntry* entry,
+                                    Transaction* transaction) {
+  entry->validating_transaction = nullptr;
+  DoomEntryRestartPendingTransactions(entry);
+}
+
+void HttpCache::DoneWithEntry(ActiveEntry* entry,
+                              Transaction* transaction,
                               bool cancel) {
   // If we already posted a task to move on to the next transaction and this was
   // the writer, there is nothing to cancel.
-  if (entry->will_process_pending_queue && entry->readers.empty())
+  if (entry->will_process_pending_queue && entry->writer == transaction)
     return;
 
-  if (entry->writer) {
-    DCHECK(trans == entry->writer);
+  // Transaction is waiting in the validated_queue.
+  auto i = std::find(entry->validated_queue.begin(),

[jkarlin, 2017/03/07 16:45:40]

There are a few linear searches in this new code. Not something to fix yet but
perhaps make a note of it.

+                     entry->validated_queue.end(), transaction);
+  if (i != entry->validated_queue.end()) {
+    entry->validated_queue.erase(i);

[Randy Smith (Not in Mondays), 2017/03/07 22:54:30]

Can't it be in the validated queue and be the writer?  In that situation, won't
the writer remain set to the transaction?

[shivanisha, 2017/03/08 21:42:12]

That should not be possible.
validated_queue insertion happens in DoneValidationWithEntry only if writer is
not the same as this.

+    return;
+  }
 
-    // Assume there was a failure.
-    bool success = false;
-    if (cancel) {
-      DCHECK(entry->disk_entry);
-      // This is a successful operation in the sense that we want to keep the
-      // entry.
-      success = trans->AddTruncatedFlag();
-      // The previous operation may have deleted the entry.
-      if (!trans->entry())
-        return;
-    }
-    DoneWritingToEntry(entry, success);
-  } else {
-    DoneReadingFromEntry(entry, trans);
+  // Assume there was a failure.
+  bool success = false;
+  if (transaction == entry->writer && cancel) {
+    DCHECK(entry->disk_entry);
+    // This is a successful operation in the sense that we want to keep the
+    // entry.
+    success = transaction->AddTruncatedFlag();
+    // The previous operation may have deleted the entry.
+    if (!transaction->entry())
+      return;
   }
+
+  if (transaction == entry->writer ||

[Randy Smith (Not in Mondays), 2017/03/07 22:54:30]

Suggestion: I'd value a comment here, parallel to the ones above.

[shivanisha, 2017/03/08 21:42:11]

Done.

+      transaction == entry->validating_transaction) {
+    DoneWritingToEntry(entry, success, transaction);
+    return;
+  }
+
+  DoneReadingFromEntry(entry, transaction);
 }
 
-void HttpCache::DoneWritingToEntry(ActiveEntry* entry, bool success) {
-  DCHECK(entry->readers.empty());
+void HttpCache::DoneWritingToEntry(ActiveEntry* entry,
+                                   bool success,
+                                   Transaction* transaction) {
+  // Transaction is done writing to entry in the validation phase.
+  if (transaction == entry->validating_transaction) {
+    entry->validating_transaction = nullptr;
 
-  entry->writer = NULL;
+    if (transaction == entry->writer)
+      entry->writer = nullptr;
 
+    if (!entry->writer)
+      DoneWritingToEntryProcessOtherTransactions(entry, success);
+    return;
+  }
+
+  // Transaction is done writing to entry in the response body phase.
+  DCHECK_EQ(transaction, entry->writer);

[Randy Smith (Not in Mondays), 2017/03/07 22:54:30]

Suggestion: Hoist this up tot he top of the routine and make it
DCHECK(transaction == entry->writer || transaction ==
entry->validating_transacation).  That way it acts as a contract check as well
as documentation at the beginning of the routine as to the expected environment.

[shivanisha, 2017/03/08 21:42:11]

Done.

+  entry->writer = nullptr;

[Randy Smith (Not in Mondays), 2017/03/07 22:54:30]

Suggestion: If you take the above suggestion, you could hoist the "if
(transaction == entry->writer) entry->writer = nullptr" up to the beginning of
the routine and eliminate this line.

[shivanisha, 2017/03/08 21:42:12]

done

+
+  // Whether its a failure or success validating transaction can be the new
+  // writer.
+  if (entry->validating_transaction) {
+    entry->writer = entry->validating_transaction;
+    return;
+  }
+
+  DoneWritingToEntryProcessOtherTransactions(entry, success);
+}
+
+void HttpCache::DoneWritingToEntryProcessOtherTransactions(ActiveEntry* entry,
+                                                           bool success) {
   if (success) {
+    DCHECK(entry->readers.empty());
     ProcessPendingQueue(entry);
-  } else {
-    DCHECK(!entry->will_process_pending_queue);
+    return;
+  }
 
-    // We failed to create this entry.
-    TransactionList pending_queue;
-    pending_queue.swap(entry->pending_queue);
-
-    entry->disk_entry->Doom();
-    DestroyEntry(entry);
-
-    // We need to do something about these pending entries, which now need to
-    // be added to a new entry.
-    while (!pending_queue.empty()) {
-      // ERR_CACHE_RACE causes the transaction to restart the whole process.
-      pending_queue.front()->io_callback().Run(ERR_CACHE_RACE);
-      pending_queue.pop_front();
-    }
-  }
+  DestroyEntryRestartPendingTransactions(entry);
 }
 
 void HttpCache::DoneReadingFromEntry(ActiveEntry* entry, Transaction* trans) {
   DCHECK(!entry->writer);
 
   auto it = entry->readers.find(trans);
   DCHECK(it != entry->readers.end());
 
   entry->readers.erase(it);
 
   ProcessPendingQueue(entry);
 }
 
-void HttpCache::ConvertWriterToReader(ActiveEntry* entry) {
-  DCHECK(entry->writer);
-  DCHECK(entry->writer->mode() == Transaction::READ_WRITE);
-  DCHECK(entry->readers.empty());
+bool HttpCache::ConvertWriterToReader(ActiveEntry* entry,
+                                      Transaction* transaction) {
+  DCHECK(entry->writer == transaction ||
+         entry->validating_transaction == transaction);
+  DCHECK(transaction->mode() == Transaction::READ_WRITE);
 
-  Transaction* trans = entry->writer;
+  if (transaction == entry->validating_transaction) {
+    entry->validating_transaction = nullptr;
+    if (transaction == entry->writer)
+      entry->writer = nullptr;
+  } else {
+    DCHECK(entry->readers.empty());
+    DCHECK_EQ(transaction, entry->writer);
+    entry->writer = nullptr;
+  }
 
-  entry->writer = NULL;
-  entry->readers.insert(trans);
+  // If a reader cannot exist at the moment, it will be added to validated queue
+  // in OnValidationMatch, so return.
+  if (entry->writer)
+    return false;
 
+  entry->readers.insert(transaction);
   ProcessPendingQueue(entry);
+  return true;
 }
 
 LoadState HttpCache::GetLoadStateForPendingTransaction(
       const Transaction* trans) {
   auto i = active_entries_.find(trans->key());
   if (i == active_entries_.end()) {
     // If this is really a pending transaction, and it is not part of
     // active_entries_, we should be creating the backend or the entry.
     return LOAD_STATE_WAITING_FOR_CACHE;
   }
@@ skipping 29 matching lines @@
 
   for (auto k = doomed_entries_.begin(); k != doomed_entries_.end() && !found;
        ++k) {
     found = RemovePendingTransactionFromEntry(k->first, trans);
   }
 
   DCHECK(found) << "Pending transaction not found";
 }
 
 bool HttpCache::RemovePendingTransactionFromEntry(ActiveEntry* entry,
-                                                  Transaction* trans) {
+                                                  Transaction* transaction) {
   TransactionList& pending_queue = entry->pending_queue;
 
-  auto j = find(pending_queue.begin(), pending_queue.end(), trans);
+  auto j = find(pending_queue.begin(), pending_queue.end(), transaction);
   if (j == pending_queue.end())
     return false;
 
   pending_queue.erase(j);
   return true;
 }
 
 bool HttpCache::RemovePendingTransactionFromPendingOp(PendingOp* pending_op,
                                                       Transaction* trans) {
   if (pending_op->writer->Matches(trans)) {
@@ skipping 11 matching lines @@
   }
   return false;
 }
 
 void HttpCache::ProcessPendingQueue(ActiveEntry* entry) {
   // Multiple readers may finish with an entry at once, so we want to batch up
   // calls to OnProcessPendingQueue.  This flag also tells us that we should
   // not delete the entry before OnProcessPendingQueue runs.
   if (entry->will_process_pending_queue)
     return;
+
   entry->will_process_pending_queue = true;
 
   base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE,
-      base::Bind(&HttpCache::OnProcessPendingQueue, GetWeakPtr(), entry));
+      FROM_HERE, base::Bind(&HttpCache::OnProcessPendingQueue, GetWeakPtr(),
+                            entry->disk_entry->GetKey(), entry));
 }
 
-void HttpCache::OnProcessPendingQueue(ActiveEntry* entry) {
+void HttpCache::OnProcessPendingQueue(const std::string& key,
+                                      ActiveEntry* entry) {
+  // Check if the entry is still active.
+  if (!IsEntryActive(key, entry))
+    return;
+
   entry->will_process_pending_queue = false;
-  DCHECK(!entry->writer);
 
   // If no one is interested in this entry, then we can deactivate it.
   if (entry->HasNoTransactions()) {
+    DestroyEntry(entry);
+    return;
+  }
+
+  if (entry->validated_queue.empty() && entry->pending_queue.empty())
+    return;
+
+  Transaction* next = nullptr;
+  bool validated_queue = false;
+
+  // To maintain FIFO order of transactions, validated_queue should be processed
+  // first and then pending_queue.
+  if (!entry->validated_queue.empty()) {
+    next = entry->validated_queue.front();
+    validated_queue = true;
+    entry->validated_queue.erase(entry->validated_queue.begin());
+  } else {
+    // Promote next transaction from the pending queue.
+    next = entry->pending_queue.front();
+    if ((next->mode() & Transaction::WRITE) && !entry->readers.empty())
+      return;  // Have to wait.
+    entry->pending_queue.erase(entry->pending_queue.begin());
+  }
+
+  int rv = AddTransactionToEntry(entry, next, validated_queue);
+  if (rv != ERR_IO_PENDING) {
+    next->io_callback().Run(rv);
+  }
+}
+
+void HttpCache::ProcessParallelValidation(ActiveEntry* entry) {
+  if (entry->will_process_parallel_validation)
+    return;
+
+  entry->will_process_parallel_validation = true;
+
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::Bind(&HttpCache::OnProcessParallelValidation,
+                            GetWeakPtr(), entry->disk_entry->GetKey(), entry));
+}
+
+void HttpCache::OnProcessParallelValidation(const std::string& key,
+                                            ActiveEntry* entry) {
+  // Check if the entry is still active.
+  if (!IsEntryActive(key, entry))
+    return;
+
+  entry->will_process_parallel_validation = false;
+
+  // A ProcessPendingQueue task before this may add a validating_transaction. In
+  // that case, do nothing.
+  if (entry->validating_transaction)
+    return;
+
+  // If no one is interested in this entry, then we can deactivate it.
+  if (entry->HasNoTransactions()) {
     DestroyEntry(entry);
     return;
   }
 
   if (entry->pending_queue.empty())
     return;
 
-  // Promote next transaction from the pending queue.
   Transaction* next = entry->pending_queue.front();
-  if ((next->mode() & Transaction::WRITE) && !entry->readers.empty())
+
+  // Read-only transactions cannot proceed to do validation.
+  if (!(next->mode() & Transaction::WRITE) && entry->writer)
     return;  // Have to wait.
 
   entry->pending_queue.erase(entry->pending_queue.begin());
 
   int rv = AddTransactionToEntry(entry, next);
   if (rv != ERR_IO_PENDING) {
     next->io_callback().Run(rv);
   }
 }
 
+void HttpCache::DoomEntryRestartPendingTransactions(ActiveEntry* entry) {

[Randy Smith (Not in Mondays), 2017/03/07 22:54:30]

This function and the next one look simple and are only called from one place. 
What's your thinking about having them as separate functions rather than
hoisting them into their callers?  (This may be my focussing on API cleanliness
at the expense of other forms, but if so I'd like to understand the other
advantages I'm missing.)

[shivanisha, 2017/03/08 21:42:11]

In the new patch DestroyEntryRestartPendingTransactions() now has 2 callers, so
that's fine. My thinking on having separate functions was to make it easier to
add more callers for them in future which might or might not happen for
subsequent shared writing CLs. So basically , yes, API cleanliness was the major
advantage here.

+  DoomActiveEntry(entry->disk_entry->GetKey());
+  TransactionList list = GetAllPendingTransactions(entry);
+  RestartPendingTransactions(list);
+}
+
+void HttpCache::DestroyEntryRestartPendingTransactions(ActiveEntry* entry) {
+  entry->disk_entry->Doom();
+  TransactionList list = GetAllPendingTransactions(entry);
+  DestroyEntry(entry);
+  RestartPendingTransactions(list);
+}
+
+HttpCache::TransactionList HttpCache::GetAllPendingTransactions(
+    ActiveEntry* entry) {
+  TransactionList list;
+  for (auto* transaction : entry->validated_queue)
+    list.push_back(transaction);
+  entry->validated_queue.clear();
+
+  for (auto* transaction : entry->pending_queue)
+    list.push_back(transaction);
+  entry->pending_queue.clear();
+
+  return list;
+}
+
+void HttpCache::RestartPendingTransactions(const TransactionList& list) {
+  // ERR_CACHE_RACE causes the transaction to restart the whole process.
+  for (auto* transaction : list)
+    transaction->io_callback().Run(ERR_CACHE_RACE);
+}
+
 void HttpCache::OnIOComplete(int result, PendingOp* pending_op) {
   WorkItemOperation op = pending_op->writer->operation();
 
   // Completing the creation of the backend is simpler than the other cases.
   if (op == WI_CREATE_BACKEND)
     return OnBackendCreated(result, pending_op);
 
   std::unique_ptr<WorkItem> item = std::move(pending_op->writer);
   bool fail_requests = false;
 
@@ skipping 124 matching lines @@
     building_backend_ = false;
     DeletePendingOp(pending_op);
   }
 
   // The cache may be gone when we return from the callback.
   if (!item->DoCallback(result, disk_cache_.get()))
     item->NotifyTransaction(result, NULL);
 }
 
 }  // namespace net