HttpCache::Transaction layer allowing parallel validation

net/http/http_transaction.h

 // Copyright (c) 2011 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_TRANSACTION_H_
 #define NET_HTTP_HTTP_TRANSACTION_H_
 
 #include <stdint.h>
 
 #include "net/base/completion_callback.h"
@@ skipping 34 matching lines @@
   // additional headers may be added to |request_headers|.
   typedef base::Callback<void(const ProxyInfo& proxy_info,
                               HttpRequestHeaders* request_headers)>
       BeforeHeadersSentCallback;
 
   // Stops any pending IO and destroys the transaction object.
   virtual ~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
   // indicate that the CompletionCallback will be notified once response info is
   // available or if an IO error occurs.  Any other return value indicates that
   // the transaction could not be started.
   //
   // Regardless of the return value, the caller is expected to keep the
   // request_info object alive until Destroy is called on the transaction.
   //
@@ skipping 124 matching lines @@
 
   // Resumes the transaction after being deferred.
   virtual int ResumeNetworkStart() = 0;
 
   virtual void GetConnectionAttempts(ConnectionAttempts* out) const = 0;
 };
 
 }  // namespace net
 
 #endif  // NET_HTTP_HTTP_TRANSACTION_H_