Fixes the cache lock issue.

net/http/http_cache_shared_writers.h

+// Copyright (c) 2016 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_SHARED_WRITERS_H_
+#define NET_HTTP_HTTP_CACHE_SHARED_WRITERS_H_
+
+#include <list>
+#include <memory>
+#include "base/memory/weak_ptr.h"
+#include "net/base/completion_callback.h"
+#include "net/http/http_cache.h"
+
+namespace net {
+
+class DataAccess;
+
+// SharedWriters 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.
+//
+// An instance of SharedWriters will be created when the first writer has
+// written the new response headers in the cache.
+// At creation:
+// - The writer’s network transaction’s ownership will be transferred to the new
+// DataAccess object owned by SharedWriters and thus can be used by any of the
+// HttpCache::Transactions for subsequent reading from the network.
+// - The pending_queue will be traversed to see if there are other
+// transactions that can be added to SharedWriters.
+//
+// Destroyed when any of the following happen:
+// - All the data has been read from the network and written to cache.
+// - ActiveEntry is destroyed.
+// - Last transaction part of SharedWriters is destroyed.
+// - Response writing is terminated due to a failure.
+// - StopCaching API is invoked and caching was successfully stopped.

[Randy Smith (Not in Mondays), 2017/01/19 00:53:43]

Suggestion: My model for class documentation is that you document the
requirements the class has on the consumers, and the behavior the class provides
to the consumers.  This documentation is describing how the consumers use the
class, which I think of as better being with the consumers than here. 

Having said that, these classes are all intertwined, and I could imagine that
this is the best place for the documentation on how this class is used--I
haven't gotten far enough into this review to say definitively that it isn't. 
But in general (and possibly in specific) I'd recommend sticking to the
"requirements on consumers + behavior provided to consumers" in class
documentation, and figuring out a different place to put documentation on how a
particular class is used.  The idea is that how classes are used should be able
to be changed without changing the class itself, and structuring the
documentation that way helps that goal (by making the interface definition more
precise).

[shivanisha, 2017/01/25 19:46:13]

Agree, thanks. Updated the comments in this file to be independent of the
implementation.

+class HttpCache::SharedWriters {
+ public:
+  // Creates a new SharedWriters object, transfers the ownership of network
+  // transaction to SharedWriters, and adds the cache transaction to
+  // all_writers_.
+  SharedWriters(HttpCache* cache,
+                ActiveEntry* entry,
+                Transaction* cache_transaction,
+                RequestPriority priority,
+                std::unique_ptr<HttpTransaction> network_transaction);

[Randy Smith (Not in Mondays), 2017/01/19 00:53:43]

Recommend documenting lifetimes for raw pointers (I.e. requirements the class
has on its consumer).

[shivanisha, 2017/01/25 19:46:13]

done.

+  ~SharedWriters();
+
+  // Adds a transaction to SharedWriters. Invokes SetShared on the transaction
+  // and either assigns the transaction to validating_transaction_ or inserts it
+  // in waiting_for_validation_. Return value is true if it is the current
+  // validating_transaction_ and false if its waiting.
+  bool AddTransaction(Transaction* transaction);
+
+  // Invokes Read on data_access_, assigns this transaction to
+  // current_writer_ and returns what data_access_->Read() returns.
+  // In case a read is already in progress then this transaction is instead
+  // added to waiting_writers_, read_in_progress is set to true and
+  // ERR_IO_PENDING is returned.
+  int Read(scoped_refptr<IOBuffer> buf,
+           int buf_len,
+           const CompletionCallback& callback,
+           Transaction* transaction,
+           bool* read_in_progress);
+
+  // Invokes CacheWrite on data_access_ and returns the return value returned
+  // from it. Current_writer_ should be equal to transaction.
+  int CacheWrite(scoped_refptr<IOBuffer> buf,
+                 int write_len,
+                 const CompletionCallback& callback,
+                 Transaction* transaction);
+
+  // Callback to be invoked when validating_transaction_ wants to continue with
+  // the entry as the validation successfully matched it.
+  void OnValidationMatch(Transaction* transaction, RequestPriority priority);
+
+  // Callback to be invoked when validating_transaction_ cannot continue with
+  // the entry as the validation did not return a 304.
+  // Its ok to continue writing to cache if this is the only transaction in
+  // SharedWriters, so network_transaction ownership is transferred to
+  // SharedWriters. But if there are other transactions, entry will be doomed
+  // and this transaction will continue reading from the network so the return
+  // value will pass the ownership of the network transaction back to the
+  // transaction.
+  std::unique_ptr<HttpTransaction> OnValidationNoMatch(
+      Transaction* transaction,
+      std::unique_ptr<HttpTransaction> network_transaction,
+      RequestPriority priority);
+
+  // Invoked when DoneReading is invoked on a shared transaction. If
+  // current_writer_ is set, do nothing, else consider it a read completion
+  // and process accordingly.
+  void DoneReading(Transaction* transaction);
+
+  // Invoked when StopCaching is called for a shared writer transaction.
+  // It stops caching only if there are no other transactions in
+  // all_writers_.
+  bool StopCaching(Transaction* transaction);
+
+  // Moves all transactions eligible for shared writing from entry's
+  // pending_queue to waiting_for_validation_.
+  void MoveFromPendingQueue();
+
+  // Posts a task for invoking the io callback of the first transaction in
+  // waiting_for_validation_.
+  void ProcessFirstWaitingValidation();
+
+  // Removes a transaction from waiting_for_validation_ and invokes
+  // ResetShared on it.
+  bool RemoveWaitingTransaction(Transaction* transaction);
+
+  // Removes a transaction which is in all_writers_ but not currently waiting
+  // on Read.
+  void RemoveIdleWriter(Transaction* transaction);
+
+  // Removes a waiting_writer_ transaction.
+  void RemoveWaitingWriter(Transaction* transaction);
+
+  // Removes the currently validating transaction.
+  void RemoveValidatingTransaction(Transaction* transaction);
+
+  // Removes the transaction that is currently accessing data_access_.
+  void RemoveCurrentWriter(Transaction* transaction);
+
+  // Should the owner be able to reset this object? Returns false if
+  // SharedWriters is in a flow where it is about to destroy itself at the end
+  // of the flow.
+  bool CanReset();
+
+  // Returns true if this object is empty: no transactions in all_writers and
+  // waiting_for_validation_ and validating_transaction_.
+  bool empty();
+
+  friend class Transaction;
+
+ private:
+  // Runs the state transition loop. Resets and calls |callback_| on exit,
+  // unless the return value is ERR_IO_PENDING.
+  int DoLoop(int result);
+
+  // IO Completion callback function. May destroy this object if needed at the
+  // end of the processing.
+  void OnIOComplete(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.
+  //
+  // current_writer_ successfully read from the network.
+  void OnNetworkReadSuccess(int len);
+
+  // current_writer_ successfully wrote to the cache.
+  void OnCacheWriteSuccess(int result);
+
+  // current_writer_ fails to write data to the cache.
+  // It transfers the ownership of data_access_ to the transaction since shared
+  // writing can no longer continue, but current_writer_ can continue to read
+  // the remaning data from the network. Any waiting_writers_ will be
+  // notified of the failure. Any idle transactions or validating_transaction_
+  // will be set to a state so that they can fail any subsequent Read calls.
+  // Any transactions in waiting_for_validation_ will move to pending_queue.
+  void OnCacheWriteFailure();
+
+  // current_writer_ fails to read from the network. Both this transaction and
+  // any waiting_writers_ will return the failure code to their consumers.
+  // Any idle transactions or validating_transaction_ will be set to a
+  // state so that they can fail any subsequent Read calls.
+  // Any transactions in waiting_for_validation_ will move to pending_queue.
+  // Marking the entry as truncated will be attempted.
+  void OnNetworkReadFailure(int result);
+
+  // Helper function for doing the cleanup for failure scenarios such as those
+  // mentioned above.
+  void FailureCleanup(int error, bool continue_network_reading);
+
+  // Moves all transactions in waiting_for_validation_ to entry's
+  // pending_queue.
+  void MoveToPendingQueue();
+
+  // Notifies the waiting_writers_ of the result, by posting a task for each
+  // of them. While processing the task for a transaction, it's IO callback
+  // will be invoked.
+  void ProcessWaitingWriters(int result);
+
+  // First waiting_for_validation transaction's callback will be invoked.
+  void OnProcessFirstWaitingValidation();
+
+  // Removes the current_writer_ transaction from this object.
+  void ResetCurrentWriter(bool continue_network_reading = false);
+
+  // Sets the state of idle writers so that they can fail any subsequent
+  // Read.
+  void SetIdleWritersFailState(int result);
+
+  // When response is completely written, any idle writers are moved to
+  // entry_->readers.
+  void MoveIdleWritersToReaders();
+
+  // Helper function to let the validating_transaction_ continue being a part of
+  // SharedWriters.
+  void ValidationDoneContinue(Transaction* transaction,
+                              RequestPriority priority);
+
+  // Helper function invoked when response is successfully written to the cache.
+  void ResponseDataComplete();
+
+  // Deletes itself. This is needed for callback scenarios where we no longer
+  // need shared writing. Entry will release ownership in the flow and
+  // SharedWriters will delete itself in the end of the flow.
+  void SelfDestroy();
+
+  enum State {
+    STATE_NONE,
+    STATE_NETWORK_READ,
+    STATE_NETWORK_READ_COMPLETE,
+    STATE_CACHE_WRITE_DATA,
+    STATE_CACHE_WRITE_DATA_COMPLETE,
+    STATE_CACHE_WRITE_TRUNCATED_RESPONSE,
+    STATE_CACHE_WRITE_TRUNCATED_RESPONSE_COMPLETE
+  };
+
+  State next_state_ = STATE_NONE;

[Randy Smith (Not in Mondays), 2017/01/19 00:53:43]

I think it's good to use either all in-declaration initializers, or all
at-constructor initializers; mixing them means that readers have to look in two
different places (and remember to look in two different places) for where
members are initialized.

[shivanisha, 2017/01/25 19:46:13]

I have tried to keep all simple initializations here and all object/collections
initialization in the constructor. I prefer this approach since looking in the
header file at the point of declaration is sufficient to make sure no variable
is left uninitialized since complex types are initialized by their own
constructors, anyways. Looking at the discussion here
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/zqB-DySA4V0,
this seems inline with the official wording of google style guide "Use in-class
member initialization for simple initializations, especially when a member
variable must be initialized the same way in more than one constructor."
Having said that, I am open to changing that if any of us strongly feel towards
the other approach.

+
+  // Http Cache.
+  base::WeakPtr<HttpCache> cache_;
+
+  // Owner of this object.
+  ActiveEntry* entry_ = nullptr;
+
+  std::unique_ptr<DataAccess> data_access_;
+
+  scoped_refptr<IOBuffer> read_buf_;
+  int io_buf_len_ = 0;
+  int write_len_ = 0;
+
+  // The cache transaction that is the current consumer of data_access_::Read
+  // or data_access_->CacheWrite and is waiting for the operation to be
+  // completed. This is used to ensure there is at most one consumer of
+  // data_access_. After the network read and cache write is successful and data
+  // written to cache, other waiting writers will be notified.
+  Transaction* current_writer_ = nullptr;
+
+  // This will point to the currently validating transaction.
+  // After the validation stage is successfully done, this transaction is
+  // added to all_writers_.
+  Transaction* validating_transaction_ = nullptr;
+
+  // If a transaction is currently set in validating_transaction_, these
+  // transactions will wait for that to complete and then the first of these
+  // will be assigned to validating_transaction_.
+  TransactionList waiting_for_validation_;
+
+  // These transactions are waiting on their Read calls when current_writer_
+  // is not null. After current_writer_ completes writing the data to the
+  // cache, their buffer would be filled with the data and their callback
+  // invoked.
+  struct WaitingWriter {
+    Transaction* transaction;
+    scoped_refptr<IOBuffer> read_buf;
+    int read_buf_len;
+    int write_len;
+    WaitingWriter(Transaction* transaction,
+                  scoped_refptr<IOBuffer> read_buf,
+                  int len);
+    ~WaitingWriter();
+    WaitingWriter(const WaitingWriter&);
+  };
+  typedef std::list<WaitingWriter> WaitingWritersList;
+  WaitingWritersList waiting_writers_;

[Randy Smith (Not in Mondays), 2017/01/19 00:53:43]

Suggestion: I'm finding the reader/writer nomenclature a bit confusing in
context.  "SharedWriters" is about writing to the cache, but really, the
SharedWriters object (and the DataAccess object it owns) is doing the writing. 
Everything other than the current_writer_ will never write to the cache (and the
current_writer_ won't either, conceptually; the SharedWriters object will). 
Maybe switch over to active_transaction_ and waiting_for_read_transactions_
(naming by what the consumer is trying to do), 

[shivanisha, 2017/01/20 16:00:09]

active_transaction_ and waiting_for_read_ sound good to me. How about
all_writers_? I might want to keep all_writers_ as is because it does not
include the transactions in validation phase, so making it all_transactions_
will be confusing.

[shivanisha, 2017/01/25 19:46:13]

Done.

+
+  // Includes a transaction if it is one of the following:
+  // 1. current_writer_ or,
+  // 2. member of waiting_writers_ or,
+  // 3. Waiting for Read to be invoked from the consumer.
+  TransactionSet all_writers_;
+
+  // Current priority of the request. If a higher priority transaction is
+  // added to all_writers_, the priority of data_access_->network_transaction_
+  // will be increased.
+  // todo (shivanisha@): If the higher priority request dies, we do not change
+  // the priority back to a lower one. Fix this. The priority should be the
+  // highest of the existing requests.
+  RequestPriority priority_ = DEFAULT_PRIORITY;
+
+  CompletionCallback callback_;  // Consumer's callback.
+  CompletionCallback io_callback_;
+
+  bool destroy_ = false;  // Should be destroyed at the end of this flow.
+
+  // Return value from network read. Saved so it could be returned to the
+  // consumer after marking the entry as truncated, if needed.
+  int network_read_rv_ = 0;
+
+  base::WeakPtrFactory<SharedWriters> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(SharedWriters);
+};
+
+}  // namespace net
+
+#endif  // NET_HTTP_HTTP_CACHE_SHARED_WRITERS_H_