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

net/http/http_cache_writers.h

Index: net/http/http_cache_writers.h
diff --git a/net/http/http_cache_writers.h b/net/http/http_cache_writers.h
new file mode 100644
index 0000000000000000000000000000000000000000..8c2403fef37419c407b045ab6d564234769fed6b
--- /dev/null
+++ b/net/http/http_cache_writers.h
@@ -0,0 +1,222 @@
+// 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.
+  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|.
+  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.
+  void AddTransaction(Transaction* transaction,
+                      std::unique_ptr<HttpTransaction> network_transaction);
+
+  // Removes a transaction.
+  void RemoveTransaction(Transaction* transaction);
+
+  // Removes all transactions.
+  void RemoveAllTransactions();
+
+  // 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;
+  }
+
+  // When response is completely written, any idle writers are moved to
+  // entry_->readers.
+  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() { is_exclusive_ = true; }

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

What happens if this is called with a lot of transactions on the object?  It
would seem like more handling than just setting the bit would be necessary; can
you give me a sense as to why that isn't true?

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

Ping?

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

Added a dcheck that only 0 or 1 transactions should be present when this is
called.

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

SG.  Just so I'm tracking, what's the use case for zero transactions?

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

Actually, adding an is_exclusive argument to AddTransaction and removing this
function. And the DCHECK basically says that this should be the first
transaction added if is_exclusive is true.

+
+  // Returns true if more writers can be added for shared writing.
+  bool CanAddWriters() const;
+
+  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.
+  void TruncateEntry();
+
+  LoadState GetWriterLoadState();
+
+  // 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);
+
+  RequestPriority GetCurrentHighestPriority();
+
+  bool IsResponseCompleted();
+
+  int WriteResponseInfo(bool truncated);
+
+  // 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_;
+
+  // Owner of this object.
+  ActiveEntry* entry_ = nullptr;

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

My understanding is that Chromium prefers to keep initialization together (i.e.
either all in the .h file or all in the constructor).  Having said that, there's
a broken pointer link (apparently the Google C++ guide used to comment on this
and now it doesn't) so if you feel strongly about keeping them separate ping me
about the issue and I'll ping the thread (or ping the thread directly:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/zqB-DySA4V0).
 

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

Ping?

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

Changed the header file such that all fields are initialized there (except the
ones that are initialized by default)

+
+  std::unique_ptr<HttpTransaction> network_transaction_;
+
+  scoped_refptr<IOBuffer> read_buf_;
+
+  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_;  // Consumer's callback.

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

Suggestion: Change comment to "Callback for active_transaction_".

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

Ping?

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

done

+  CompletionCallback io_callback_;

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

Suggestion: In my ideal world this would be const, since it's initialized in the
constructor and never touched after that.  That might mean you need to
initialize it in the constructor data member list, which might mean you need to
put it after WeakPtrFactory, or maybe cast?  But I think marking this const
would be a useful signal to future readers.

An alternative would be to actually just replicate the Bind() call in all three
places where this is used, which I'd be completely fine with but I realize is
inconsistent with usage elsewhere in the cache.  More code, but IMO more
readable code (since the reader doesn't have to lookup the io_callback_
initialization).

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

Ping?

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

It seems nothing should come after WeakPtrFactory in the class as per
https://cs.chromium.org/chromium/src/base/memory/weak_ptr.h?q=WeakPtrFactory&...

Adding initialization on all 3 call sites.

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

If you're doing the initialization at the call sites you can use a local
variable or do it inline; no need for this declaration.

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

Removed the member variable.

+  base::WeakPtrFactory<Writers> weak_factory_;
+  DISALLOW_COPY_AND_ASSIGN(Writers);
+};
+
+}  // namespace net
+#endif  // NET_HTTP_HTTP_CACHE_WRITERS_H_