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 9917c69f04b81d4326bccfbca4bce66f6f9f7b8a..d732e49afc7241043e2905eb5178f9f70601d837 100644
--- a/net/http/http_network_transaction.cc
+++ b/net/http/http_network_transaction.cc
@@ -604,6 +604,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_);
+
+  DoLoop(OK);
+}
+
 bool HttpNetworkTransaction::IsSecureRequest() const {
   return request_->url.SchemeIsCryptographic();
 }
@@ -674,6 +683,9 @@ int HttpNetworkTransaction::DoLoop(int result) {
     State state = next_state_;
     next_state_ = STATE_NONE;
     switch (state) {
+      case STATE_THROTTLE:
+        DCHECK_EQ(OK, rv);
+        rv = DoThrottle();
       case STATE_NOTIFY_BEFORE_CREATE_STREAM:
         DCHECK_EQ(OK, rv);
         rv = DoNotifyBeforeCreateStream();
@@ -785,6 +797,21 @@ int HttpNetworkTransaction::DoLoop(int result) {
   return rv;
 }
 
+int HttpNetworkTransaction::DoThrottle() {
+  throttle_ = session_->throttler()->CreateThrottle(
+      priority_, request_->load_flags & LOAD_IGNORE_LIMITS);

[mmenke, 2016/04/29 16:57:28]

The implicit conversion of "request_->load_flags & LOAD_IGNORE_LIMITS" to a bool
doesn't generate an error of some sort?

[mmenke, 2016/04/29 16:57:28]

One ownership suggestion:  We could make the throttler an optional
URLRequestContext component.  Does make network stack setup marginally more
complex than it is, particularly if we allow configuring the throttle, but it
also lets us worry about Cronet and throttling in isolation.  Also adds yet more
cases to test.  :(

[Randy Smith (Not in Mondays), 2016/05/08 00:46:30]

So I'm inclined against that given the current approach, but would be happy to
re-examine the current approach.  The problem is that the throttler is first
intended to implement the semantics of the new priority level (THROTTLED), which
is part of the Cronet interface by virtue of being listed in
net::RequestPriority.  Given that, I want the throttler in Cronet :-}.  I could
imagine a different abstraction, where the name of the priority level did not
reflect those specific semantics, and there was an embedder callout for
throttling policy which determined what actually happened at the different
priority levels; in that world, I think making throttling an optional
URLRequestContext component would make sense.  That seems more reasonable when I
consider the second intended use of the throttler, which is to move the
ResourceScheduler throttling of IDLE requests down into net/, which seems a very
reasonable embedder policy decision.  

Given all this, do you think I should change the name of THROTTLED, make the
throttler optional, and setup something to allow the embedder to set policy?  If
so, any thoughts as to a reasonable new name?

[Randy Smith (Not in Mondays), 2016/05/08 00:46:31]

Heh.  Only on windows.  Even more confusing, this idiom seems present in other
areas of the chrome code base, e.g.
https://chromium.googlesource.com/chromium/src/+blame/master/chrome/browser/r...
.  But that doesn't look like the network stack behavior (though there's a
DCHECK ... :-?), so I'll change it.  Fixed.

[mmenke, 2016/05/09 17:37:48]

So it's one thing to do "if (x & FLAG)" - that takes an int, and does something
or not if it's non-zero.  That's clear cut, and all well and good.  I don't
trust compilers to store only 0 or 1 in a bool when coercing an integer to it,
however.

[mmenke, 2016/05/09 17:37:48]

Do you think it's that implementing needed behavior for the throttled is all
this code is ever likely to do?  I'm skeptical of that, and I'm concerned about
other behaviors bleeding into places it's not expected.

[mmenke, 2016/05/09 18:21:21]

Hrm...thinking a bit more on this, I really like the THROTTLED name, as it makes
clearer what's going on...At the same time, I am a bit concerned about behavior
we may not want in Cronet (Or for sync, or websockets, or whatever) creeping in
here, which is what made me start thinking about making this optional (As well
as the "where should this live" question).

[mmenke, 2016/05/09 20:49:41]

Erm...Hope you managed to understand that, but that should be "Do you think that
implementing behavior needed for throttled requests is all this class is ever
likely to do".  One of these days, I'll learn to proofread.

[Randy Smith (Not in Mondays), 2016/05/09 22:12:54]

No, actually, my current plan is very much to do more than that; sorry I didn't
make that clear above.  The first thing I'm going to do with this class is
implement THROTTED, but it's very much not the only thing I'm planning to do
with it--the immediate follow-on is  implement the throttling that's currently
happening for IDLE & LOWEST in ResourceScheduler down here with an eye towards
removing ResourceScheduler.  

Thus my final question.  I can definitely see wanting to do a policy/mechanism
separation here, but to me that implies that THROTTLED might not be throttled,
and hence should have a different name.  Thoughts?

[Randy Smith (Not in Mondays), 2016/05/09 22:12:54]

Yeah, this :-}.  I'm not really worried about sync or websockets, as they won't
be using the new priority, and I believe that they go through the
ResourceScheduler currently (well, ok, at least that websockets does--we may
want to examine sync more closely) so they should get whatever throttling
happens around IDLE.  But I'm completely with you on the tension between keeping
THROTTLED and between pulling policy out of the netstack.  My inclination is to
change the THROTTLED name and do a policy/mechanism separation, but I'm happy
not to if you prefer. 

If we do change the name, I want to go for something that captures the "higher
priority requests coming soon" nature of THROTTLED (i.e. describe the nature of
the request rather than what the network stack is going to do with it). 
PRELIMINARY?  (With a comment as to what that means.)  MORE_COMING?  (Mostly
:-}).  SPECULATIVE?

[mmenke, 2016/05/11 18:12:38]

I'm not concerned about the changes to how THROTTLED requests are delayed, since
as you say, no one is currently making any of those.  What I am concerned about,
however, is changing how requests with other priorities are throttled. 
Consider, for instance, your comment above about throttling LOWEST and IDLE
priority requests.
Are these all going to speculative?  If this is only intended for use with
prefetch (At least within a web request context), SPECULATIVE sounds great.  I'm
not sure what we're planning to use it for (I probably knew at one point, but my
cache floweth over).

+
+  // If this transaction is currently throttled, the throttler will notify
+  // when it is unthrottled through OnThrottleStateChanged().
+  if (throttle_->throttled()) {
+    next_state_ = STATE_THROTTLE;
+    return ERR_IO_PENDING;
+  }
+
+  next_state_ = STATE_NOTIFY_BEFORE_CREATE_STREAM;
+  return OK;
+}
+
 int HttpNetworkTransaction::DoNotifyBeforeCreateStream() {
   next_state_ = STATE_CREATE_STREAM;
   bool defer = false;