HttpCache::Transaction layer allowing parallel validation

net/http/http_cache.h

Index: net/http/http_cache.h
diff --git a/net/http/http_cache.h b/net/http/http_cache.h
index 635cb92d557d17c6c60ca459f4117deb075f71f7..a5499fb53389287103de7ff3c94a8ea6cca7c852 100644
--- a/net/http/http_cache.h
+++ b/net/http/http_cache.h
@@ -244,6 +244,9 @@ class NET_EXPORT HttpCache : public HttpTransactionFactory,
   friend class ViewCacheHelper;
   struct PendingOp;  // Info for an entry under construction.
 
+  // To help with testing.
+  friend class MockHttpCache;
+
   using TransactionList = std::list<Transaction*>;
   using TransactionSet = std::unordered_set<Transaction*>;
   typedef std::list<std::unique_ptr<WorkItem>> WorkItemList;
@@ -256,12 +259,56 @@ class NET_EXPORT HttpCache : public HttpTransactionFactory,
     // Returns true if no transactions are associated with this entry.
     bool HasNoTransactions();
 
-    disk_cache::Entry* disk_entry;
-    Transaction*       writer;
+    // Returns true if no active readers/writer transactions are associated
+    // with this entry.
+    bool HasNoActiveTransactions();
+
+    disk_cache::Entry* disk_entry = nullptr;
+
+    // We implement a basic reader/writer lock for the disk cache entry. If

[jkarlin, 2017/04/06 17:37:13]

Pull this comment up above the definition of the ActiveEntry class.

[shivanisha, 2017/04/07 21:11:16]

done and revised this comment for read-only transactions, since they are not
special case anymore.

+    // there is a writer, then all read-only transactions must wait. Non
+    // read-only transactions can proceed to their validation phase. Validation
+    // is allowed for one transaction at a time so that we do not end up with
+    // wasted network requests.
+
+    // The following variables represent the various transactions associated
+    // with this entry in different stages. A transaction goes through these
+    // transitions.
+    //
+    // Write mode transactions:
+    // add_to_entry_queue-> headers_transaction -> done_headers_queue ->

[jkarlin, 2017/04/06 17:37:13]

I'd love to see the state machine description above the definition of
ActiveEntry as well.

[shivanisha, 2017/04/07 21:11:16]

done

+    // writer
+    // add_to_entry_queue-> headers_transaction -> done_headers_queue ->
+    // readers (once the data is written to the cache by another writer)
+    //
+    // Read only transactions:
+    // add_to_entry_queue-> readers (once the data is written to the cache by

[jkarlin, 2017/04/06 17:37:13]

Why special case read-only transactions? It seems like it should follow the
normal path of add_to_entry_queue-> headers_transaction-> done_headers_queue->
readers. 

After F2F we're in agreement about this.

[shivanisha, 2017/04/07 21:11:16]

done

+    // the writer)
+
+    // Transactions waiting to be added to entry.
+    TransactionList add_to_entry_queue;
+
+    // Transaction currently in the headers phase, either validating the
+    // response or getting new headers. This can exist simultaneously with
+    // writer or readers while validating existing headers.
+    Transaction* headers_transaction = nullptr;
+
+    // Transactions that have completed their headers phase and are waiting
+    // to read the response body or write the response body.
+    TransactionList done_headers_queue;
+
+    // Transaction currently reading from the network and writing to the cache.
+    Transaction* writer = nullptr;
+
+    // Transactions that can only read from the cache. Only one of writer or
+    // readers can exist at a time.
     TransactionSet readers;
-    TransactionList    pending_queue;
-    bool               will_process_pending_queue;
-    bool               doomed;
+
+    // The following variables are true if OnProcessQueuedTransactions is posted
+    bool will_process_queued_transactions = false;
+
+    // True if entry is doomed.
+    bool doomed = false;
   };
 
   using ActiveEntriesMap =
@@ -342,28 +389,40 @@ class NET_EXPORT HttpCache : public HttpTransactionFactory,
   // Destroys an ActiveEntry (active or doomed).
   void DestroyEntry(ActiveEntry* entry);
 
-  // Adds a transaction to an ActiveEntry. If this method returns ERR_IO_PENDING
-  // the transaction will be notified about completion via its IO callback. This
-  // method returns ERR_CACHE_RACE to signal the transaction that it cannot be
-  // added to the provided entry, and it should retry the process with another
-  // one (in this case, the entry is no longer valid).
-  int AddTransactionToEntry(ActiveEntry* entry, Transaction* trans);
+  // Adds a transaction to an ActiveEntry. This method returns ERR_IO_PENDING
+  // and the transaction will be notified about completion via its IO callback.

[jkarlin, 2017/04/06 17:37:13]

Comment on what error to expect on failure.

[shivanisha, 2017/04/07 21:11:16]

done

+  int AddTransactionToEntry(ActiveEntry* entry, Transaction* transaction);
+
+  // Transaction invokes this when its response headers phase is complete
+  // either on validating existing headers/skipping validation or getting new
+  // headers. The transaction is added to a queue and a task is posted
+  // to process queued transactions of the entry. Returns ERR_IO_PENDING.

