Fixes the cache lock issue.

net/http/http_cache_shared_writers.cc

Index: net/http/http_cache_shared_writers.cc
diff --git a/net/http/http_cache_shared_writers.cc b/net/http/http_cache_shared_writers.cc
new file mode 100644
index 0000000000000000000000000000000000000000..e9a9b1e27cd158e29c20dcb90469fd8566ece93d
--- /dev/null
+++ b/net/http/http_cache_shared_writers.cc
@@ -0,0 +1,612 @@
+// Copyright (c) 2016 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 "build/build_config.h"  // For OS_POSIX
+
+#if defined(OS_POSIX)
+#include <unistd.h>
+#endif
+
+#include <algorithm>
+#include <memory>
+#include <utility>
+#include "base/bind.h"
+#include "base/callback_helpers.h"
+#include "base/compiler_specific.h"
+#include "base/format_macros.h"
+#include "base/location.h"
+#include "base/macros.h"
+#include "base/single_thread_task_runner.h"
+#include "net/http/http_cache_data_access.h"
+#include "net/http/http_cache_shared_writers.h"
+#include "net/http/http_cache_transaction.h"
+
+namespace net {
+
+HttpCache::SharedWriters::SharedWriters(
+    HttpCache* cache,
+    ActiveEntry* entry,
+    Transaction* cache_transaction,
+    RequestPriority priority,
+    std::unique_ptr<HttpTransaction> network_transaction)
+    : cache_(cache->GetWeakPtr()),
+      entry_(entry),
+      priority_(priority),
+      weak_factory_(this) {
+  cache_transaction->SetShared();
+  all_writers_.insert(cache_transaction);
+  data_access_ =
+      base::MakeUnique<DataAccess>(std::move(network_transaction), entry);
+  io_callback_ = base::Bind(&HttpCache::SharedWriters::OnIOComplete,
+                            weak_factory_.GetWeakPtr());
+}
+
+HttpCache::SharedWriters::~SharedWriters() {}
+
+bool HttpCache::SharedWriters::AddTransaction(Transaction* transaction) {
+  transaction->SetShared();
+
+  if (!validating_transaction_) {
+    validating_transaction_ = transaction;

[Randy Smith (Not in Mondays), 2017/01/19 00:53:43]

How hard would it be to move the validation work into SharedWriters?  It feels
weird to have SharedWriters tracking the validation queue, but not actually
manage the validation.

[shivanisha, 2017/01/19 21:13:06]

Moving the validation work would mean moving the logic of states after
STATE_ADD_TO_ENTRY_COMPLETE and before STATE_NETWORK_READ. We probably do not
want all that to be part of SW anyways. Conceptually I look at SW as mainly
pertaining to response body read/write but it also needs to keep track of any
simultaneous transaction in the headers phase to enable that transaction to
ultimately join the shared writing.

[Randy Smith (Not in Mondays), 2017/01/20 19:54:31]

Ok .... I can only repeat that this feels weird to me, since it means the
responsibilities between the classes aren't sharply defined (SW has a
synchronization responsibility around the validation requests but doesn't
actually do the validation requests, but it does manage the network transaction
for the read/write portion of the process).  I'm not going to be in a position
to dig down into what the implementation is and should be, so I won't make any
definitive statements as to what the right thing to do is, but I'd recommend
looking at pulling the synchronization among the validation requests out of
SharedWriters (can ActiveEntry do it?  Does it need to be serialized, or can you
simply race many different network transactions?) or moving the managing of the
validation into SharedWriters.  But up to you and the other reviewers.

[shivanisha, 2017/01/25 19:46:13]

I hear your concerns and that this means SW has an additional responsibility
apart from synchronizing and managing the response body read. The validations
need to be done one at a time so that one transaction can take the
responsibility of dooming the entry and create a new SharedWriters object and
subsequent transactions can be added to it as well. Having one transaction is in
line with the invariant that the lock still exists till the headers are read.
ActiveEntry has an invariant of at most 1 writer and moving this to ActiveEntry
will violate that. It's also important for SW to be aware of these transactions
so it can take certain decisions like whether to remain alive or not when the
response is completely written based on whether there is a validating
transaction or not.

[shivanisha, 2017/01/25 19:50:09]

Other reviewers, any thoughts on this?

+    return true;
+  }
+
+  waiting_for_validation_.push_back(transaction);
+  return false;
+}
+
+bool HttpCache::SharedWriters::empty() {
+  int count = all_writers_.size() + waiting_for_validation_.size() +
+              (validating_transaction_ ? 1 : 0);
+  return count ? false : true;
+}
+
+int HttpCache::SharedWriters::DoLoop(int result) {
+  DCHECK(next_state_ != STATE_NONE);
+
+  int rv = result;
+
+  do {
+    State state = next_state_;
+    next_state_ = STATE_NONE;
+    switch (state) {
+      case STATE_NETWORK_READ:
+        DCHECK_EQ(OK, rv);
+        rv = DoNetworkRead();
+        break;
+      case STATE_NETWORK_READ_COMPLETE:
+        rv = DoNetworkReadComplete(rv);
+        break;
+      case STATE_CACHE_WRITE_DATA:
+        rv = DoCacheWriteData(rv);
+        break;
+      case STATE_CACHE_WRITE_DATA_COMPLETE:
+        rv = DoCacheWriteDataComplete(rv);
+        break;
+      case STATE_CACHE_WRITE_TRUNCATED_RESPONSE:
+        rv = DoCacheWriteTruncatedResponse();
+        break;
+      case STATE_CACHE_WRITE_TRUNCATED_RESPONSE_COMPLETE:
+        rv = DoCacheWriteTruncatedResponseComplete(rv);
+        break;
+      default:
+        NOTREACHED() << "bad state";
+        rv = ERR_FAILED;
+        break;
+    }
+  } while (rv != ERR_IO_PENDING && next_state_ != STATE_NONE);
+
+  if (rv != ERR_IO_PENDING && !callback_.is_null()) {
+    read_buf_ = NULL;  // Release the buffer before invoking the callback.
+    base::ResetAndReturn(&callback_).Run(rv);
+  }
+
+  return rv;
+}
+
+int HttpCache::SharedWriters::Read(scoped_refptr<IOBuffer> buf,
+                                   int buf_len,
+                                   const CompletionCallback& callback,
+                                   Transaction* transaction,
+                                   bool* read_in_progress) {
+  DCHECK(buf);
+  DCHECK_GT(buf_len, 0);
+  DCHECK(!callback.is_null());
+
+  // If another transaction is already reading from the network, then this
+  // transaction waits for the read to complete and gets its buffer filled
+  // with the data returned from that read.
+  if (current_writer_) {
+    WaitingWriter waiting_writer(transaction, buf, buf_len);
+    waiting_writers_.push_back(waiting_writer);
+    *read_in_progress = true;

[Randy Smith (Not in Mondays), 2017/01/19 00:53:43]

So this strikes me as problematic API design--along one asynchronous path out of
this routine (no current_writer_), the callback passed in is called, and along
the other (this one) we instead delegate to the cache to call a callback
associated with the transaction.  Both pathways should use the same callback
mechanism; they're both telling the consumer "Your data is ready".

[shivanisha, 2017/01/19 21:13:06]

The difference in the design between the 2 cases is because in this case, its
possible that SW is destroyed after posting the tasks. Since we can be sure of
HttpCache being alive the responsibility is delegated to it. I wanted to avoid
invoking callbacks as a function call instead of posting individual tasks for
all transactions to avoid the complexities of going through the state flows of
various transactions in a single control flow.

[Randy Smith (Not in Mondays), 2017/01/20 19:54:31]

Let's talk this through in a VC--I'm having a lot of reactions, the #1 being
that it's still really not the right API and we should find a way to fix it. 
But the SW must be alive at the time of ProcessWaitingWriters, and if it posts a
task consisting of just the passed in callback (i.e. one that isn't dependent on
a SW weak pointer), there's no reason that shouldn't succeed.  So there's
something about this response I'm missing.

[shivanisha, 2017/01/25 19:46:13]

Done. Discussed this with Randy over VC that it is possible to use a
CompletionCallback with PostTask.

+    return ERR_IO_PENDING;
+  }
+
+  DCHECK_EQ(next_state_, STATE_NONE);
+  DCHECK(callback_.is_null());
+
+  current_writer_ = transaction;
+
+  read_buf_ = std::move(buf);
+  io_buf_len_ = buf_len;
+
+  next_state_ = STATE_NETWORK_READ;
+  int rv = DoLoop(OK);
+
+  if (rv == ERR_IO_PENDING) {
+    DCHECK(callback_.is_null());
+    callback_ = callback;
+  }
+  return rv;
+}
+
+int HttpCache::SharedWriters::DoNetworkRead() {
+  next_state_ = STATE_NETWORK_READ_COMPLETE;
+  return data_access_->Read(read_buf_, io_buf_len_, io_callback_);
+}
+
+int HttpCache::SharedWriters::DoNetworkReadComplete(int result) {
+  // Remember at this point current_writer_ may or may not be alive.
+  if (result < 0) {
+    // Empty SharedWriters of all transactions.
+    OnNetworkReadFailure(result);
+    return result;
+  }
+
+  if (result == 0) {
+    // Check if the response is actually completed or if not, attempt to mark
+    // the entry as truncated.
+    if (cache_->IsResponseCompleted(
+            entry_, data_access_->network_transaction_->GetResponseInfo())) {
+      ProcessWaitingWriters(result);
+      ResponseDataComplete();
+    } else {
+      OnNetworkReadFailure(result);
+    }
+  } else {  // Successful non zero response.
+    // if no consumer exists, then invoke cache write itself.
+    if (!current_writer_)
+      next_state_ = STATE_CACHE_WRITE_DATA;

[Randy Smith (Not in Mondays), 2017/01/19 00:53:42]

Thought: It occurs to me that it might be useful from a performance POV to race
writing the data back to the cache with returning it to the consumer, but it
complicates the state machine.  OTOH, it might make the responsibilities between
the consumer and the SharedWriters object simpler; the consumer(s) just call
Read() as appropriate and get responses back, and doesn't need to worry about
writing data to the Cache; SharedWriters takes care of all that and hides the
details from the consumer.  I'm inclined to think this is worth exploring if it
makes the SharedWriters API simpler, and not if it doesn't, but I haven't gotten
far enough with this CL to evaluate that.

[shivanisha, 2017/01/20 16:00:09]

I remember talking to others about it and that SimplaCache actually does not
wait for data to be written to the cache before returning. I will confirm with
Gavin, though.

+  }
+
+  return result;
+}
+
+void HttpCache::SharedWriters::OnNetworkReadFailure(int result) {
+  FailureCleanup(result, false);
+
+  if (cache_->CanResumeEntry(
+          true, "GET", data_access_->network_transaction_->GetResponseInfo(),
+          entry_)) {
+    next_state_ = STATE_CACHE_WRITE_TRUNCATED_RESPONSE;
+    network_read_rv_ = result;
+  } else {
+    cache_->ResponseFailedSharedWriters(false, entry_);
+  }
+}
+
+int HttpCache::SharedWriters::DoCacheWriteTruncatedResponse() {
+  next_state_ = STATE_CACHE_WRITE_TRUNCATED_RESPONSE_COMPLETE;
+  return cache_->WriteResponseInfo(
+      entry_, data_access_->network_transaction_->GetResponseInfo(),
+      io_callback_, true, &io_buf_len_);
+}
+
+int HttpCache::SharedWriters::DoCacheWriteTruncatedResponseComplete(
+    int result) {
+  bool success = true;
+  if (result != io_buf_len_) {
+    DLOG(ERROR) << "failed to write response info to cache";
+    success = false;
+  }
+  // This object should be destroyed in the end of this flow.
+  destroy_ = true;

[Randy Smith (Not in Mondays), 2017/01/19 00:53:43]

Hmmm.  Setting destroy_ to true means that on the next OnIOComplete() after its
set, *after* the DoLoop call, the object will be destroyed.  To me this means
that destroy_ should only be set at the same time as an async method is
dispatched from this object, so that you can be sure that OnIOComplete() will be
called.  But if this routine is being executed inside of a DoLoop invoked by an
async response from, e.g., writing the cache response info, it seems like the
object will leak.  What am I missing?

[shivanisha, 2017/01/25 19:46:13]

Yup, simplified the destruction logic s.t. destroy_ is no longer needed.

+
+  cache_->ResponseFailedSharedWriters(success, entry_);
+
+  // Return the return value returned from the network read to the comsumer.
+  return network_read_rv_;
+}
+
+int HttpCache::SharedWriters::CacheWrite(scoped_refptr<IOBuffer> buf,
+                                         int write_len,
+                                         const CompletionCallback& callback,
+                                         Transaction* transaction) {
+  DCHECK_EQ(next_state_, STATE_NONE);
+  DCHECK(buf);
+  DCHECK_GE(write_len, 0);
+  DCHECK(callback_.is_null());
+  DCHECK(!callback.is_null());
+  DCHECK_EQ(current_writer_, transaction);
+
+  read_buf_ = std::move(buf);
+  next_state_ = STATE_CACHE_WRITE_DATA;
+  int rv = DoLoop(write_len);
+
+  if (rv == ERR_IO_PENDING) {
+    DCHECK(callback_.is_null());
+    callback_ = callback;
+  }
+
+  return rv;
+}
+
+int HttpCache::SharedWriters::DoCacheWriteData(int num_bytes) {
+  next_state_ = STATE_CACHE_WRITE_DATA_COMPLETE;
+  write_len_ = num_bytes;
+  return data_access_->CacheWrite(read_buf_, num_bytes, io_callback_);
+}
+
+int HttpCache::SharedWriters::DoCacheWriteDataComplete(int result) {
+  if (result != write_len_)
+    // Need to take care of all the transactions in SharedWriters and
+    // delete SharedWriters as without the cache, we cannot continue the shared
+    // logic.
+    OnCacheWriteFailure();
+  else
+    OnCacheWriteSuccess(result);
+
+  return result;
+}
+
+void HttpCache::SharedWriters::OnCacheWriteSuccess(int result) {
+  // Save the data in all the waiting transactions' read buffers.
+  for (auto it = waiting_writers_.begin(); it != waiting_writers_.end(); it++) {
+    it->write_len = std::min(it->read_buf_len, result);
+    memcpy(it->read_buf->data(), read_buf_->data(), it->write_len);
+  }
+  // Notify waiting_writers_. Tasks will be posted for all the
+  // transactions.
+  ProcessWaitingWriters(write_len_);
+
+  if (result > 0) {  // not the end of response
+    current_writer_ = nullptr;
+    return;
+  }
+
+  DCHECK_EQ(result, 0);
+
+  ResponseDataComplete();
+}
+
+void HttpCache::SharedWriters::ResponseDataComplete() {
+  ResetCurrentWriter();
+
+  // If there is a transaction validating currently, return.
+  if (validating_transaction_)
+    return;
+
+  // Else empty the SharedWriters object.
+  MoveIdleWritersToReaders();
+  DCHECK(all_writers_.empty());
+
+  MoveToPendingQueue();
+
+  destroy_ = true;
+
+  // Inform cache_ so it can take care of entry_.
+  cache_->ResponseCompleteSharedWriters(entry_);
+}
+
+void HttpCache::SharedWriters::OnCacheWriteFailure() {
+  Transaction* current_writer = current_writer_;
+
+  // Needs data_access_ to be valid so needs to be invoked before
+  // ContinueWithoutSharedWriting.
+  FailureCleanup(ERR_CACHE_WRITE_FAILURE, true);
+
+  destroy_ = true;
+
+  if (current_writer)  // If the transaction is still alive in this callback.
+    current_writer->ContinueWithoutSharedWriting(std::move(data_access_),
+                                                 false);
+
+  // Inform cache_ so it can take care of entry_.
+  cache_->ResponseFailedSharedWriters(false, entry_);
+}
+
+void HttpCache::SharedWriters::MoveIdleWritersToReaders() {
+  // Should be invoked after waiting_writers_ are all processed so that
+  // all_writers_ only contains the idle writers.
+  DCHECK(waiting_writers_.empty());
+  DCHECK(!current_writer_);
+  for (auto idle_writer : all_writers_) {
+    entry_->readers.insert(idle_writer);
+    idle_writer->ResetShared(false, true);
+  }
+  all_writers_.clear();
+}
+
+void HttpCache::SharedWriters::ProcessWaitingWriters(int result) {
+  for (auto it = waiting_writers_.begin(); it != waiting_writers_.end(); it++) {
+    Transaction* transaction = it->transaction;
+
+    if (result > 0) {  // success
+      // Fill result with the length of buffer filled for this transaction which
+      // may be different from the transaction that actually wrote to the cache
+      // based on the buffer size.
+      result = it->write_len;
+    } else {
+      // If its response completion or failure, this transaction needs to be
+      // removed.
+      transaction->ResetShared();
+      all_writers_.erase(transaction);
+    }
+
+    // Post task to notify transaction.
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(&HttpCache::NotifyTransaction, cache_->GetWeakPtr(),
+                   transaction->GetWeakPtr(), result));
+  }
+
+  waiting_writers_.clear();
+}
+
+void HttpCache::SharedWriters::FailureCleanup(int error,
+                                              bool continue_network_reading) {
+  ResetCurrentWriter(continue_network_reading);
+
+  // Notify waiting_writers_ of the failure. Tasks will be posted for all the
+  // transactions.
+  ProcessWaitingWriters(error);
+
+  // Idle readers should know to fail when Read is invoked by their consumers.
+  SetIdleWritersFailState(error);
+  DCHECK(all_writers_.empty());
+
+  // If there exists a validating_transaction_, it may be waiting
+  // to read response headers from the cache or waiting for receiving
+  // validation response from the network. In both scenarios, it should be safe
+  // to fail.
+  if (validating_transaction_) {
+    validating_transaction_->SetSharedWritingFailState(error);
+    validating_transaction_->ResetShared(true);
+    validating_transaction_ = nullptr;
+  }
+
+  MoveToPendingQueue();
+}
+
+bool HttpCache::SharedWriters::StopCaching(Transaction* transaction) {
+  // If this is the only transaction in SharedWriters either in validation or
+  // reading stage, then stopping will be successful. If not, then we will not
+  // stop caching since there are other consumers waiting to read from the
+  // cache.
+  bool result = false;
+  if (transaction == validating_transaction_) {
+    if (all_writers_.empty()) {
+      result = true;
+      validating_transaction_ = nullptr;
+    }
+  } else if (all_writers_.size() == 1 && all_writers_.count(transaction) &&
+             !validating_transaction_) {
+    if (current_writer_ == transaction) {
+      current_writer_ = nullptr;
+    }
+    all_writers_.erase(transaction);
+    result = true;
+  }
+  if (result) {
+    transaction->ContinueWithoutSharedWriting(std::move(data_access_), true);
+    entry_->writer = transaction;
+    MoveToPendingQueue();
+  }
+  return result;
+}
+
+void HttpCache::SharedWriters::ResetCurrentWriter(
+    bool continue_network_reading) {
+  // If current_writer_ is already destroyed, return.
+  if (!current_writer_)
+    return;
+  current_writer_->ResetShared(continue_network_reading);
+  all_writers_.erase(current_writer_);
+  current_writer_ = nullptr;
+}
+
+void HttpCache::SharedWriters::SetIdleWritersFailState(int result) {
+  // Since this is only for idle transactions, all waiting_writers_ and
+  // current_writer_ should be empty.
+  DCHECK(waiting_writers_.empty());
+  DCHECK(!current_writer_);
+
+  for (auto transaction : all_writers_) {
+    transaction->SetSharedWritingFailState(result);
+    transaction->ResetShared();
+  }
+
+  all_writers_.clear();
+}
+
+void HttpCache::SharedWriters::OnValidationMatch(Transaction* transaction,
+                                                 RequestPriority priority) {
+  DCHECK_EQ(validating_transaction_, transaction);
+  ValidationDoneContinue(transaction, priority);
+}
+
+void HttpCache::SharedWriters::ValidationDoneContinue(
+    Transaction* transaction,
+    RequestPriority priority) {
+  validating_transaction_ = nullptr;
+  if (priority > priority_) {
+    data_access_->network_transaction_->SetPriority(priority);
+    priority_ = priority;
+  }
+  all_writers_.insert(transaction);
+  ProcessFirstWaitingValidation();
+}
+
+void HttpCache::SharedWriters::ProcessFirstWaitingValidation() {
+  if (!waiting_for_validation_.empty() || validating_transaction_)
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(&HttpCache::SharedWriters::OnProcessFirstWaitingValidation,
+                   weak_factory_.GetWeakPtr()));
+}
+
+void HttpCache::SharedWriters::OnProcessFirstWaitingValidation() {
+  if (waiting_for_validation_.empty() && !validating_transaction_)
+    return;
+
+  Transaction* transaction = nullptr;
+  if (validating_transaction_) {
+    transaction = validating_transaction_;
+  } else {
+    transaction = waiting_for_validation_.front();
+    waiting_for_validation_.erase(waiting_for_validation_.begin());
+    validating_transaction_ = transaction;
+  }
+  transaction->io_callback().Run(OK);
+}
+
+std::unique_ptr<HttpTransaction> HttpCache::SharedWriters::OnValidationNoMatch(
+    Transaction* transaction,
+    std::unique_ptr<HttpTransaction> network_transaction,
+    RequestPriority priority) {
+  DCHECK_EQ(validating_transaction_, transaction);
+  // If there is no transaction in all_writers_, its ok to rewrite the entry
+  // response.
+  if (all_writers_.empty()) {
+    data_access_.reset(new DataAccess(std::move(network_transaction), entry_));
+    ValidationDoneContinue(transaction, priority);
+    return std::unique_ptr<HttpTransaction>();
+  }
+
+  transaction->ResetShared();
+  validating_transaction_ = nullptr;
+  MoveToPendingQueue();
+  return network_transaction;
+}
+
+void HttpCache::SharedWriters::DoneReading(Transaction* transaction) {
+  // If current_writer_ is set, then wait for current_writer_ to detect the end
+  // of stream.
+  if (current_writer_) {
+    return;
+  }
+  DCHECK(waiting_writers_.empty());
+  RemoveIdleWriter(transaction);
+  // If there is a transaction validating currently, return.
+  if (validating_transaction_) {
+    return;
+  }
+
+  // Else empty the SharedWriters object.
+  MoveIdleWritersToReaders();
+  DCHECK(all_writers_.empty());
+
+  MoveToPendingQueue();
+}
+
+void HttpCache::SharedWriters::RemoveIdleWriter(Transaction* transaction) {
+  // The transaction should be part of all_writers.
+  auto it = all_writers_.find(transaction);
+  DCHECK(it != all_writers_.end());
+  all_writers_.erase(transaction);
+  transaction->ResetShared();
+}
+
+void HttpCache::SharedWriters::RemoveWaitingWriter(Transaction* transaction) {
+  auto it = waiting_writers_.begin();
+  for (; it != waiting_writers_.end(); it++) {
+    if (transaction == it->transaction) {
+      waiting_writers_.erase(it);
+      all_writers_.erase(transaction);
+      transaction->ResetShared();
+      // If a waiting_writer_ existed, there should have been a current_writer_.
+      DCHECK(current_writer_);
+      break;
+    }
+  }
+}
+
+void HttpCache::SharedWriters::RemoveValidatingTransaction(
+    Transaction* transaction) {
+  DCHECK_EQ(validating_transaction_, transaction);
+  validating_transaction_ = nullptr;
+  transaction->ResetShared();
+  ProcessFirstWaitingValidation();
+}
+
+bool HttpCache::SharedWriters::RemoveWaitingTransaction(
+    Transaction* transaction) {
+  auto it = std::find(waiting_for_validation_.begin(),
+                      waiting_for_validation_.end(), transaction);
+  if (it != waiting_for_validation_.end()) {
+    transaction->ResetShared();
+    waiting_for_validation_.erase(it);
+    return true;
+  }
+  return false;
+}
+
+void HttpCache::SharedWriters::RemoveCurrentWriter(Transaction* transaction) {
+  DCHECK_EQ(current_writer_, transaction);
+  ResetCurrentWriter();
+  callback_.Reset();
+}
+
+void HttpCache::SharedWriters::MoveFromPendingQueue() {
+  auto it = entry_->pending_queue.begin();
+  while (it != entry_->pending_queue.end()) {
+    Transaction* transaction = *it;
+    if (transaction->IsEligibleForSharedWriting()) {
+      transaction->SetShared();
+      waiting_for_validation_.push_back(transaction);
+      it = entry_->pending_queue.erase(it);
+    } else {
+      ++it;
+    }
+  }
+}
+
+void HttpCache::SharedWriters::MoveToPendingQueue() {
+  // For maintaining the order of the transactions as they arrived, append
+  // these to the front of the pending_queue. Note that the order is preserved
+  // only among the transactions that are eligible for sharing. For others, they
+  // may have arrived earlier but may be processed later which is fair since
+  // they have to anyways wait till the entry is written to the cache.
+  while (!waiting_for_validation_.empty()) {
+    Transaction* transaction = waiting_for_validation_.back();
+    transaction->ResetShared(true);
+    entry_->pending_queue.push_front(transaction);
+    waiting_for_validation_.pop_back();
+  }
+}
+
+bool HttpCache::SharedWriters::CanReset() {
+  return !destroy_;
+}
+
+HttpCache::SharedWriters::WaitingWriter::WaitingWriter(
+    Transaction* cache_transaction,
+    scoped_refptr<IOBuffer> buf,
+    int len)
+    : transaction(cache_transaction),
+      read_buf(std::move(buf)),
+      read_buf_len(len),
+      write_len(0) {}
+
+HttpCache::SharedWriters::WaitingWriter::~WaitingWriter() {}
+
+HttpCache::SharedWriters::WaitingWriter::WaitingWriter(const WaitingWriter&) =
+    default;
+
+void HttpCache::SharedWriters::OnIOComplete(int result) {
+  bool destroy = destroy_;
+  DoLoop(result);
+
+  // Only check the local variable as this object may have been destroyed if
+  // destroy_ was not set to true.
+  if (destroy)
+    SelfDestroy();

[Randy Smith (Not in Mondays), 2017/01/19 00:53:43]

Worth a DCHECK that DoLoop() didn't return ERR_IO_PENDING?  Or is that ok?

[shivanisha, 2017/01/25 19:46:13]

N/A now that self destruction is not needed.

+}
+
+void HttpCache::SharedWriters::SelfDestroy() {

[Randy Smith (Not in Mondays), 2017/01/19 00:53:43]

I don't think there's a need for a separate function.

[shivanisha, 2017/01/25 19:46:13]

N/A now that self destruction is not needed.

+  delete this;
+}
+
+}  // namespace net