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 34 matching lines @@
 #include "net/http/http_response_headers.h"
 #include "net/http/http_response_info.h"
 #include "net/http/http_util.h"
 #include "net/log/net_log_with_source.h"
 #include "net/quic/chromium/quic_server_info.h"
 
 #if defined(OS_POSIX)
 #include <unistd.h>
 #endif
 
+// Helper methods.
+namespace {
+void RestartPendingTransactions(const net::HttpCache::TransactionList& list) {

[Randy Smith (Not in Mondays), 2017/03/17 18:13:05]

It looks to me as if this function is the reason why TransactionList and
TransactionSet are now public on HttpCache.  Is that correct?  If so, it doesn't
strike me as worth it--I think more restricted access would be valuable enough
to make it worth either making this function a static member of HttpCache or
hoisting it's usage into the calling locations (it isn't particularly long). 
WDYT?  

I'm not invested in the change, but do prefer private to public for all the
usual reasons so I'd like to understand the counter argument before accepting
it.

[shivanisha, 2017/03/20 16:34:51]

Good catch, since this change not only made TransactionList and TransactionSet
public but also the Transaction class itself. I agree, its not worth it. Making
the logic here inline in the calling location and reverting the private scopes
of Transaction, TransactionList and TransactionSet.

+  // ERR_CACHE_RACE causes the transaction to restart the whole process.
+  for (auto* transaction : list)
+    transaction->io_callback().Run(net::ERR_CACHE_RACE);

[Randy Smith (Not in Mondays), 2017/03/17 18:13:05]

To me, calling a callback on a transaction is saying to a consumer "I've
completed the work you requested on this transaction, and this is the result". 
Given that it's effectively handing responsibility for the transactions back to
the consumer, I'd like it to be explicitly coupled with removing any references
to the transaction at this level.  Would you be willing to rewrite this routine
so it modifies the lists passed to it s.t. the transactions for which the
io_callback() is being called are removed from the passed in list?

[shivanisha, 2017/03/20 16:34:51]

This routine is now removed and callback is invoked in the calling location.
Renamed GetAllPendingTransactions as RemoveAllQueuedTransactions so that it is
clear that the function is also clearing the queues and also clarified in its
documentation.

+}
+
+}  // namespace
+
 namespace net {
 
 HttpCache::DefaultBackend::DefaultBackend(
     CacheType type,
     BackendType backend_type,
     const base::FilePath& path,
     int max_bytes,
     const scoped_refptr<base::SingleThreadTaskRunner>& thread)
     : type_(type),
       backend_type_(backend_type),
@@ skipping 24 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.
+  // and |add_to_entry_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() && add_to_entry_queue.empty() &&
+         done_headers_queue.empty() && !headers_transaction;
+}
+
+bool HttpCache::ActiveEntry::HasNoActiveTransactions() {
+  return !writer && readers.empty() && !headers_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 queued transactions ,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->pending_queue.clear();
+    entry->will_process_queued_transactions = false;
+    entry->add_to_entry_queue.clear();
     entry->readers.clear();
-    entry->writer = NULL;
+    entry->done_headers_queue.clear();
+    entry->headers_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 239 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() ||
-         entry_ptr->will_process_pending_queue);
+         entry_ptr->headers_transaction ||
+         entry_ptr->will_process_queued_transactions);
   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;
 }
 
 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);
+  DCHECK(!entry->will_process_queued_transactions);
   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) {
   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).