[jkarlin, 2017/04/06 17:37:13]

Comment on what error to expect on failure.

[jkarlin, 2017/04/06 17:37:13]

Add "the transaction will be notified about completion via its IO callback" as
in the comment for AddTransactionToEntry

[shivanisha, 2017/04/07 21:11:16]

done

+  int DoneWithResponseHeaders(ActiveEntry* entry, Transaction* transaction);
 
   // Called when the transaction has finished working with this entry. |cancel|
   // is true if the operation was cancelled by the caller instead of running
   // to completion.
-  void DoneWithEntry(ActiveEntry* entry, Transaction* trans, bool cancel);
+  void DoneWithEntry(ActiveEntry* entry, Transaction* transaction, bool cancel);
+
+  // Returns true if this transaction can write headers to the entry.
+  bool CanTransactionWriteResponseHeaders(ActiveEntry* entry,
+                                          Transaction* transaction,
+                                          int response_code) const;
+
+  // Returns true if this transaction is currently writing or about to start
+  // writing the response body.

[jkarlin, 2017/04/06 17:37:13]

Given the above method, and the name of this method, I'd expect the comment to
say:

"Returns true if this transaction can write response body bytes to the entry."

Instead it's really a method that tries to answer two questions:

1) Can the transaction write to the response body?
2) Did the transaction start writing to this entry but not yet finish?

I believe we can split those into two functions:

1) CanTransactionWriteResponseBody
2) IsTransactionWritingIncomplete

WDYT?

[shivanisha, 2017/04/07 21:11:16]

I tried doing that and made CanTransactionWriteResponseBody return true if
transaction is the current writer and dcheck(CanTransactionWriteResponseBody) in
DoCacheWriteData but one of the tests GET_Crazy416 fails because the transaction
is still headers_transaction. It is a test which tests incorrect receiving of
416 for a non-range request. Removing this dcheck and this function and keeping
one function IsTransactionWritingIncomplete to be called in the destructor.

[jkarlin, 2017/04/11 14:38:28]

Acknowledged.

+  bool CanTransactionWriteResponseBody(ActiveEntry* entry,
+                                       Transaction* transaction,
+                                       const std::string& method) const;
 
   // Called when the transaction has finished writing to this entry. |success|
   // is false if the cache entry should be deleted.
-  void DoneWritingToEntry(ActiveEntry* entry, bool success);
+  void DoneWritingToEntry(ActiveEntry* entry,
+                          bool success,
+                          Transaction* transaction);
 
   // Called when the transaction has finished reading from this entry.
-  void DoneReadingFromEntry(ActiveEntry* entry, Transaction* trans);
-
-  // Converts the active writer transaction to a reader so that other
-  // transactions can start reading from this entry.
-  void ConvertWriterToReader(ActiveEntry* entry);
+  void DoneReadingFromEntry(ActiveEntry* entry, Transaction* transaction);
 
   // Returns the LoadState of the provided pending transaction.
   LoadState GetLoadStateForPendingTransaction(const Transaction* trans);
@@ -379,12 +438,34 @@ class NET_EXPORT HttpCache : public HttpTransactionFactory,
   // Removes the transaction |trans|, from the pending list of |pending_op|.
   bool RemovePendingTransactionFromPendingOp(PendingOp* pending_op,
                                              Transaction* trans);
-  // Resumes processing the pending list of |entry|.
-  void ProcessPendingQueue(ActiveEntry* entry);
+
+  // Removes and returns all queued transactions in |entry| in FIFO order. This
+  // includes transactions that have completed the headers phase and those that
+  // have not been added to the entry yet in that order. |list| is the output
+  // argument.
+  void RemoveAllQueuedTransactions(ActiveEntry* entry, TransactionList* list);

[jkarlin, 2017/04/06 17:37:13]

Note that these methods need to appear in cc file in the same order. Right now
ProcessEntryFailure appears above ProcessQueuedTransactions.

[shivanisha, 2017/04/07 21:11:16]

done

+
+  // Resumes processing the queued transactions of |entry|.
+  void ProcessQueuedTransactions(ActiveEntry* entry);
+
+  // Checks if a transaction can be added to the entry. If yes, it will
+  // invoke the IO callback of the transaction. This is a helper function for
+  // OnProcessQueuedTransactions.
+  void ProcessAddToEntryQueue(ActiveEntry* entry);
+
+  // Checks if a transaction that has already completed the response headers
+  // phase can resume reading/writing the response body. If yes, it will
+  // invoke the IO callback of the transaction. This is a helper function for
+  // OnProcessQueuedTransactions.
+  void ProcessDoneHeadersQueue(ActiveEntry* entry);
+
+  // Processes either writer's failure to write response body or
+  // headers_transactions's failure to write headers.
+  void ProcessEntryFailure(ActiveEntry* entry);
 
   // Events (called via PostTask) ---------------------------------------------
 
-  void OnProcessPendingQueue(ActiveEntry* entry);
+  void OnProcessQueuedTransactions(ActiveEntry* entry);
 
   // Callbacks ----------------------------------------------------------------