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..8a33935cc184dcc1379c72ebd22a153a75be5236
--- /dev/null
+++ b/net/http/http_cache_writers.h
@@ -0,0 +1,220 @@
+// 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"
+
+// Multiple transactions reading from the network and writing to the cache
+// enables each of those transactions to drive reading the response body from
+// the network. This ensures that a slow consumer does not starve other
+// consumers of the same resource. A shared transaction will either read the
+// already written part of the response from the cache or invoke read on the
+// network and write to the cache, depending on its read offset.

[Randy Smith (Not in Mondays), 2017/05/16 22:35:28]

I'm having a hard time understanding this comment in context.  It doesn't make
any explicit reference to the class being defined in this file, and hence could
easily lead someone who doesn't know the implementation well (who should pretty
much always be the target audience) to not understand its relevance to the code
below.  

Maybe something like "If multiple HttpCache::Transactions are accessing the same
cache entry, they multiplex their access to that entry through
HttpCache::Writers.  Any of those transactions may access that entry, either
reading from the cache or reading from the network and writing to the cache,
depending on the read offset.  This ensures that a slow consumer does not starve
other consumers of the same resource."?  But I'm not committed to the wording, I
just want something that uses the vocabulary that a person might understand when
they first open this file.

[shivanisha, 2017/05/17 13:02:30]

Sounds good. Updated the comment.

+
+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 HttpCache::Writers {
+ public:
+  // Creates a new Writers object and transfers the ownership of network
+  // transaction to Writers. Consumer must ensure that |*entry| and
+  // |*cache| outlive |this|.
+  Writers(base::WeakPtr<HttpCache> cache, ActiveEntry* entry);

[Randy Smith (Not in Mondays), 2017/05/16 22:35:29]

Two questions: 
a) How can this transfer a network transaction to Writers if there's no network
transaction argument?
b) If the Writers object is owned by the ActiveEntry, which is owned by the
cache, why is a WeakPtr<> needed?

[shivanisha, 2017/05/17 13:02:30]

a) Removed that part of the comment. Was a remnant from an earlier iteration.
Thanks
b) Converted to a raw pointer

+  ~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/05/16 22:35:29]

Given that the offset in the object was mentioned in an earlier comment, it's a
bit confusing that this comment implies that everyone shares an offset.  Can you
update comments somewhere to clarify this?  (Either the comment at the top that
mentions offset, here, or some additional comment that describes how
transactions with different offsets are handled.)

[shivanisha, 2017/05/17 13:02:30]

Removed the reference to offset in the above comment. Transactions invoking Read
here will always be at the latest offset else they would not invoke Read and
will read the cached data in the HC::T state machine itself.

+  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 a transaction to Writers. Should only be invoked if

[Randy Smith (Not in Mondays), 2017/05/16 22:35:28]

nit: I believe this only adds HttpCache::Transactions.  Given that the signature
refers to both those and HttpTransactions, I'd advise being specific about which
type you mean.

[shivanisha, 2017/05/17 13:02:30]

Done

+  // 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. It is ok for

[Randy Smith (Not in Mondays), 2017/05/16 22:35:28]

This seems contradictory with the constructor comment.

[shivanisha, 2017/05/17 13:02:30]

Constructor comment corrected.

+  // |transaction| to die before this object.

[Randy Smith (Not in Mondays), 2017/05/16 22:35:29]

Leaving the object with an invalid raw pointer?  That seems an invitation for
use-after-free errors.  It would be good if there was a mechanism for cleaning
up the Writers object correctly when a transaction dies.  

(If it's a requirement for the HC::T to call RemoveTransaction when it dies, the
right way to write this comment is "|transaction| is required to call
|RemoveTransaction(transaction)| before being destroyed."  In general,
documentation comments should describe what consumers have to do to correct use
the interface.  (Abstract base class comments are a bit tricky since they also
need to describe what producers have to do to satisfy the interface.))

[shivanisha, 2017/05/17 13:02:30]

Updated the comment to include RemoveTransaction requirement. Thanks