-
-  if (entry->writer || entry->will_process_pending_queue) {
-    entry->pending_queue.push_back(trans);
-    return ERR_IO_PENDING;
-  }
-
-  if (trans->mode() & Transaction::WRITE) {
-    // transaction needs exclusive access to the entry
-    if (entry->readers.empty()) {
-      entry->writer = trans;
+  // Always add a new transaction to the queue to maintain FIFO order.
+  entry->add_to_entry_queue.push_back(transaction);
+  ProcessQueuedTransactions(entry);
+  return ERR_IO_PENDING;
+}
+
+void HttpCache::ProcessAddToEntryTransaction(ActiveEntry* entry) {
+  if (entry->headers_transaction) {
+    // Note the entry may be new or may already have a response body written to
+    // it. In both cases, a transaction with write mode set, needs to wait
+    // because only one transaction is allowed to be validating at a time.
+    // If transaction is read-only and entry is not yet complete, then it
+    // needs to wait. If entry is complete, it needs to wait because its
+    // possible that the headers_transaction dooms the entry if its not a match.

[Randy Smith (Not in Mondays), 2017/03/17 18:13:05]

Suggestion: Based on the state transitions that you document in the header file,
this conditional is obvious--the next step for transactions in the
add_to_entry_queue is to headers_transaction, and if that slot is filled, the
transition can't occur.  So I'd think this comment belongs in documenting that
state transition rather than here?

[shivanisha, 2017/03/20 16:34:52]

I agree that this comment is basically explaining "why" a transaction should
wait when headers_transaction slot is filled. I wrote this here mainly to
emphasize nuances of this situation like "the entry may be new or may already
have a response body written to it" and answering questions like : why a
read-only transaction can't start reading the already written response. Since
these are implementation specific aspects, it seemed better for a reader to read
while understanding the code in this function.

+    return;
+  }
+
+  Transaction* transaction = entry->add_to_entry_queue.front();
+  if (entry->writer) {
+    // A transaction either starts validation or waits.

[shivanisha, 2017/03/16 00:54:54]

"A transaction either starts headers phase or waits."

[shivanisha, 2017/03/20 16:34:51]

done

+    if (transaction->mode() & Transaction::WRITE) {
+      entry->headers_transaction = transaction;

[Randy Smith (Not in Mondays), 2017/03/17 18:13:05]

Suggestion: Hoist this conditional up above the if(entry->writer) conditional
above?  By the state transitions sketched out in the header, the next step for a
WRITE mode transaction is the headers_transaction, so there isn't a conceptual
need to distinguish between whether there's a writer or not?  That's only needed
if there's a reader. 

[shivanisha, 2017/03/20 16:34:51]

Here I wanted to emphasize the conceptual differences between the 2 states when
there is a writer or not, namely, either the entry is being written to (there is
a writer) or during the start of an entry or when the response is already
written (writer does not exist)

     } else {
-      entry->pending_queue.push_back(trans);
-      return ERR_IO_PENDING;
+      return;
     }
   } else {
-    // transaction needs read access to the entry
-    entry->readers.insert(trans);
-  }
-
-  // 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())
-    ProcessPendingQueue(entry);
-
-  return OK;
-}
-
-void HttpCache::DoneWithEntry(ActiveEntry* entry, Transaction* trans,
+    // This state may be reached either during the start of an entry or when the
+    // response is written.
+    if (transaction->mode() & Transaction::WRITE) {
+      entry->headers_transaction = transaction;
+    } else {
+      // A read-only transaction can start reading the response.
+      // If a read transaction is here, then it has to be an already created
+      // entry as the other case is already handled in the Transaction state
+      // machine.
+      entry->readers.insert(transaction);
+
+      // More readers can join too.
+      ProcessQueuedTransactions(entry);

[Randy Smith (Not in Mondays), 2017/03/17 18:13:05]

Suggestion: This routine is already executed after a PostTask; is another one
needed?  I.e. can this be handled with a loop in the calling routine?

[shivanisha, 2017/03/20 16:34:51]

Since additional queued transactions should only be processed in case of
readers, adding it here makes the complete logic reside in the same place.

+    }
+  }
+
+  entry->add_to_entry_queue.erase(entry->add_to_entry_queue.begin());
+  transaction->io_callback().Run(OK);
+}
+
+void HttpCache::ProcessDoneHeadersTransaction(ActiveEntry* entry) {
+  DCHECK(!entry->writer && !entry->done_headers_queue.empty() &&
+         !entry->headers_transaction);
+  Transaction* transaction = entry->done_headers_queue.front();
+  // If this transaction is responsible for writing the response body.
+  if (transaction->mode() & Transaction::WRITE) {
+    DCHECK_EQ(entry->done_headers_queue.size(), (size_t)1);
+    entry->writer = transaction;
+  } else {
+    // Response body is already written, convert to a reader.
+    auto return_val = entry->readers.insert(transaction);
+    DCHECK_EQ(return_val.second, true);
+  }
+  // Post another task to give a chance to more transactions to either join
+  // readers or another transaction to start parallel validation.
+  ProcessQueuedTransactions(entry);
+  entry->done_headers_queue.erase(entry->done_headers_queue.begin());
+  transaction->io_callback().Run(OK);

[Randy Smith (Not in Mondays), 2017/03/17 18:13:05]

Help me understand why this callback is being run?  I presume we want to run
these callbacks when some consumer visible change occurs on the transaction. 
But the consumer visible change in this particular case is that we're done with
the headers (right?), so shouldn't this be in the code that triggers the queue
processing (because it has to do with the effect on the single transaction, not
on the other transactions, and because it's associated with the transition from
headers_transaction -> done_headers_queue, not from done_headers_queue onwards?)

[shivanisha, 2017/03/20 16:34:51]

This is indeed the callback of the "other" transaction which was earlier in
done_headers_queue and now taken out of it.

+}
+
+int HttpCache::DoneResponseHeaders(ActiveEntry* entry,
+                                   Transaction* transaction,
+                                   bool is_match) {
+  DCHECK_EQ(entry->headers_transaction, transaction);
+  entry->headers_transaction = nullptr;
+
+  if (!is_match) {
+    // Doom or destroy this entry based on whether it has active consumers.
+    ProcessEntryFailure(entry);
+    return OK;
+  }
+
+  entry->done_headers_queue.push_back(transaction);
+  ProcessQueuedTransactions(entry);
+  return ERR_IO_PENDING;
+}
+
+HttpCache::TransactionList HttpCache::GetAllPendingTransactions(

[shivanisha, 2017/03/16 00:54:53]

will rename this to GetAllQueuedTransactions and send TransactionList& as an
output argument instead of return value to avoid any copies.

[shivanisha, 2017/03/20 16:34:51]

Renamed it to RemoveAllQueuedTransactions to be clear.

+    ActiveEntry* entry) {
+  TransactionList list;
+  for (auto* transaction : entry->done_headers_queue)
+    list.push_back(transaction);
+  entry->done_headers_queue.clear();
+
+  for (auto* transaction : entry->add_to_entry_queue)
+    list.push_back(transaction);
+  entry->add_to_entry_queue.clear();
+
+  return list;
+}
+
+void HttpCache::ProcessEntryFailure(ActiveEntry* entry) {
+  // Failure case is either writer failing to completely write the response to
+  // the cache or validating transaction received a non-304 response.
+  if (entry->HasNoActiveTransactions() &&
+      !entry->will_process_queued_transactions) {
+    entry->disk_entry->Doom();
+    TransactionList list = GetAllPendingTransactions(entry);
+    DestroyEntry(entry);
+    RestartPendingTransactions(list);
+  } else {
+    DoomActiveEntry(entry->disk_entry->GetKey());
+    TransactionList list = GetAllPendingTransactions(entry);
+    RestartPendingTransactions(list);
+  }
+}
+
+void HttpCache::ProcessQueuedTransactions(ActiveEntry* entry) {
+  // Multiple readers may finish with an entry at once, so we want to batch up
+  // calls to OnProcessQueuedTransactions. This flag also tells us that we
+  // should not delete the entry before OnProcessQueuedTransactions runs.
+  if (entry->will_process_queued_transactions)
+    return;
+
+  entry->will_process_queued_transactions = true;
+
+  // Post a task instead of invoking the io callback of another transaction here
+  // to avoid re-entrancy.
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE,
+      base::Bind(&HttpCache::OnProcessQueuedTransactions, GetWeakPtr(), entry));
+}
+
+void HttpCache::OnProcessQueuedTransactions(ActiveEntry* entry) {
+  entry->will_process_queued_transactions = false;
+
+  // If no one is interested in this entry, then we can deactivate it.
+  if (entry->HasNoTransactions()) {
+    DestroyEntry(entry);
+    return;
+  }
+
+  if (entry->done_headers_queue.empty() && entry->add_to_entry_queue.empty())
+    return;
+
+  // To maintain FIFO order of transactions, done_headers_queue should be
+  // processed first, and then add_to_entry_queue.
+  // If another transaction is writing the response, let validated transactions
+  // wait till the response is complete. If the response is not yet started, the
+  // done_headers_queue transaction should start writing it.
+  if (!entry->writer && !entry->done_headers_queue.empty()) {
+    DCHECK(!entry->headers_transaction);

[Randy Smith (Not in Mondays), 2017/03/17 18:13:05]

Why this invariant?   What if we have the writer and the headers_transaction
completing quickly, one after the other, and the PostTasks merging so that when
we reach this point both are empty?

[shivanisha, 2017/03/20 16:34:51]

Good catch, Thanks. Its possible that writer just completed writing to the entry
and is thus null and there is a headers_transaction validating the headers.
Removing this dcheck from here and also from ProcessDoneHeadersTransaction. 

+    ProcessDoneHeadersTransaction(entry);
+    return;
+  }
+
+  if (!entry->add_to_entry_queue.empty())
+    ProcessAddToEntryTransaction(entry);

[Randy Smith (Not in Mondays), 2017/03/17 18:13:05]

Comment (not even a suggestion): If I were writing this code, I would inline
ProcessAddToEntryTransaction and ProcessDoneHeadersTransaction, basically
because I think the virtue of seeing all the queue manipulation code in the same
place outweighs the virtue of having separate functions.  The fact that when
reviewing these functions I found myself repeatedly jumping back and forth
between OnProcessQueuedTransactions and its helpers also biases me this way. 
But I think this is  basically an aesthetic issue, so if you've thought about
the two options and decided that you preferred this, I'm happy with that.

Request: However, I would like to have the functions near each other in the .cc
file.

[shivanisha, 2017/03/20 16:34:51]

Moved the functions to be near each other.

My main motivation to have separate functions is to let the reader have a high
level understanding of the flow from reading OnProcessQueuedTransactions and
then to dig deeper in the state transitions of a transaction by reading
ProcessDoneHeadersTransaction and ProcessAddToEntryTransaction.

+}
+
+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())
-    return;
-
-  if (entry->writer) {
-    DCHECK(trans == entry->writer);
-
+  // Transaction is waiting in the done_headers_queue.
+  if (!entry->done_headers_queue.empty()) {
+    auto i = std::find(entry->done_headers_queue.begin(),
+                       entry->done_headers_queue.end(), transaction);
+    if (i != entry->done_headers_queue.end()) {
+      entry->done_headers_queue.erase(i);
+      return;
+    }
+  }
+
+  // Transaction is removed in the validation phase.

[shivanisha, 2017/03/16 00:54:53]

"Transaction is removed in the headers phase."

[shivanisha, 2017/03/20 16:34:51]

done

+  if (transaction == entry->headers_transaction) {
+    entry->headers_transaction = nullptr;
+    ProcessQueuedTransactions(entry);
+    return;
+  }
+
+  // Transaction is removed in the writing phase.
+  if (transaction == entry->writer) {
     // 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();
+      success = transaction->AddTruncatedFlag();
       // The previous operation may have deleted the entry.
-      if (!trans->entry())
+      if (!transaction->entry())
         return;
     }
-    DoneWritingToEntry(entry, success);
-  } else {
-    DoneReadingFromEntry(entry, trans);
-  }
-}
-
-void HttpCache::DoneWritingToEntry(ActiveEntry* entry, bool success) {
-  DCHECK(entry->readers.empty());
-
-  entry->writer = NULL;
-
-  if (success) {
-    ProcessPendingQueue(entry);
-  } else {
-    DCHECK(!entry->will_process_pending_queue);
-
-    // 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();
-    }
-  }
-}
-
-void HttpCache::DoneReadingFromEntry(ActiveEntry* entry, Transaction* trans) {
+    DoneWritingToEntry(entry, success, transaction);
+    return;
+  }
+
+  // Transaction is reading from the entry.
+  DoneReadingFromEntry(entry, transaction);
+}
+
+void HttpCache::DoneWritingToEntry(ActiveEntry* entry,
+                                   bool success,
+                                   Transaction* transaction) {
+  DCHECK(transaction == entry->writer ||
+         transaction == entry->headers_transaction);
+
+  // Transaction is done writing to entry in the response body phase.
+  if (transaction == entry->writer)
+    entry->writer = nullptr;
+  else
+    entry->headers_transaction = nullptr;
+
+  // If writer fails, all transactions should fail.
+  if (!success && entry->headers_transaction) {
+    entry->headers_transaction->SetValidatingCannotProceed();
+    entry->headers_transaction = nullptr;
+    DCHECK(entry->HasNoActiveTransactions());
+  }
+  if (!success)
+    ProcessEntryFailure(entry);
+  else
+    ProcessQueuedTransactions(entry);
+}
+
+void HttpCache::DoneReadingFromEntry(ActiveEntry* entry,
+                                     Transaction* transaction) {
   DCHECK(!entry->writer);
-
-  auto it = entry->readers.find(trans);
+  auto it = entry->readers.find(transaction);
   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());
