HttpCache::Transaction layer allowing parallel validation

net/http/http_transaction.h

Index: net/http/http_transaction.h
diff --git a/net/http/http_transaction.h b/net/http/http_transaction.h
index eff45d10c5648d618e4366a6ea91a398b06016b1..6b5bce143c6e3bf38838f5781de372c77aeb38fe 100644
--- a/net/http/http_transaction.h
+++ b/net/http/http_transaction.h
@@ -53,8 +53,8 @@ class NET_EXPORT_PRIVATE HttpTransaction {
   // Starts the HTTP transaction (i.e., sends the HTTP request).
   //
   // The consumer should ensure that request_info points to a valid value till

[Randy Smith (Not in Mondays), 2017/05/09 23:20:32]

nit: "till" -> "while"

[shivanisha, 2017/05/10 14:46:03]

done

-  // final response headers are received; after that point, the HttpTransaction
-  // will not access |*request_info| and it may be deleted.
+  // the consumer is alive; after that point, the HttpTransaction will not

[Randy Smith (Not in Mondays), 2017/05/09 23:20:32]

How does the HttpTransaction know when its consumer dies?  

(I had thought we kept the API contract to being "the HttpTransaction will be
destroyed when its consumer is destroyed"; i.e. the HttpTransaction would be
owned by its consumer.  In that case, I think this requirement translates to
"The consumer must ensure that |*request_info| remains alive for the lifetime of
the HttpTransaction.", which is both a simple and more common interface
contract.)

[shivanisha, 2017/05/10 14:46:03]

In that case, I think this requirement translates to "The consumer must ensure
that |*request_info| remains alive for the lifetime of the HttpTransaction.",
which is both a simple and more common interface contract.)

That's true for HttpCache::Transaction but not for HttpNetworkTransaction since
when we land parallel writing, it will be owning HttpNetworkTransaction even
after consumer has died.

[Randy Smith (Not in Mondays), 2017/05/10 21:34:26]

Ok, I think we're using terminology differently: When I say "consumer" for a
particular network transaction, I mean the immediate class that's driving it. 
So the consumer for the HttpNetworkTransaction would be the owning
HttpCacheTransaction.  

If the lifetime for |*request_info| is the lifetime of the owning
URLRequestHttpJob, then my concern is even stronger.  How is the
HttpNetworkTransaction going to know when that dies?  If it can't, how can this
interface contract be useful for the HttpNetworkTransaction if it can't know?  

I'm inclined to think that interface contracts with regard to time should be
written s.t. both the (immediate) consumer and implementing class have access to
the event occurrences that are mentioned.  Otherwise it doesn't really make
sense as an interface contract.  But I suspect there's something I'm missing
about event visibility here.

[shivanisha, 2017/05/11 19:58:24]

Agree with consumer being the immediate consumer which means
HttpCache::Transaction for HttpNetworkTransaction in a typical case. 
HttpNetworkTransaction will not know that its consumer that passed the
request_info to it in the first place has died, but it makes an approximation by
nulling the request info pointer when headers are received. That made the
comment refer to headers in the first place. But then for another derived class
of HttpTransaction, HttpCache::Transaction, headers may be received multiple
time (for partial requests) and thus the headers boundary does not apply to it.
So the newest comment in a way wants to tell the HttpTransaction in question to
make an inference if it thinks its initial consumer might die before it and null
the request info ptr accordingly at the right point.
Since HC::T knows that it will not outlive its initial consumer , it doesn't
really need to null the request info pointer.

[Randy Smith (Not in Mondays), 2017/05/11 21:47:09]

First of all, remind me what the intended use case is for this bit of interface
contract?   I'm remembering doing this when you were going to orphan
HttpCache::Transactions to allow them to drive HttpNetworkTransaction after
their owner (URLRequestHttpJob) had died, but I thought we've moved away from
that design.

Unless we have a specific current or soon expected use case in which an
HttpTransaction will outlive its consumer, I really think this should just be
"The consumer must ensure that |*request_info| outlives the HttpTransaction." 
If we do have such a use case, I think this needs to be phrased in terms of what
the requirements on the consumer are s.t. the transaction will be able to access
|*request_info| appropriately.  

This comment is tricky, because it's specifying requirements for *both* the
consumer and for the actual subclass implementing HttpTransaction.  But that
means that what it specifies needs to be visible to both of those targets. 
Since the HttpTransaction doesn't have visibility into when it's consumer dies,
I don't like the current spec.

+  // access |*request_info|.
   //
   // Returns OK if the transaction could be started synchronously, which means
   // that the request was served from the cache.  ERR_IO_PENDING is returned to