+  void AddTransaction(Transaction* transaction,
+                      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() { return all_writers_.empty(); }
+
+  // Returns true is |transaction| is part of writers.
+  bool IsPresent(Transaction* transaction) {
+    return all_writers_.count(transaction) > 0;
+  }
+
+  // When response is completely written, any idle writers are moved to
+  // entry_->readers.
+  void MoveIdleWritersToReaders();
+
+  // Set exclusive as true.

[Randy Smith (Not in Mondays), 2017/05/16 22:35:29]

This doesn't actually describe the behavior that the consumer sees; all it says
is that a bit is being set on the structure (which is the implementation). 
What's the change in behavior from setting exclusive to true?

[shivanisha, 2017/05/17 13:02:30]

Done

+  void SetExclusive() { is_exclusive_ = true; }
+
+  // Returns true if more writers can be added for shared writing.
+  bool CanAddWriters();

[Randy Smith (Not in Mondays), 2017/05/16 22:35:29]

Is this a const function?

More generally, for state machine classes, I think of state changing functions
as having restrictions on when they can be called (so as to not confuse the
state machine).  So I have a urge to see everything in this class either having
documented restrictions on when they can be called or be marked const.  That's
obviously not appropriate globally (e.g. Read() handles this just fine
internally, either queueing or starting the state machine) but if you could put
a moment of those into whether any of the non-const functions in this header
should have restrictions on when they can be called I'd be grateful.

[shivanisha, 2017/05/17 13:02:31]

Made a few functions as const. Thanks

+
+  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();

[Randy Smith (Not in Mondays), 2017/05/16 22:35:29]

The fact that the implementation of this involves setting a state and calling
into DoLoop() suggests to me that three are specific restrictions on when this
can be called (e.g. not when another read is outstanding?).  Could you document
those conditions, or respond to this comment telling me why it's ok to just do a
state transition by fiat at any point in the state machine to a new state?

(I'd also value understanding better what the use cases are for uses of this
function.  That may not be appropriate in the documentation comment--the
documentation comment should indicate what the effect of the function is from
the consumers POV along with any restrictions the consumer needs to obey in
calling it--but if not, respond to this comment with a sketch?)

[shivanisha, 2017/05/17 13:02:30]

Documented and also added a DCHECK that state should be NONE.
Since the active transaction can be destroyed at any time and if that was the
only transaction associated with Writers then the entry should be correctly
marked as truncated after the ongoing read/write is complete. I haven't yet
added a call to it because based on integration with HttpCache, it could be
invoked from HttpCache layer or from the Writers layer itself. That's a TODO.

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

Can that be done as a conditional inside of RemoveTransaction() rather than as a
separate interface?

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

RemoveTransaction() will be called as soon as the transaction is removed while
TruncateEntry should be invoked after the ongoing cache write is complete.

+
+  LoadState GetWriterLoadState();
+
+  // For testing.

[Randy Smith (Not in Mondays), 2017/05/16 22:35:29]

nit: Test functions usually have "ForTesting" suffixed; I think there's some
auto-analysis to confirm they're only used for testing.

[shivanisha, 2017/05/17 13:02:30]

Added the suffix.

+  int CountTransactions() { return all_writers_.size(); }
+  bool IsTruncated() { 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();

[Randy Smith (Not in Mondays), 2017/05/16 22:35:28]

nit: I don't think this format is allowed by the style guide; it's either lower
case separated by underscores (and usually implemented inline) or starting with
a capital.

[shivanisha, 2017/05/17 13:02:30]

Done.

+
+  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.
+  base::WeakPtr<HttpCache> cache_;
+
+  // Owner of this object.
+  ActiveEntry* entry_ = nullptr;
+
+  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.
+  CompletionCallback io_callback_;
+  base::WeakPtrFactory<Writers> weak_factory_;
+  DISALLOW_COPY_AND_ASSIGN(Writers);
+};
+
+}  // namespace net
+#endif  // NET_HTTP_HTTP_CACHE_WRITERS_H_