-
-  Transaction* trans = entry->writer;
-
-  entry->writer = NULL;
-  entry->readers.insert(trans);
-
-  ProcessPendingQueue(entry);
+  ProcessQueuedTransactions(entry);
 }
 
 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) {
-  TransactionList& pending_queue = entry->pending_queue;
+                                                  Transaction* transaction) {
+  TransactionList& add_to_entry_queue = entry->add_to_entry_queue;
 
-  auto j = find(pending_queue.begin(), pending_queue.end(), trans);
-  if (j == pending_queue.end())
+  auto j =
+      find(add_to_entry_queue.begin(), add_to_entry_queue.end(), transaction);
+  if (j == add_to_entry_queue.end())
     return false;
 
-  pending_queue.erase(j);
+  add_to_entry_queue.erase(j);
   return true;
 }
 
 bool HttpCache::RemovePendingTransactionFromPendingOp(PendingOp* pending_op,
                                                       Transaction* trans) {
   if (pending_op->writer->Matches(trans)) {
     pending_op->writer->ClearTransaction();
     pending_op->writer->ClearEntry();
     return true;
   }
   WorkItemList& pending_queue = pending_op->pending_queue;
 
   for (auto it = pending_queue.begin(); it != pending_queue.end(); ++it) {
     if ((*it)->Matches(trans)) {
       pending_queue.erase(it);
       return true;
     }
   }
   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));
-}
-
-void HttpCache::OnProcessPendingQueue(ActiveEntry* entry) {
-  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->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())
-    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::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