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

net/http/http_cache_writers.h

+// 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.
+
+#ifndef NET_HTTP_HTTP_CACHE_WRITERS_H_
+#define NET_HTTP_HTTP_CACHE_WRITERS_H_
+#include <list>
+#include <memory>
+#include "base/memory/weak_ptr.h"
+#include "net/base/completion_callback.h"
+#include "net/http/http_cache.h"
+
+// If multiple HttpCache::Transactions are accessing the same cache entry
+// simultaneously, their access to the data read from network is synchronized
+// by HttpCache::Writers. This enables each of those transactions to drive
+// reading the response body from the network ensuring a slow consumer does not
+// starve other consumers of the same resource.
+
+namespace net {
+
+// Writer represents the set of all HttpCache::Transactions that are
+// reading from the network using the same network transaction and writing to
+// the same cache entry. It is owned by the ActiveEntry.
+class NET_EXPORT_PRIVATE HttpCache::Writers {
+ public:
+  // |*entry| and |*cache| should always be valid when accessed from Writers.

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

I think of comments as being directions to the consumer as to what rules they
have to follow for the class to behave properly for them.  The consumer of HC::W
doesn't know when entry/cache are going to be accessed.  So I'd phrase this as
"The consumer must guarantee that |*entry| and |*cache| outlive this object."

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

done

+  Writers(HttpCache* cache, ActiveEntry* entry);
+  ~Writers();
+
+  // Invokes Read on network transaction if a read is not already in progress.
+  // In case a read is already in progress then this transaction is added to
+  // a waiting queue and ERR_IO_PENDING is returned. If ERR_IO_PENDING is
+  // returned, the result of the read will be later communicated to the consumer
+  // via the |callback|.

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

nit, suggestion: I think there's a bit of implementation detail (the waiting
queue, the specification of the reason for the IO_PENDING) leaking into this
interface contract.  From the caller's perspective, it doesn't matter whether
the sync/async return is because there's already a read in progress or because
the network read is invoked and *it's* async.  Of course, the purpose of this
class is coordinating among multiple readers, so that can/should be mentioned. 
I'd suggest something like

"Retrieves data from the network transaction associated with the Writers object.
  This may be done directly (via a network read into |*buf->data()|) or
indirectly (by copying from another transactions buffer into |*buf->data()| on
network read completion) depending on whether or not a read is current in
progress.  May return the result synchronously or return ERR_IO_PENDING: if
ERR_IO_PENDING is returned, |callback| will be run to inform the consumer of the
result of the Read()."

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

done

+  int Read(scoped_refptr<IOBuffer> buf,
+           int buf_len,
+           const CompletionCallback& callback,
+           Transaction* transaction);
+
+  // Invoked when StopCaching is called on a member transaction.
+  // It stops caching only if there are no other transactions. Returns true if
+  // caching can be stopped.
+  bool StopCaching(Transaction* transaction);
+
+  // Membership functions.
+
+  // Adds an HttpCache::Transaction to Writers and if its the first transaction
+  // added, transfers the ownership of the network transaction to Writers.
+  // Should only be invoked if CanAddTransaction() returns true. If
+  // |network_transaction| is non-null, it will be assigned to
+  // |network_transaction_|. The first transaction added should definitely pass
+  // a non-null |network_transaction|. |transaction| can be destroyed at any
+  // point and it should invoke RemoveTransaction() during its destruction.

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

This leaves me a little confused as to whether it's legal to pass a
network_transaction if this isn't the first AddTransaction() call, and what
happens if that is done.  Would you clarify the comment?

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

Rephrased

+  void AddTransaction(Transaction* transaction,
+                      std::unique_ptr<HttpTransaction> network_transaction);
+
+  // Removes a transaction.
+  void RemoveTransaction(Transaction* transaction);
+
+  // Invoked when there is a change in a member transaction's priority or a
+  // member transaction is removed.
+  void PriorityChanged();
+
+  // Returns true if this object is empty.
+  bool IsEmpty() const { return all_writers_.empty(); }
+
+  // Returns true is |transaction| is part of writers.
+  bool IsPresent(Transaction* transaction) const {
+    return all_writers_.count(transaction) > 0;
+  }
+
+  // Returns true if |this| only contains idle writers.
+  bool ContainsOnlyIdleWriters() const;
+
+  // Move any idle writers to entry_->readers.  Should only be invoked when a
+  // response is completely written and when ContainesOnlyIdleWriters()
+  // returns true.
+  void MoveIdleWritersToReaders();
+
+  // Makes writing an exclusine operation implying that Writers can contain at
+  // most one transaction for the complete reading/writing of the response body.
+  void SetExclusive();
+
+  // Returns true if more writers can be added for shared writing.
+  bool CanAddWriters();
+
+  HttpTransaction* network_transaction() { return network_transaction_.get(); }
+
+  // Invoked from HttpCache when it is notified of a transaction failing to
+  // write. |error| indicates network read error code or cache write error.
+  void ProcessFailure(Transaction* transaction, int error);
+
+  // Invoked to mark an entry as truncated. Should be invoked when reading is
+  // not currently in progress.

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

How will the caller know that reading is not currently in progress?  

More generally, I'm trying to wrap my head around the use case for this. 
Looking at HC::T, it looks like we truncate cached data after we receive the
response headers.  But that means it would happen in the validating transaction,
not when we're sharing access, or maybe on the addition of the first writer &
the network transaction to the class.  Would it make sense to combine those
functionalities?

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

As a parallel with HC::T states this is not equivalent to
STATE_TRUNCATE_CACHED_DATA* which happens after we receive response headers but
this is equivalent to STATE_CACHE_WRITE_TRUNCATED_RESPONSE* which might happen
when transaction responsible for writing the data is destroyed. See
AddTruncatedFlag(). Removing the comment to make it simpler.

+  void TruncateEntry();
+
+  LoadState GetWriterLoadState();

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

This currently requires a network transaction to be attached to the structure. 
Given that the structure seems to allow a null network transaction (during
initialization or in some corner cases) shouldn't there be a (consumer oriented)
comment here saying when it can be called?  Or the routine should avoid
requiring network_transaction_ set?

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

Done.

+
+  // For testing.
+
+  int CountTransactionsForTesting() const { return all_writers_.size(); }
+  bool IsTruncatedForTesting() const { return truncated_; }
+
+ private:
+  enum class State {
+    NONE,
+    NETWORK_READ,
+    NETWORK_READ_COMPLETE,
+    CACHE_WRITE_DATA,
+    CACHE_WRITE_DATA_COMPLETE,
+    CACHE_WRITE_TRUNCATED_RESPONSE,
+    CACHE_WRITE_TRUNCATED_RESPONSE_COMPLETE,
+  };
+
+  // These transactions are waiting on Read. After the active transaction
+  // completes writing the data to the cache, their buffer would be filled with
+  // the data and their callback will be invoked.
+  struct WaitingForRead {
+    Transaction* transaction;
+    scoped_refptr<IOBuffer> read_buf;
+    int read_buf_len;
+    int write_len;
+    const CompletionCallback callback;
+    WaitingForRead(Transaction* transaction,
+                   scoped_refptr<IOBuffer> read_buf,
+                   int len,
+                   const CompletionCallback& consumer_callback);
+    ~WaitingForRead();
+    WaitingForRead(const WaitingForRead&);
+  };
+  using WaitingForReadList = std::list<WaitingForRead>;
+
+  // Runs the state transition loop. Resets and calls |callback_| on exit,
+  // unless the return value is ERR_IO_PENDING.
+  int DoLoop(int result);
+
+  // State machine functions.
+  int DoNetworkRead();
+  int DoNetworkReadComplete(int result);
+  int DoCacheWriteData(int num_bytes);
+  int DoCacheWriteDataComplete(int result);
+  int DoCacheWriteTruncatedResponse();
+  int DoCacheWriteTruncatedResponseComplete(int result);
+
+  // Helper functions for callback.
+
+  void OnNetworkReadFailure(int result);
+  void OnCacheWriteFailure();
+  void OnDataReceived(int result);
+
+  // Helper function for writing to cache.
+  int WriteToEntry(int index,
+                   int offset,
+                   IOBuffer* data,
+                   int data_len,
+                   const CompletionCallback& callback);
+
+  // Notifies the transactions waiting on Read of the result, by posting a task
+  // for each of them.
+  void ProcessWaitingForReadTransactions(int result);
+
+  // Sets the state of idle writers so that they can fail any subsequent
+  // Read.
+  void SetIdleWritersFailState(int result);
+
+  // Called to reset state when all transaction references are removed from
+  // |this|.
+  void ResetStateForEmptyWriters();
+
+  // IO Completion callback function.
+  void OnIOComplete(int result);
+
+  State next_state_ = State::NONE;
+
+  // True if only reading from network and not writing to cache.
+  bool network_read_only_ = false;
+
+  // Http Cache.
+  HttpCache* cache_ = nullptr;
+
+  // Owner of this object.
+  ActiveEntry* entry_ = nullptr;
+
+  std::unique_ptr<HttpTransaction> network_transaction_ = nullptr;
+
+  scoped_refptr<IOBuffer> read_buf_ = nullptr;
+
+  int io_buf_len_ = 0;
+  int write_len_ = 0;
+
+  // The cache transaction that is the current consumer of network_transaction_
+  // ::Read or writing to the entry and is waiting for the operation to be
+  // completed. This is used to ensure there is at most one consumer of
+  // network_transaction_ at a time.
+  Transaction* active_transaction_ = nullptr;
+
+  // Transactions whose consumers have invoked Read, but another transaction is
+  // currently the |active_transaction_|. After the network read and cache write
+  // is complete, the waiting transactions will be notified.
+  WaitingForReadList waiting_for_read_;
+
+  // Includes all transactions.
+  TransactionSet all_writers_;
+
+  // True if multiple transactions are not allowed e.g. for partial requests.
+  bool is_exclusive_ = false;
+
+  // Current priority of the request. If a higher priority transaction is
+  // added, the priority of network transaction will be increased.
+  RequestPriority priority_ = MINIMUM_PRIORITY;
+
+  bool truncated_ = false;  // used for testing.
+
+  CompletionCallback callback_;  // Callback for active_transaction_.
+  CompletionCallback io_callback_;
+  base::WeakPtrFactory<Writers> weak_factory_;
+  DISALLOW_COPY_AND_ASSIGN(Writers);
+};
+
+}  // namespace net
+#endif  // NET_HTTP_HTTP_CACHE_WRITERS_H_