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..9295b6918012f0be9b1db9037c510a15af2c6d40
--- /dev/null
+++ b/net/http/http_cache_writers.h
@@ -0,0 +1,226 @@
+// 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>

[jkarlin, 2017/06/12 18:30:32]

newline above

[shivanisha, 2017/06/14 02:33:17]

Added

+#include <memory>
+#include "base/memory/weak_ptr.h"

[jkarlin, 2017/06/12 18:30:32]

newline above

[shivanisha, 2017/06/14 02:33:17]

added

+#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| must outlive this object.
+  Writers(HttpCache* cache, ActiveEntry* 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().
+  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

[jkarlin, 2017/06/12 18:30:32]

s/its/it's/

[shivanisha, 2017/06/14 02:33:17]

done

+  // added, transfers the ownership of the network transaction to Writers.
+  // Should only be invoked if CanAddTransaction() returns true.
+  // |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 exclusine operation implying that Writers can

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

nit: exclusive

[shivanisha, 2017/06/12 18:51:14]

done

+  // contain at most one transaction till the completion of the response body.
+  // |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,
+                      bool is_exclusive);

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

The current implementation requires the consumer to take responsibility to not
call in with an exclusive bit set if that's not legal.  My instinct is that it
might make more sense to just have AddTransaction return a success flag, and
bounce the add if is_exclusive isn't legal.  Whether this is the right idea or
not depends heavily on how the function is used, so I'll defer to your judgement
and/or re-raise the issue in the integration CL, but I wanted to call out the
possibility of the slightly different interface contract now.  

[shivanisha, 2017/06/12 18:51:14]

ok, let's come back to this in the integration CL

+
+  // 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();
+
+  // 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.
+  void TruncateEntry();
+
+  // Should be invoked only when writers has transactions attached to it and
+  // thus has a valid network transaction.
+  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,
+    FAIL_READ

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

My personal preference would be to keep the invariant that states in State
correspond to sub-functions of DoLoop() and the semantics of those states are
implemented in the subfunctions.  I'd suggest turning this into a boolean
instead of a state.

[jkarlin, 2017/06/12 18:30:32]

+1

[shivanisha, 2017/06/12 18:51:14]

Deferring this comment since the follow up CL anyways removed this state.

+  };
+
+  // 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.
+

[jkarlin, 2017/06/12 18:30:32]

Remove newline?

[shivanisha, 2017/06/14 02:33:17]

Comment applies to all 3 following functions.

[jkarlin, 2017/06/14 20:03:14]

Right, for group comments I typically see the comment directly above the first
function, and it applies to all functions below until empty line.

+  void OnNetworkReadFailure(int result);
+  void OnCacheWriteFailure();
+  void OnDataReceived(int result);
+
+  // Helper function for writing to cache.
+  int WriteToEntry(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 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();
+
+  // 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. It is always the maximum of all the writer
+  // transactions.
+  RequestPriority priority_ = MINIMUM_PRIORITY;
+
+  // Error to be returned on a future Read when state is FAIL_READ.
+  int error_code_ = 0;
+
+  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_

[jkarlin, 2017/06/12 18:30:32]

newline above

[shivanisha, 2017/06/14 02:33:17]

Added