Adds a new class HttpCache::Writers for multiple cache transactions reading from the network.

net/http/http_cache_writers.cc

Index: net/http/http_cache_writers.cc
diff --git a/net/http/http_cache_writers.cc b/net/http/http_cache_writers.cc
new file mode 100644
index 0000000000000000000000000000000000000000..84698cd97c780fedee5b45d4e238800e0461740d
--- /dev/null
+++ b/net/http/http_cache_writers.cc
@@ -0,0 +1,418 @@
+// Copyright (c) 2017 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_writers.h"
+
+#include <algorithm>
+#include <utility>
+#include "net/disk_cache/disk_cache.h"
+#include "net/http/http_cache_transaction.h"
+
+namespace net {
+
+HttpCache::Writers::Writers(HttpCache* cache, ActiveEntry* entry)
+    : cache_(cache), entry_(entry), weak_factory_(this) {
+  io_callback_ =
+      base::Bind(&HttpCache::Writers::OnIOComplete, weak_factory_.GetWeakPtr());
+}
+
+HttpCache::Writers::~Writers() {}
+
+int HttpCache::Writers::Read(scoped_refptr<IOBuffer> buf,
+                             int buf_len,
+                             const CompletionCallback& callback,
+                             Transaction* transaction) {
+  DCHECK(buf);
+  DCHECK_GT(buf_len, 0);
+  DCHECK(!callback.is_null());

[Randy Smith (Not in Mondays), 2017/05/18 01:02:55]

nit, suggestion: DCHECK(transaction)

[shivanisha, 2017/05/18 20:59:44]

done

+
+  // 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 (active_transaction_) {
+    WaitingForRead waiting_transaction(transaction, buf, buf_len, callback);
+    waiting_for_read_.push_back(waiting_transaction);
+    return ERR_IO_PENDING;
+  }
+
+  DCHECK_EQ(next_state_, State::NONE);

[Randy Smith (Not in Mondays), 2017/05/18 01:02:55]

This (and the test above) seem wrong to me--if an active transaction can remove
itself while a network transaction is outstanding, the test above should be for
if a network transaction is outstanding/the state machine is active.

[shivanisha, 2017/05/18 20:59:44]

Ah, that's right. Changed the condition to next_state_ != State::NONE

[Randy Smith (Not in Mondays), 2017/05/22 01:56:55]

I'm not seeing the change (looking at PS6->8 diff)?

[shivanisha, 2017/05/31 19:21:26]

It is present in PS8.

[Randy Smith (Not in Mondays), 2017/06/06 12:54:57]

Ah, sorry, was looking at the DCHECK.  Sorry.

I do think the DCHECK is basically redundant now, and I'd suggest removing it,
but up to you.

[shivanisha, 2017/06/07 15:19:18]

Removed the dcheck.

+  DCHECK(callback_.is_null());

[Randy Smith (Not in Mondays), 2017/05/18 01:02:55]

This feels redundant with the below same DCHECK; do we gain much from having it
in both places?

[shivanisha, 2017/05/18 20:59:44]

removed from below

+  active_transaction_ = transaction;
+
+  read_buf_ = std::move(buf);

[Randy Smith (Not in Mondays), 2017/05/18 01:02:55]

Why std::move on a scoped_refptr<>?  This is just taking another reference,
right?   It's not taking the buffer away from the caller.

[shivanisha, 2017/05/18 20:59:44]

Its just taking the buffer away from the argument which would have anyways
happened when the function ended, (not from the caller). That's how I understood
it would work. Let me know if its wrong.

[Randy Smith (Not in Mondays), 2017/05/24 23:09:19]

Acknowledged.

+  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;
+}
+
+bool HttpCache::Writers::StopCaching(Transaction* transaction) {
+  // If this is the only transaction in Writers, then stopping will be
+  // successful. If not, then we will not stop caching since there are
+  // other consumers waiting to read from the cache.
+  if (all_writers_.size() == 1) {
+    DCHECK(all_writers_.count(transaction));
+    network_read_only_ = true;
+    return true;
+  }
+  return false;
+}
+
+void HttpCache::Writers::AddTransaction(
+    Transaction* transaction,
+    std::unique_ptr<HttpTransaction> network_transaction) {
+  DCHECK(transaction);
+  DCHECK(CanAddWriters());
+
+  auto return_val = all_writers_.insert(transaction);

[Randy Smith (Not in Mondays), 2017/05/18 01:02:55]

nit, suggestion: I don't find the type of return_val intuitive here (i.e. I
don't have the stl interface memorized), so I'd vote in favor of spelling it
out.

[shivanisha, 2017/05/18 20:59:44]

done

+  DCHECK_EQ(return_val.second, true);
+
+  if (network_transaction)
+    network_transaction_ = std::move(network_transaction);

[Randy Smith (Not in Mondays), 2017/05/18 01:02:54]

What drove adding the network transaction here rather than on construction? 
It's only the first HC::T that'll add a network transaction, so having it as
part of the AddTransaction method feels a bit funny.  

[shivanisha, 2017/05/18 20:59:44]

I am hoping writers can be a member variable of ActiveEntry instead of a
unique_ptr and in that case writers will be created even before a transaction is
added to it.

[Randy Smith (Not in Mondays), 2017/05/24 23:09:18]

Hmmm.  Ok.  There's a range of design choices here, and I don't have a strong
feeling that one is better than the other, but keep the other ones (unique_ptr,
separate methods for adding network transaction & HC::Ts, separate methods for
adding the first transaction and adding subsequent ones, this method but
destroying other network transactions when they come in) in mind as
possibilities and make a conscious decision among them.  

[Randy Smith (Not in Mondays), 2017/05/24 23:09:18]

Worthwhile adding a DCHECK that network_transaction_ wasn't already set?

[shivanisha, 2017/05/31 19:21:26]

Done

[shivanisha, 2017/05/31 19:21:26]

Added the dcheck

+  DCHECK(network_transaction_);
+
+  priority_ = std::max(transaction->priority(), priority_);
+  network_transaction_->SetPriority(priority_);
+}
+
+void HttpCache::Writers::RemoveTransaction(Transaction* transaction) {
+  if (!transaction)
+    return;
+
+  // The transaction should be part of all_writers.
+  auto it = all_writers_.find(transaction);
+  DCHECK(it != all_writers_.end());
+  all_writers_.erase(transaction);
+  PriorityChanged();
+
+  if (active_transaction_ == transaction) {
+    active_transaction_ = nullptr;
+    callback_.Reset();

[Randy Smith (Not in Mondays), 2017/05/18 01:02:55]

Is this really all that needs to happen if we're nuking the active transaction? 
 I'm specifically concerned about the case where after a RemoveTransaction() a
new transaction comes along and adds itself and tries to start driving the
(busy) network transaction.  Won't that cause a DCHECK() in read?

[shivanisha, 2017/05/18 20:59:44]

Updated Read to see that if next_state_ is not NONE , consider the network
transaction to be busy.

+    return;
+  }
+
+  auto waiting_it = waiting_for_read_.begin();
+  for (; waiting_it != waiting_for_read_.end(); waiting_it++) {
+    if (transaction == waiting_it->transaction) {
+      waiting_for_read_.erase(waiting_it);
+      // If a waiting transaction existed, there should have been an
+      // active_transaction_.
+      DCHECK(active_transaction_);
+      return;
+    }
+  }
+}
+
+void HttpCache::Writers::RemoveAllTransactions() {
+  all_writers_.clear();

[Randy Smith (Not in Mondays), 2017/05/18 01:02:54]

No need to nuke waiting_for_read_ or do anything with callbacks?

[shivanisha, 2017/05/18 20:59:44]

On second thoughts, I actually do not see a need for this API. Removing it
unless there is a need to do this.

+}
+
+void HttpCache::Writers::PriorityChanged() {
+  RequestPriority current_highest = GetCurrentHighestPriority();

[Randy Smith (Not in Mondays), 2017/05/18 01:02:55]

nit, suggestion: My personal bias is that, if there's only one call site for a
function and that function is fairly short, it should be hoisted into the call
site location, possibly with a comment describing what it does (in place of the
descriptive ability of the function name).

[shivanisha, 2017/05/18 20:59:44]

sgtm, done

+  if (priority_ != current_highest) {
+    network_transaction_->SetPriority(current_highest);
+    priority_ = current_highest;
+  }
+}
+
+void HttpCache::Writers::MoveIdleWritersToReaders() {
+  // Should be invoked after |waiting_for_read_| transactions  and
+  // |active_transaction_| are processed so that all_writers_ only contains idle
+  // writers.

[Randy Smith (Not in Mondays), 2017/05/18 01:02:55]

This reads to me as a requirement on the caller, and hence belonging in the
declaration not the definition?

[shivanisha, 2017/05/18 20:59:44]

done in the comment and also added a function ContainsOnlyIdleWriters() to be
used by the caller.

+  DCHECK(waiting_for_read_.empty());
+  DCHECK(!active_transaction_);
+  for (auto* idle_writer : all_writers_) {
+    entry_->readers.insert(idle_writer);

[Randy Smith (Not in Mondays), 2017/05/18 01:02:54]

Hmm.  I think this is dependent on the cache entry having been fully written,
otherwise the readers are going to call back into Writers::Read(), aren't they? 
Should we assert that somehow, or more closely link moving idle writers to
readers to when we finish filling in the cache entry?

[shivanisha, 2017/05/18 20:59:44]

In CL https://codereview.chromium.org/2721933002/  when HC::T invokes
DoneWritingToEntry(), HttpCache takes care of checking that it is indeed a
headers_transaction or a writer , nulls the corresponding pointer field and then
works on other queued transactions.  

I am envisioning the integration of the parallel validation CL and this CL s.t.
HC::T will still be responsible for invoking  DoneWritingToEntry(), HttpCache
then takes care of checking that it is indeed a headers_transaction or one of
the writers , nulls the corresponding pointer field or calls RemoveTransaction()
on writers and then invokes works on other queued transactions.  As part of
working on other queued transactions, it will also invoke
MoveIdleWritersToReaders().

So the flow of events still stay the same, except there is a set of writers to
work on instead of a single writer and in addition to adding just the
done_headers_queue transactions to readers, it also adds the idle writers. Let
me know how this sounds.

[Randy Smith (Not in Mondays), 2017/05/22 01:56:55]

SGTM.

+  }
+  all_writers_.clear();
+}
+
+bool HttpCache::Writers::CanAddWriters() const {
+  if (all_writers_.empty()) {
+    DCHECK(!network_read_only_);

[Randy Smith (Not in Mondays), 2017/05/18 01:02:54]

Why?

[shivanisha, 2017/05/18 20:59:43]

I believe this should be changed to network_read_only_ = false, to take care of
a race where network_read_only_ was set to true , then all transactions left and
then before entry could be destroyed a new transaction needs to be added, it
should be allowed. Not completely sure of this decision though.

[Randy Smith (Not in Mondays), 2017/05/22 01:56:55]

I'd be more inclined to say that network_read_only_ should be set to false
whenever all_writers_ transitions to empty; at that point the transaction that
set it to true is no longer relevant.  (Which, amusingly, answers my "Why?"
question above--if there aren't any writers, that implies we should have reset
it.  Though it sounds like that's not what you were thinking.)

[shivanisha, 2017/05/31 19:21:26]

Added a function ResetStateForEmptyWriters which is called whenever all_writers_
is empty.

+    return true;
+  }
+
+  return !is_exclusive_ && !network_read_only_;
+}
+
+void HttpCache::Writers::ProcessFailure(Transaction* transaction, int error) {
+  DCHECK(!transaction || transaction == active_transaction_);
+
+  // Notify waiting_for_read_ of the failure. Tasks will be posted for all the
+  // transactions.
+  ProcessWaitingForReadTransactions(error);
+
+  // Idle readers should know to fail when Read is invoked by their consumers.
+  SetIdleWritersFailState(error);

[Randy Smith (Not in Mondays), 2017/05/18 01:02:54]

Why have this logic in the consumers (HC::Ts) instead of in this class?  The
interface would be simpler if this class just got into a state where calls to
Read() would just return |error|.  That means that HC::Ts might read from the
cache for a while before hitting the error, but I think that's a good thing? 
They might only end up needing what was in the cache.

[shivanisha, 2017/05/18 20:59:44]

Its possible that entry is destroyed and then only HC::T is left to take care of
a future read.

[shivanisha, 2017/05/19 13:49:57]

Or we can doom this entry but let these transaction still associated so that
Active Entry is not destroyed?

[Randy Smith (Not in Mondays), 2017/05/24 23:09:18]

Yeah, that was the direction I think I was gesturing in.  But I don't think it's
worth a lot of complex coding/changing current implementation a lot.  Could you
sketch out to me the path which would resulting in destroying the entry while
there are HC::Ts that were once associated with it?

[shivanisha, 2017/05/31 19:21:26]

If there is a failure then waiting_for_read_ transactions' callbacks are invoked
with the error, idle transactions have a state set which will be checked in
their future Read calls and Writers is then empty. Writers will then invoke
cache_->DoneWritingToEntry with success as false leading to ProcessEntryFailure
being called which will destroy the entry and restart any add_to_entry_queue
transactions.

[Randy Smith (Not in Mondays), 2017/06/06 12:54:57]

Makes sense.  Ok, I'm slightly in favor at the architectural level of dooming
the entry but not destroying it while there are still associated
transactions--the AE is the coordination point, so it makes sense to me for it
to hold the error state.  Having said that, as I say above I don't think it's
worth a lot of code changes, so feel free to do that or not as you prefer.

[shivanisha, 2017/06/07 15:19:18]

Changed the logic to keep this state in Writers itself as that seems simpler.

[shivanisha, 2017/06/08 13:38:41]

Actually while working on the follow up CL, a use case came up because of which
the earlier design would be preferable which sets the state in the individual
transactions and not in the Writers class. So, let's say there is a cache write
failure. Now we want a fail state for future Reads for all idle transactions but
not for the transaction that actually had the cache write failure since it can
continue to read from the network through writers. So making that change in the
follow up CL that integrates this class with HC::T and HC.

[Randy Smith (Not in Mondays), 2017/06/08 17:10:00]

Accepted if that's how you decide that's the best way to handle it, but in the
case you describe, the cache write is happening on behalf of the active
transaction.  So you could set the fail state on the writers class for future
reads, while returning success for the current transaction.  I think that's
conceptually clean--all future reads bounce, but Read() has already been called
for the transaction that you want to return successfully.

[shivanisha, 2017/06/08 17:55:56]

We actually want future reads on the active_transaction_ to go through as well.
Since network_read_only_ will be set to true, these future reads will only read
from the network.

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

Ah, makes sense.  Drat.

+  DCHECK(all_writers_.empty());
+}
+
+void HttpCache::Writers::TruncateEntry() {
+  // TODO(shivanisha) On integration, see if the entry really needs to be
+  // truncated on the lines of Transaction::AddTruncatedFlag and then proceed.
+  DCHECK_EQ(next_state_, State::NONE);
+  next_state_ = State::CACHE_WRITE_TRUNCATED_RESPONSE;
+  DoLoop(OK);
+}
+
+LoadState HttpCache::Writers::GetWriterLoadState() {
+  return network_transaction_->GetLoadState();
+}
+
+HttpCache::Writers::WaitingForRead::WaitingForRead(
+    Transaction* cache_transaction,
+    scoped_refptr<IOBuffer> buf,
+    int len,
+    const CompletionCallback& consumer_callback)
+    : transaction(cache_transaction),
+      read_buf(std::move(buf)),
+      read_buf_len(len),
+      write_len(0),
+      callback(consumer_callback) {
+  DCHECK(cache_transaction);
+  DCHECK(buf);
+  DCHECK_GT(len, 0);
+  DCHECK(!consumer_callback.is_null());
+}
+
+HttpCache::Writers::WaitingForRead::~WaitingForRead() {}
+HttpCache::Writers::WaitingForRead::WaitingForRead(const WaitingForRead&) =
+    default;
+
+int HttpCache::Writers::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 (next_state_ != State::NONE && rv != ERR_IO_PENDING);
+
+  if (rv != ERR_IO_PENDING && !callback_.is_null()) {
+    read_buf_ = NULL;  // Release the buffer before invoking the callback.
+    base::ResetAndReturn(&callback_).Run(rv);

[Randy Smith (Not in Mondays), 2017/05/18 01:02:55]

nit, suggestion: The pattern I've seen more commonly is that the DoLoop() has no
responsibility for handling the callback; it's callers do it.  Here, that
responsibility is shared (between DoLoop() and Read(), at least) which opens a
bit of confusion into which a bug could squeeze.  So I'd suggest having DoLoop()
just unilaterally return the rv, and have the callers do the right thing in
terms of handling it (setting the callback on ERR_IO_PENDING in Read(), nulling
and calling it on !ERR_IO_PENDING in the io callback).  But up to you.

[shivanisha, 2017/05/18 20:59:44]

I guess I am only much aware of the way it is handled in HC::T and using the
same pattern here. I am not completely certain if I understand your suggestion.
I like this pattern though because invoking the consumer's callback is in a
single place.

[Randy Smith (Not in Mondays), 2017/05/24 23:09:18]

Acknowledged.

+  }
+  return rv;
+}
+
+int HttpCache::Writers::DoNetworkRead() {
+  next_state_ = State::NETWORK_READ_COMPLETE;
+  return network_transaction_->Read(read_buf_.get(), io_buf_len_, io_callback_);
+}
+
+int HttpCache::Writers::DoNetworkReadComplete(int result) {
+  if (result < 0) {
+    OnNetworkReadFailure(result);
+    return result;
+  }
+
+  next_state_ = State::CACHE_WRITE_DATA;
+  return result;
+}
+
+void HttpCache::Writers::OnNetworkReadFailure(int result) {
+  // At this point active_transaction_ may or may not be alive.
+  // If no consumer, invoke entry processing here itself.

[Randy Smith (Not in Mondays), 2017/05/18 01:02:55]

suggestion, possibly not for this CL: I'd rather have this done from a single
place rather than two; how reasonable is it to always do this call from here
rather than having the active_transaction_ do it if it's still around?

(Obviously, this applies to all DoneWithEntry/DoneWritingToEntry calls.)

[shivanisha, 2017/05/18 20:59:43]

I will look at this suggestion again in the integration CL. Adding a TODO here.

[shivanisha, 2017/05/19 13:49:57]

Actually on second thoughts, its simpler to invoke it from here. Changing all
sites to invoke DoneWithEntry/DoneWritingToEntry unconditionally from writers.

+  if (!active_transaction_)
+    cache_->DoneWithEntry(entry_, nullptr, true);

[Randy Smith (Not in Mondays), 2017/05/18 01:02:54]

Is there some path from here to ProcessFailure?  How do the WaitingForRead
transactions get notified?

[shivanisha, 2017/05/18 20:59:44]

DoneWithEntry => DoneWritingToEntry => ProcessFailure along with invoking
ProcessEntryFailure (in CL: https://codereview.chromium.org/2721933002/)

[Randy Smith (Not in Mondays), 2017/05/24 23:09:18]

Acknowledged.

+}
+
+int HttpCache::Writers::DoCacheWriteData(int num_bytes) {
+  next_state_ = State::CACHE_WRITE_DATA_COMPLETE;
+  write_len_ = num_bytes;
+  if (!num_bytes || network_read_only_)
+    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::Writers::WriteToEntry(int index,
+                                     int offset,
+                                     IOBuffer* data,
+                                     int data_len,
+                                     const CompletionCallback& callback) {
+  int rv = 0;
+
+  PartialData* partial = nullptr;
+  // Transaction must be alive if this is a partial request.

[Randy Smith (Not in Mondays), 2017/05/18 01:02:55]

This isn't backed up with a DCHECK, and I'm not sure how we can guarantee that
it's always true--is there any reason a partial transaction might not be
destroyed while the network request is outstanding?  How do we deal with that
case?

[shivanisha, 2017/05/18 20:59:43]

In case of partial requests, SetExclusive will be invoked and thus when the
single transaction is removed, no other transaction will invoke Read on this.
Also resetting the network transaction when the transaction is removed so that
if there is an ongoing read, it should be destroyed.

[Randy Smith (Not in Mondays), 2017/05/24 23:09:18]

Thanks for the explanation.  
(nit, suggestion:) I think the above comment would be improved by a few words
from that explanation, maybe just saying "The active transaction must be alive
if this is a partial request, as partial requests are exclusive and hence will
always be the active transaction."  (with the todo?) . But up to you if you
think that's worth it.

[shivanisha, 2017/05/31 19:21:26]

done

+  // Todo(shivanisha): When partial requests support parallel writing, this
+  // assumption will not be true.
+  if (active_transaction_)
+    partial = active_transaction_->partial();
+
+  if (!partial || !data_len) {
+    rv = entry_->disk_entry->WriteData(index, offset, data, data_len, callback,
+                                       true);
+  } else {
+    rv = partial->CacheWrite(entry_->disk_entry, data, data_len, callback);
+  }
+  return rv;
+}
+
+int HttpCache::Writers::DoCacheWriteDataComplete(int result) {
+  if (result != write_len_) {
+    OnCacheWriteFailure();
+
+    // |active_transaction_| can continue reading from the network.
+    result = write_len_;
+  } else {
+    OnDataReceived(result);
+  }
+  return result;
+}
+
+int HttpCache::Writers::DoCacheWriteTruncatedResponse() {
+  next_state_ = State::CACHE_WRITE_TRUNCATED_RESPONSE_COMPLETE;
+  return WriteResponseInfo(true);
+}
+
+int HttpCache::Writers::DoCacheWriteTruncatedResponseComplete(int result) {
+  if (result != io_buf_len_) {
+    DLOG(ERROR) << "failed to write response info to cache";
+  }
+  truncated_ = true;
+  return OK;
+}
+
+void HttpCache::Writers::OnDataReceived(int result) {
+  if (result == 0 && !active_transaction_) {
+    // Check if the response is actually completed or if not, attempt to mark
+    // the entry as truncated.
+    if (IsResponseCompleted()) {
+      ProcessWaitingForReadTransactions(result);
+      cache_->DoneWritingToEntry(entry_, true);
+    } else {
+      OnNetworkReadFailure(result);
+      return;
+    }
+  }
+
+  // Save the data in all the waiting transactions' read buffers.
+  for (auto it = waiting_for_read_.begin(); it != waiting_for_read_.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_for_read_. Tasks will be posted for all the
+  // transactions.
+  ProcessWaitingForReadTransactions(write_len_);

[Randy Smith (Not in Mondays), 2017/05/18 01:02:54]

nit, suggestion: The grouping would feel slightly better to me if the copying
and the notification were in the same function; it's all part of processing the
waiting for read transactions.

[shivanisha, 2017/05/19 13:49:57]

done

+
+  if (result > 0) {  // not the end of response
+    active_transaction_ = nullptr;

[Randy Smith (Not in Mondays), 2017/05/18 01:02:55]

Why keep the active transaction set if the end of the response has been hit?  It
would seem that once the network request returns, anything special about the
active_transaction_ is finished, and it should be removed.

[shivanisha, 2017/05/19 13:49:57]

Now that DoneWithEntry/DoneWritingToEntry will be invoked from here , no need to
retain this pointer.

+    return;
+  }
+  DCHECK_EQ(result, 0);
+  if (!active_transaction_)
+    cache_->DoneWritingToEntry(entry_, true);
+}
+
+void HttpCache::Writers::OnCacheWriteFailure() {
+  network_read_only_ = true;
+
+  // Call the cache_ function here even if active_transaction_ is alive because
+  // it wouldn't know if this was an error case, since it gets a positive result
+  // back. This will in turn ProcessFailure.
+  // TODO(shivanisha) : Also pass active_transaction_ and
+  // ERR_CACHE_WRITE_FAILURE to DoneWritingToEntry on integration so that it can
+  // be passed back in ProcessFailure.
+  cache_->DoneWritingToEntry(entry_, false);
+}
+
+bool HttpCache::Writers::IsResponseCompleted() {

[Randy Smith (Not in Mondays), 2017/05/18 01:02:54]

I'm a bit uncomfortable with this function's name; it's not testing whether the
response is completed, it's testing whether it could be completed in the context
of a 0 length read signaling EOF from the network transaction.

This is a case where just hoisting the function up to its single call site would
solve the problem, as getting rid of the confusion caused by the name is my goal
:-}.  Alternatively, would you call it something like
"IsResponsePossiblyCompleted()" or something like that, and maybe put a comment
here or at its declaration site indicating that it's testing for completion in
the presence of an EOF signal.

[shivanisha, 2017/05/19 13:49:57]

Moved it to its call site.

+  int current_size = entry_->disk_entry->GetDataSize(kResponseContentIndex);
+  const HttpResponseInfo* response_info =
+      network_transaction_->GetResponseInfo();
+  int64_t content_length = response_info->headers->GetContentLength();
+  if ((content_length >= 0 && content_length <= current_size) ||
+      content_length < 0)
+    return true;
+  return false;
+}
+
+int HttpCache::Writers::WriteResponseInfo(bool truncated) {

[Randy Smith (Not in Mondays), 2017/05/18 01:02:54]

Again, I'd vote in favor of hoisting this into the calling function.  There's
nothing in that function, so I don't think there's any value in terms of
readability or comprehensibility to leaving it here.  If you disagree, I'll go
with your preference, but if you're willing give me a sense of why it's your
preference.  If we have different aesthetics in this space I can probably review
your code better if I understand your aesthetics.

[shivanisha, 2017/05/19 13:49:57]

Moved to calling site. I had this as separate function mainly because I thought
it would be needed by both HC::T and this class but I think its simple enough to
be in both classes.
I had thought the same for IsResponseCompleted but looks like that would not be
needed by HC::T. 

+  // When writing headers, we normally only write the non-transient headers.
+  const HttpResponseInfo* response = network_transaction_->GetResponseInfo();
+  bool skip_transient_headers = true;
+  scoped_refptr<PickledIOBuffer> data(new PickledIOBuffer());
+  response->Persist(data->pickle(), skip_transient_headers, truncated);

[Randy Smith (Not in Mondays), 2017/05/18 01:02:54]

nit, suggestion: I'd just write this as 

response->Persist(data->pickle(), true /* skip_transient_headers *, truncated);

Possibly with the comment immediately above that line (though I don't think the
comment says much the code doesn't).

[shivanisha, 2017/05/19 13:49:57]

done

+  data->Done();
+  io_buf_len_ = data->pickle()->size();
+  return entry_->disk_entry->WriteData(kResponseInfoIndex, 0, data.get(),
+                                       io_buf_len_, io_callback_, truncated);
+}
+
+void HttpCache::Writers::ProcessWaitingForReadTransactions(int result) {
+  for (auto it = waiting_for_read_.begin(); it != waiting_for_read_.end();
+       it++) {
+    Transaction* transaction = it->transaction;
+    if (result > 0) {  // success
+      result = it->write_len;

[Randy Smith (Not in Mondays), 2017/05/18 01:02:55]

(Just an addition to an earlier comment: Setting result from write_len seems
like it is pretty directly related to how many bytes were copied.  If you did
the copying and posting in one function, you wouldn't need the temporary data
member.)

[shivanisha, 2017/05/19 13:49:57]

Moved copying to this function.

+    } else {
+      // If its response completion or failure, this transaction needs to be
+      // removed.
+      all_writers_.erase(transaction);

[Randy Smith (Not in Mondays), 2017/05/18 01:02:54]

Caption: This comment is an extended babble with me trying to get my thoughts in
order and possibly spark thoughts in you--if you'd like to ignore it for now,
that's fine, and I'll come back to this issue when I understand this patch
better and how it's going to be used.  

I'm a bit uncomfortable with the responsibility for keeping all_writers_ up to
date being shared between consumer and producer.  Given that the consumer has to
remove itself upon destruction, is it reasonable to expect it to remove itself
upon error as well?  Or leave itself on the list until it's destroyed (which
probably won't be very long)?  Or does that cause problems in other areas?

This fits in with a bit of discomfort about transactions both being listed in
the Writers and being passed into Read(), which makes me want to explore whether
or not we really need to have all_writers_.  It looks to me as if that's only
used for StopCaching() and MoveIdleWritersToReaders().  StopCaching() is a bit
weird in that the first transaction in may call it, and thus forbid later
transactions, but later transactions don't have that option, so there's an
asymmetry.   And MoveIdleWritersToReaders() seems really weird to me, since
HC::Ts can move back and forth between being readers and writers based on how
far ahead of their read point the cache entry gets.  So I find myself wanting to
explore whether there's an architecture that uses Writers as a synchronization
point, but doesn't keep a list of writers at all; it only keeps the active
transaction and the list of WaitingToRead transactions.  

I guess I'll ask: How do you envision the additions and removals to this class
(and to the queues in ActiveEntry) occurring if:

* Transaction A does validation, reads a chunk of data from the network.
* Transaction B does validation, reads a lot of chunks of data from the network,
not completely filling the entry.
* Transaction A comes back and reads data, now from the cache.

Transaction A will stay in all_writers_, but will be reading from the cache. 
What do we gain from having it in all_writers_?  

[shivanisha, 2017/05/19 13:49:58]

In case transaction B comes back , reads from network and fails writing to
cache, writers will need to know the transactions that should be notified of
this failure.

[shivanisha, 2017/05/19 17:12:58]

Actually, keeping all_writers_ is important from a high level perspective for
the following invariant to be true:
Currently, entry->writer != nullptr means there is a writer.
After shared writing, !entry->writers.IsEmpty() should signify that there is a
writer present. If we do not save this then while adding a new transaction it
won't know if a writer is present.

[Randy Smith (Not in Mondays), 2017/05/22 01:56:55]

Acknowledged.

+    }
+    // Post task to notify transaction.
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::Bind(it->callback, result));
+  }
+  waiting_for_read_.clear();
+}
+
+void HttpCache::Writers::SetIdleWritersFailState(int result) {
+  // Since this is only for idle transactions, all waiting_for_read_ and
+  // active_transaction_ should be empty.
+  DCHECK(waiting_for_read_.empty());
+  DCHECK(!active_transaction_);
+  for (auto* transaction : all_writers_)
+    transaction->SetSharedWritingFailState(result);
+  all_writers_.clear();
+}
+
+RequestPriority HttpCache::Writers::GetCurrentHighestPriority() {
+  RequestPriority priority = MINIMUM_PRIORITY;
+  for (auto* transaction : all_writers_)
+    priority = std::max(transaction->priority(), priority);
+  return priority;
+}
+
+void HttpCache::Writers::OnIOComplete(int result) {
+  DoLoop(result);
+}
+
+}  // namespace net