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.

[jkarlin, 2017/06/28 15:50:10]

This should be a part of the HttpCache::Writers comment.

[shivanisha, 2017/06/28 19:38:20]

moved

+
+namespace net {
+
+// Writer represents the set of all HttpCache::Transactions that are

[jkarlin, 2017/06/28 15:50:10]

s/Writer/Writers/

[shivanisha, 2017/06/28 19:38:20]

done

+// 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:
+  friend class WritersTest;

[Randy Smith (Not in Mondays), 2017/06/27 18:49:37]

nit, suggestion: I'm used to seeing friend declarations in the private section. 
I can't find anything about it in the style guide, so up to you, but my sense is
that consistency would suggest the private section.

[jkarlin, 2017/06/28 15:50:10]

newline after the friend declaration

[shivanisha, 2017/06/28 18:02:33]

moved to private section

+  // |*disk_entry| must outlive this object.
+  Writers(disk_cache::Entry* entry);
+  ~Writers();
+
+  // 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 currently 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().

[jkarlin, 2017/06/28 15:50:09]

Comment doesn't make it clear what happens if Writers is destroyed while
consumer is waiting for the callback to be called. Will it still be called?

[shivanisha, 2017/06/28 19:38:20]

It will not be invoked. Added in the comment

+  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

[jkarlin, 2017/06/28 15:50:10]

If there are two simultaneous downloads of the same large file, and both call
StopCaching(), they won't actually stop, is that correct?

[shivanisha, 2017/06/28 19:38:20]

That's right

[jkarlin, 2017/06/30 18:20:39]

This needs to be clearly documented in HttpTransaction::StopCaching. 

[shivanisha, 2017/07/05 18:55:42]

Yes, will document it in the integration CL. Added a TODO for it here

+  // caching can be stopped.
+  bool StopCaching(Transaction* transaction);
+
+  // Membership functions.

[jkarlin, 2017/06/28 15:50:09]

Where does the scope of this comment end? I doubt that TruncateEntry is a
membership function. I'd just remove the comment.

[shivanisha, 2017/06/28 19:38:20]

Removed

+
+  // Adds an HttpCache::Transaction to Writers and if it's the first transaction
+  // added, transfers the ownership of the network transaction to Writers.
+  // Should only be invoked if CanAddTransaction() returns true.

[jkarlin, 2017/06/28 15:50:09]

Do you mean CanAddWriters?

[shivanisha, 2017/06/28 19:38:20]

yes, updated

+  // |network_transaction| should be non-null only for the first transaction
+  // and it will be assigned to |network_transaction_|. If |is_exclusive| is
+  // true, it makes writing an exclusive operation implying that Writers can
+  // contain at most one transaction till the completion of the response body.
+  // |transaction| can be destroyed at any point and it should invoke

[jkarlin, 2017/06/28 15:50:09]

Should the last line read: Call RemoveTransaction before destroying
|transaction|, it is safe to call at any time.

?

[shivanisha, 2017/06/28 19:38:20]

The existing statement is conveying the same meaning? (Avoiding changes if
possible to avoid many merge conflicts with the subsequent CL)

+  // RemoveTransaction() during its destruction.
+  void AddTransaction(Transaction* transaction,
+                      std::unique_ptr<HttpTransaction> network_transaction,
+                      bool is_exclusive);
+
+  // Removes a transaction.
+  void RemoveTransaction(Transaction* transaction);

[jkarlin, 2017/06/28 15:50:09]

When can this be called?

[shivanisha, 2017/06/28 19:38:20]

Anytime a transaction is destroyed.Added

+
+  // Invoked when there is a change in a member transaction's priority or a
+  // member transaction is removed.
+  void UpdatePriority();
+
+  // 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 {

[jkarlin, 2017/06/28 15:50:09]

suggest s/IsPresent/HasTransaction/

[shivanisha, 2017/06/28 19:38:20]

done

+    return all_writers_.count(transaction) > 0;
+  }
+
+  // Remove and return any idle writers. Should only be invoked when a
+  // response is completely written and when ContainesOnlyIdleWriters()
+  // returns true.
+  TransactionSet RemoveAllIdleWriters();
+
+  // Returns true if more writers can be added for shared writing.
+  bool CanAddWriters();
+
+  HttpTransaction* network_transaction() { return network_transaction_.get(); }

[jkarlin, 2017/06/28 15:50:09]

Interesting, who would need to call this function?

[shivanisha, 2017/06/28 19:38:20]

HC::T might. If not, I will remove it from the integration CL. Added a TODO

+
+  // Invoked to mark an entry as truncated.
+  void TruncateEntry();

[jkarlin, 2017/06/28 15:50:09]

When can this be called? Even if a Read() is ongoing?

[shivanisha, 2017/06/28 19:38:20]

No, it has a dcheck to assert that

[jkarlin, 2017/06/30 18:20:39]

Please update the comment to reflect when it can be called.

[shivanisha, 2017/07/05 18:55:42]

done

+
+  // Should be invoked only when writers has transactions attached to it and
+  // thus has a valid network transaction.
+  LoadState GetWriterLoadState();
+
+  // For testing.
+

[jkarlin, 2017/06/28 15:50:09]

Remove newline.

[shivanisha, 2017/06/28 19:38:20]

done

+  int CountTransactionsForTesting() const { return all_writers_.size(); }

[jkarlin, 2017/06/28 15:50:09]

Are the ForTesting functions necessary? This class is friends with the testing
class.

[shivanisha, 2017/06/28 19:38:20]

The compiler still gives an error if private members accessed in the test body

+  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);
+
+  // Notifies the transactions waiting on Read of the result, by posting a task
+  // for each of them.
+  void ProcessWaitingForReadTransactions(int result);
+
+  // Sets the state to FAIL_READ so that any subsequent Read on an idle
+  // transaction fails.
+  void SetIdleWritersFailState(int result);
+
+  // Called to reset state when all transaction references are removed from
+  // |this|.
+  void ResetStateForEmptyWriters();
+
+  // Invoked when |active_transaction_| fails to read from network or write to
+  // cache. |error| indicates network read error code or cache write error.
+  void ProcessFailure(Transaction* transaction, int error);
+
+  // Returns true if |this| only contains idle writers.
+  bool ContainsOnlyIdleWriters() const;
+
+  // 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;
+
+  // TODO(shivanisha) Add HttpCache* cache_ = nullptr; on integration.
+
+  disk_cache::Entry* disk_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. ResetStateForEmptyWriters should be invoked
+  // whenever all_writers_ becomes empty.
+  TransactionSet all_writers_;
+
+  // True if multiple transactions are not allowed e.g. for partial requests.
+  bool is_exclusive_ = false;
+
+  // Current priority of the request. It is always the maximum of all the writer
+  // transactions.
+  RequestPriority priority_ = MINIMUM_PRIORITY;
+
+  bool truncated_ = false;  // used for testing.
+
+  CompletionCallback callback_;  // Callback for active_transaction_.
+
+  base::WeakPtrFactory<Writers> weak_factory_;
+  DISALLOW_COPY_AND_ASSIGN(Writers);
+};
+
+}  // namespace net
+
+#endif  // NET_HTTP_HTTP_CACHE_WRITERS_H_