Implementation of network level throttler.

net/http/http_network_transaction.cc

Index: net/http/http_network_transaction.cc
diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc
index 120436443e4128f972a61b6982c5d308275e4459..906a30a479b0a40c0e093e348ad19145ad352d1d 100644
--- a/net/http/http_network_transaction.cc
+++ b/net/http/http_network_transaction.cc
@@ -120,7 +120,8 @@ HttpNetworkTransaction::HttpNetworkTransaction(RequestPriority priority,
       next_state_(STATE_NONE),
       establishing_tunnel_(false),
       websocket_handshake_stream_base_create_helper_(NULL),
-      net_error_details_() {
+      net_error_details_(),
+      throttle_(session->throttler()->CreateThrottle(priority)) {

[mmenke, 2016/04/18 14:57:55]

Also, making this part of the HttpNetworkSession seems a bit weird - currently,
the HttpNetworkSession basically only has the HttpStreamFactory, and things it
directly or transitively depends on, which doesn't include the throttler.

Though then there's the question of where this does belong -
HttpNetworkTransaction doesn't currently have any other notion of
per-network-stack globals, and stashing it in NetworkLayer seems weird.  This
also has the advantage that URLRequestContexts that share HttpNetworkSessions
share it as well.

Think this is worth some thought.

[mmenke, 2016/04/18 14:57:55]

BUG:  Needs load_flags & LOAD_IGNORE_LIMITS as an argument.  We don't actually
know load_flags here, currently.  Could delay creation or change that, though if
you go the latter route, you'd need to modify TransactionFactory, I assume.

[Randy Smith (Not in Mondays), 2016/04/19 11:37:05]

Hmmm.  I'm not sure I see a lot of good alternatives while keeping it at this
place in the stack (which at least at the moment strikes me as the right place
for it).  I could put it on HttpStreamFactoryImpl, which would be good if we
ever need to push the throttler down into the HttpStreamRequest/HttpStream?  

[Randy Smith (Not in Mondays), 2016/04/19 11:37:05]

Acknowledged.

[mmenke, 2016/04/19 13:52:10]

I'll think a bit about it - this may be the best place to put it.

[Randy Smith (Not in Mondays), 2016/04/20 21:57:38]

Done.

   session->ssl_config_service()->GetSSLConfig(&server_ssl_config_);
   session->GetAlpnProtos(&server_ssl_config_.alpn_protos);
   session->GetNpnProtos(&server_ssl_config_.npn_protos);
@@ -605,6 +606,15 @@ void HttpNetworkTransaction::GetConnectionAttempts(
   *out = connection_attempts_;
 }
 
+void HttpNetworkTransaction::OnThrottleStateChanged() {
+  // TODO(rdsmith): This DCHECK is dependent on the throttle state
+  // initializing as throttled only transition from throttled->unthrottled.
+  // That is true right now, but may not be so in the future.
+  DCHECK_EQ(STATE_NOTIFY_BEFORE_CREATE_STREAM, next_state_);

[mmenke, 2016/04/18 14:57:55]

This seems fragile to me, as it requires a transaction always be started
immediately after construction.

[Randy Smith (Not in Mondays), 2016/04/19 11:37:05]

So I agree in the abstract, and am happy to change it, but I'd like to
understand your concern in more detail than I currently do.  Are you referring
to the fact that if the transaction isn't started right after construction there
will be a record of an outstanding transaction that the throttler thinks is
running but that isn't actually running and taking up network bandwidth?  (Which
would mean we apply more throttling than we should).  Or is there a possible
(future?) set of state transitions that you think would actually break
functionality?

[mmenke, 2016/04/19 13:52:10]

It was actually more a comment about the correctness of the DCHECK.  If we don't
start the transaction immediately after creation (I assume we do this, but the
cache transaction is weird), or if we add any states before this which we can
get stuck at, this DCHECK can fail.

It may be better to do something like:

DoStart() {
  next_state_ = STATE_THROTTLE;
  ...
}

DoStartThrottling() {
  // This is what actually starts us waiting.
  throttle_.reset(new Throttle(this));
  next_state_ = STATE_NOTIFY_BEFORE_CREATE_STREAM;
  if (throttle_.throttled())
    return ERR_IO_PENDING;
  return OK;
}

Or could get rid of throttle and just do:

DoStartThrottling() {
  next_state_ = STATE_NOTIFY_BEFORE_CREATE_STREAM;
  return ThrottleService->StartThrottling(this);  // Returns OK if we can start
immediately.
}

And unregister with the ThrottleService (Or whatever we call it) on destruction,
at least if we ever registered with the throttler.  That explicit call may make
things a little clearer, but I think either's fine.

[Randy Smith (Not in Mondays), 2016/04/20 21:57:38]

Done.

+
+  DoLoop(OK);
+}
+
 bool HttpNetworkTransaction::IsSecureRequest() const {
   return request_->url.SchemeIsCryptographic();
 }
@@ -787,6 +797,11 @@ int HttpNetworkTransaction::DoLoop(int result) {
 }
 
 int HttpNetworkTransaction::DoNotifyBeforeCreateStream() {
+  // If this transaction is currently throttled, the throttler will notify
+  // when it is unthrottled through OnThrottleStateChanged().
+  if (throttle_->throttled())
+    return ERR_IO_PENDING;
+
   next_state_ = STATE_CREATE_STREAM;
   bool defer = false;
   if (!before_network_start_callback_.is_null())