Implementation of network level throttler.

Implementation of network level throttler.

NetworkThrottleManager serves throttle objects to consumers (currently
HttpNetworkTransaction) that inform the consumer of their throttled state
and let the throttler track their lifetime.

BUG=600839
R=mmenke@chromium.org

Committed: https://crrev.com/1d343be5a6202b9f5acaa8c04131fa159d0627c0
Cr-Commit-Position: refs/heads/master@{#426879}

[Randy Smith (Not in Mondays), 2016-04-17 22:45:34]

Matt: A design review for you in the shape of a CL :-}.  In written form: I've
created a new class (NetworkStreamThrottler) to abstract away the throttling
decisions from the rest of the stack.  It vends Throttle objects through which
it informs consumers of their throttle state and tracks the lifetime of those
consumers.  Rather than intercept at HttpStreamFactoryImpl::RequestStream (as I
had talked to you about) I've made the Throttle objects be owned by
HttpNetworkTransactions, which solves the problem of handing off the lifetime of
the stream from HttpStreamRequest to HttpStream.

Currently the NetworkSttreamThrottler has a null implementation, which obviously
is going to change :-}.  What I'm looking for right now is how you feel about
the separation of responsibilities and plumbing in the CL (including tying the
throttle lifetime to the network transaction).  

One thing I dislike about this design long-term is that it doesn't provide any
easy way to let DNS requests through but block connection requests.   However,
a) those are both architecturally separate from transactions, and right now I'm
throttling transactions, and b) from the perspective of replacing
ResourceScheduler, this is a fine place to throttle.  So I figure I'll deal with
the possible need to separate DNS & connection throttling behaviors at some
point in the future.

Let me know what you think.

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

A couple quick comments.  Overall, this approach seems very reasonable to me.

https://codereview.chromium.org/1901533002/diff/1/net/http/http_network_trans...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1901533002/diff/1/net/http/http_network_trans...
net/http/http_network_transaction.cc:124:
throttle_(session->throttler()->CreateThrottle(priority)) {
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.

https://codereview.chromium.org/1901533002/diff/1/net/http/http_network_trans...
net/http/http_network_transaction.cc:124:
throttle_(session->throttler()->CreateThrottle(priority)) {
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.

https://codereview.chromium.org/1901533002/diff/1/net/http/http_network_trans...
net/http/http_network_transaction.cc:613:
DCHECK_EQ(STATE_NOTIFY_BEFORE_CREATE_STREAM, next_state_);
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]

Thanks for the review!

Possibly we should talk about where the throttler should live in person--I don't
know the netstack architecture well enough to suggests alternatives beyond what
I do below.

https://codereview.chromium.org/1901533002/diff/1/net/http/http_network_trans...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1901533002/diff/1/net/http/http_network_trans...
net/http/http_network_transaction.cc:124:
throttle_(session->throttler()->CreateThrottle(priority)) {
On 2016/04/18 14:57:55, mmenke wrote:
> 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.

Acknowledged.

https://codereview.chromium.org/1901533002/diff/1/net/http/http_network_trans...
net/http/http_network_transaction.cc:124:
throttle_(session->throttler()->CreateThrottle(priority)) {
On 2016/04/18 14:57:55, mmenke wrote:
> 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.

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?

https://codereview.chromium.org/1901533002/diff/1/net/http/http_network_trans...
net/http/http_network_transaction.cc:613:
DCHECK_EQ(STATE_NOTIFY_BEFORE_CREATE_STREAM, next_state_);
On 2016/04/18 14:57:55, mmenke wrote:
> This seems fragile to me, as it requires a transaction always be started
> immediately after construction.

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]

Note that I'm thinking about taking a day off today.

https://codereview.chromium.org/1901533002/diff/1/net/http/http_network_trans...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1901533002/diff/1/net/http/http_network_trans...
net/http/http_network_transaction.cc:124:
throttle_(session->throttler()->CreateThrottle(priority)) {
On 2016/04/19 11:37:05, Randy Smith - Not in Fridays wrote:
> On 2016/04/18 14:57:55, mmenke wrote:
> > 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.
> 
> 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?  

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

https://codereview.chromium.org/1901533002/diff/1/net/http/http_network_trans...
net/http/http_network_transaction.cc:613:
DCHECK_EQ(STATE_NOTIFY_BEFORE_CREATE_STREAM, next_state_);
On 2016/04/19 11:37:05, Randy Smith - Not in Fridays wrote:
> On 2016/04/18 14:57:55, mmenke wrote:
> > This seems fragile to me, as it requires a transaction always be started
> > immediately after construction.
> 
> 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?

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-19 14:02:17]

On 2016/04/19 13:52:10, mmenke wrote:
> Note that I'm thinking about taking a day off today.

No worries; I have several irons in the fire.  I'll adapt easily to whatever
your response latency ends up being.

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

Matt: I've shifted the throttle creation to its own state before
NOTIFY_BEFORE_CREATE and added the load flags check to the arguments.  I'm
hoping I'll get guidance from you on where the throttler should live.  

I'm also wondering a bit about CL granularity.  There are a couple of different
changes that are relevant to the current work:
* This CL, which plumbs a new throttler into the stack.
* https://codereview.chromium.org/1866483002/: Which adds a priority to the
priority list.
* A change to actually implement the semantics of that new priority as we've
previously discussed.
* A change to use that new priority from a higher level consumer of the network
stack.

My bias is not to land CLs that don't actually have an effect on the behavior of
the browser, but I also have a bias to make CLs  as small as I can :-}, and
those changes have a pretty clean linear dependency.  I'd propose to run the
first three changes by you in three separate CLs but plan on landing them all
together, and doing the fourth change separately.  But if you'd like a different
plan, I'm open to it.

https://codereview.chromium.org/1901533002/diff/1/net/http/http_network_trans...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1901533002/diff/1/net/http/http_network_trans...
net/http/http_network_transaction.cc:124:
throttle_(session->throttler()->CreateThrottle(priority)) {
On 2016/04/18 14:57:55, mmenke wrote:
> 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.

Done.

https://codereview.chromium.org/1901533002/diff/1/net/http/http_network_trans...
net/http/http_network_transaction.cc:613:
DCHECK_EQ(STATE_NOTIFY_BEFORE_CREATE_STREAM, next_state_);
On 2016/04/19 13:52:10, mmenke wrote:
> On 2016/04/19 11:37:05, Randy Smith - Not in Fridays wrote:
> > On 2016/04/18 14:57:55, mmenke wrote:
> > > This seems fragile to me, as it requires a transaction always be started
> > > immediately after construction.
> > 
> > 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?
> 
> 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.

Done.

[mmenke, 2016-04-22 18:06:11]

On 2016/04/20 21:57:38, Randy Smith - Not in Fridays wrote:
> Matt: I've shifted the throttle creation to its own state before
> NOTIFY_BEFORE_CREATE and added the load flags check to the arguments.  I'm
> hoping I'll get guidance from you on where the throttler should live.  
> 
> I'm also wondering a bit about CL granularity.  There are a couple of
different
> changes that are relevant to the current work:
> * This CL, which plumbs a new throttler into the stack.
> * https://codereview.chromium.org/1866483002/: Which adds a priority to the
> priority list.
> * A change to actually implement the semantics of that new priority as we've
> previously discussed.
> * A change to use that new priority from a higher level consumer of the
network
> stack.
> 
> My bias is not to land CLs that don't actually have an effect on the behavior
of
> the browser, but I also have a bias to make CLs  as small as I can :-}, and
> those changes have a pretty clean linear dependency.  I'd propose to run the
> first three changes by you in three separate CLs but plan on landing them all
> together, and doing the fourth change separately.  But if you'd like a
different
> plan, I'm open to it.

I'm fine with separate reviews, and find it makes my life a lot easy.  I'd
actually vote for landing them separately, since it makes reverting simpler. 
Landing 3 interdependent CLs at once makes it more difficult to revert things to
get everything into a functional state.  Up to you, though.

>
https://codereview.chromium.org/1901533002/diff/1/net/http/http_network_trans...
> File net/http/http_network_transaction.cc (right):
> 
>
https://codereview.chromium.org/1901533002/diff/1/net/http/http_network_trans...
> net/http/http_network_transaction.cc:124:
> throttle_(session->throttler()->CreateThrottle(priority)) {
> On 2016/04/18 14:57:55, mmenke wrote:
> > 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.
> 
> Done.
> 
>
https://codereview.chromium.org/1901533002/diff/1/net/http/http_network_trans...
> net/http/http_network_transaction.cc:613:
> DCHECK_EQ(STATE_NOTIFY_BEFORE_CREATE_STREAM, next_state_);
> On 2016/04/19 13:52:10, mmenke wrote:
> > On 2016/04/19 11:37:05, Randy Smith - Not in Fridays wrote:
> > > On 2016/04/18 14:57:55, mmenke wrote:
> > > > This seems fragile to me, as it requires a transaction always be started
> > > > immediately after construction.
> > > 
> > > 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?
> > 
> > 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.
> 
> Done.

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

Some more comments.  Hope to get to your other CL later today.  Sorry for the
delay.

https://codereview.chromium.org/1901533002/diff/20001/net/base/network_stream...
File net/base/network_stream_throttler.cc (right):

https://codereview.chromium.org/1901533002/diff/20001/net/base/network_stream...
net/base/network_stream_throttler.cc:60: RequestPriority old_priority,
I don't think we need old_priority as an argument?  If we want it, queue_pointer
already has it, anyways.

https://codereview.chromium.org/1901533002/diff/20001/net/base/network_stream...
File net/base/network_stream_throttler.h (right):

https://codereview.chromium.org/1901533002/diff/20001/net/base/network_stream...
net/base/network_stream_throttler.h:22: virtual ~Delegate() {}
Make the destructor protected?  Doesn't matter much, but makes it clearer the
throttler doesn't delete these.

https://codereview.chromium.org/1901533002/diff/20001/net/base/network_stream...
net/base/network_stream_throttler.h:35: bool throttled() { return throttled_; }
const

https://codereview.chromium.org/1901533002/diff/20001/net/base/network_stream...
net/base/network_stream_throttler.h:39: void set_delegate(Delegate* delegate) {
delegate_ = delegate; }
Can we make this a constructor argument instead?

https://codereview.chromium.org/1901533002/diff/20001/net/base/network_stream...
net/base/network_stream_throttler.h:52: base::WeakPtr<NetworkStreamThrottler>
throttler);
Is there any case where the Throttle can be destroyed before the
NetworkStreamThrottler?  Generally, if transactions outlive any component of the
network stack, we're doomed, anyways.

https://codereview.chromium.org/1901533002/diff/20001/net/base/network_stream...
net/base/network_stream_throttler.h:54: void NotifyThrottleStateChanged(bool
new_throttle_state);
I don't think we need to take a bool?  Seems like we'll only unthrottle
requests.  If we want to start unthrottled (Which presumably we will), should
make that a constructor parameter.

https://codereview.chromium.org/1901533002/diff/20001/net/base/network_stream...
net/base/network_stream_throttler.h:55: QueuePointer queue_pointer() { return
queue_pointer_; }
const?

https://codereview.chromium.org/1901533002/diff/20001/net/http/http_network_t...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1901533002/diff/20001/net/http/http_network_t...
net/http/http_network_transaction.cc:802: priority_, request_->load_flags &
LOAD_IGNORE_LIMITS);
The implicit conversion of "request_->load_flags & LOAD_IGNORE_LIMITS" to a bool
doesn't generate an error of some sort?

https://codereview.chromium.org/1901533002/diff/20001/net/http/http_network_t...
net/http/http_network_transaction.cc:802: priority_, request_->load_flags &
LOAD_IGNORE_LIMITS);
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.  :(

[mmenke, 2016-04-29 18:03:15]

I think it's worth noting that we're talking about bringing GRPC to Chrome. 
GRPC creates bidirectional streams (Where throttling may or may not make sense -
we may want to throttle background streams to speed up pay loads, though
initially, at least, it may only be used for blimp, where it is actually used
just for page loads, so throttling may be less useful).  It also hooks up below
the HttpNetworkTransaction layer.

Think we're fine for the foreseeable future, but something to think about.

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

All comments incorporated and a simple test added; PTAL?  As with the other CL
(adding the THROTTLED priority level) I don't plan to land this until I can land
that one and one that actually implements its semantics in reasonable close
temporal proximity.

WRT the test, my thoughts were:
* The behavior of HttpNetworkTransaction isn't changing (in this CL), so there's
no need to make changes there.
* The only observable behavior of the throttler is vending objects that are
unthrottled, so that's what I tested :-}.  When I actually put some smarts in,
I'll expand the test.
Let me know if you think more is needed on the test side.  

I'm also curious if you have any opinions about the asynchrony/if a post task is
necessary for throttle changed notifications. 

Thanks much in advance!

https://codereview.chromium.org/1901533002/diff/20001/net/base/network_stream...
File net/base/network_stream_throttler.cc (right):

https://codereview.chromium.org/1901533002/diff/20001/net/base/network_stream...
net/base/network_stream_throttler.cc:60: RequestPriority old_priority,
On 2016/04/29 16:57:28, mmenke wrote:
> I don't think we need old_priority as an argument?  If we want it,
queue_pointer
> already has it, anyways.

Ah, confusing implementations in my head.  PriorityQueue takes care of that. 
Done.

https://codereview.chromium.org/1901533002/diff/20001/net/base/network_stream...
File net/base/network_stream_throttler.h (right):

https://codereview.chromium.org/1901533002/diff/20001/net/base/network_stream...
net/base/network_stream_throttler.h:22: virtual ~Delegate() {}
On 2016/04/29 16:57:28, mmenke wrote:
> Make the destructor protected?  Doesn't matter much, but makes it clearer the
> throttler doesn't delete these.

Done.

https://codereview.chromium.org/1901533002/diff/20001/net/base/network_stream...
net/base/network_stream_throttler.h:35: bool throttled() { return throttled_; }
On 2016/04/29 16:57:28, mmenke wrote:
> const

Done.

https://codereview.chromium.org/1901533002/diff/20001/net/base/network_stream...
net/base/network_stream_throttler.h:39: void set_delegate(Delegate* delegate) {
delegate_ = delegate; }
On 2016/04/29 16:57:28, mmenke wrote:
> Can we make this a constructor argument instead?

Sure.  It doesn't look like I ever called this function anywhere anyway.  Ooops
:-{.

https://codereview.chromium.org/1901533002/diff/20001/net/base/network_stream...
net/base/network_stream_throttler.h:52: base::WeakPtr<NetworkStreamThrottler>
throttler);
On 2016/04/29 16:57:28, mmenke wrote:
> Is there any case where the Throttle can be destroyed before the
> NetworkStreamThrottler?  Generally, if transactions outlive any component of
the
> network stack, we're doomed, anyways.

I think you mean the other way around, and no, I don't think there is.  Though
that guarantee goes through many layers of ownership (as do all the guarantees
you're referring to).  I'll change the interface and put in some documentation
about that requirement.

https://codereview.chromium.org/1901533002/diff/20001/net/base/network_stream...
net/base/network_stream_throttler.h:54: void NotifyThrottleStateChanged(bool
new_throttle_state);
On 2016/04/29 16:57:28, mmenke wrote:
> I don't think we need to take a bool?  Seems like we'll only unthrottle
> requests.  If we want to start unthrottled (Which presumably we will), should
> make that a constructor parameter.

Hmmm.  This was planning for the long-term; I could imagine that the throttling
would go both ways, and eventually I'd like to do something when it does.  (E.g.
throttling IDLE connections when there are more than N connections of any type).
 But yeah, it probably makes sense to postpone that interface until it's
actually used; certainly HttpNetworkTransaction can't currently handle a
transition back to throttled.

I'm confused by your comment about a constructor parameter, though; to what
class?  Throttle's already got a parameter defining it's initial throttle state.
 (For this CL I want to start unthrottled and never change; in the next CL where
I make this class do something rather than just put in the plumbing, objects can
start in either state, depending on the throttler.

https://codereview.chromium.org/1901533002/diff/20001/net/base/network_stream...
net/base/network_stream_throttler.h:55: QueuePointer queue_pointer() { return
queue_pointer_; }
On 2016/04/29 16:57:28, mmenke wrote:
> const?

Done.

https://codereview.chromium.org/1901533002/diff/20001/net/http/http_network_t...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1901533002/diff/20001/net/http/http_network_t...
net/http/http_network_transaction.cc:802: priority_, request_->load_flags &
LOAD_IGNORE_LIMITS);
On 2016/04/29 16:57:28, mmenke wrote:
> The implicit conversion of "request_->load_flags & LOAD_IGNORE_LIMITS" to a
bool
> doesn't generate an error of some sort?

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.

https://codereview.chromium.org/1901533002/diff/20001/net/http/http_network_t...
net/http/http_network_transaction.cc:802: priority_, request_->load_flags &
LOAD_IGNORE_LIMITS);
On 2016/04/29 16:57:28, mmenke wrote:
> 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.  :(

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?

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

Responses, will do a pass today.

https://codereview.chromium.org/1901533002/diff/20001/net/base/network_stream...
File net/base/network_stream_throttler.h (right):

https://codereview.chromium.org/1901533002/diff/20001/net/base/network_stream...
net/base/network_stream_throttler.h:54: void NotifyThrottleStateChanged(bool
new_throttle_state);
On 2016/05/08 00:46:30, Randy Smith - Not in Fridays wrote:
> On 2016/04/29 16:57:28, mmenke wrote:
> > I don't think we need to take a bool?  Seems like we'll only unthrottle
> > requests.  If we want to start unthrottled (Which presumably we will),
should
> > make that a constructor parameter.
> 
> Hmmm.  This was planning for the long-term; I could imagine that the
throttling
> would go both ways, and eventually I'd like to do something when it does. 
(E.g.
> throttling IDLE connections when there are more than N connections of any
type).
>  But yeah, it probably makes sense to postpone that interface until it's
> actually used; certainly HttpNetworkTransaction can't currently handle a
> transition back to throttled.
> 
> I'm confused by your comment about a constructor parameter, though; to what
> class?  Throttle's already got a parameter defining it's initial throttle
state.
>  (For this CL I want to start unthrottled and never change; in the next CL
where
> I make this class do something rather than just put in the plumbing, objects
can
> start in either state, depending on the throttler.

I missed the constructor parameter when I made that comment.

https://codereview.chromium.org/1901533002/diff/20001/net/http/http_network_t...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1901533002/diff/20001/net/http/http_network_t...
net/http/http_network_transaction.cc:802: priority_, request_->load_flags &
LOAD_IGNORE_LIMITS);
On 2016/05/08 00:46:31, Randy Smith - Not in Fridays wrote:
> On 2016/04/29 16:57:28, mmenke wrote:
> > The implicit conversion of "request_->load_flags & LOAD_IGNORE_LIMITS" to a
> bool
> > doesn't generate an error of some sort?
> 
> 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.

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.

https://codereview.chromium.org/1901533002/diff/20001/net/http/http_network_t...
net/http/http_network_transaction.cc:802: priority_, request_->load_flags &
LOAD_IGNORE_LIMITS);
On 2016/05/08 00:46:30, Randy Smith - Not in Fridays wrote:
> On 2016/04/29 16:57:28, mmenke wrote:
> > 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.  :(
> 
> 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?

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:08:20]

For tests, I think it may make sense to make a virtual NetworkStreamThrottler,
and test with that.  I do think we want to make sure a throttled transaction can
be unthrottled before landing this, even if we currently never throttle
anything.  This would be more of an integration test, than just a
NetworkStreamThrottler test.

As long as we only throttle before the headers are received, I think we're fine
unthrottling synchronously - it's guaranteed that headers won't be passed
synchronously to URLRequest::Delegates when they're received, though it may be
worth commenting on that, both in the URLRequestJob method that does the
notifying, and in the the throttler code.

I can't see a case where unthrottling one request would result in deleting some
unthrottled request (Unthrottling a request would have to result in some
unthrottled request getting a read error / read completion, which does bubble up
synchronously, and then the unthrottled request would have to be deleted).  As
long as we don't keep throttled and unthrottle NetworkStreamThrottler pointers
in the same vector, this isn't a problem, anyways, but good to be aware of
potential issues there.

https://codereview.chromium.org/1901533002/diff/80001/net/base/network_stream...
File net/base/network_stream_throttler_unittest.cc (right):

https://codereview.chromium.org/1901533002/diff/80001/net/base/network_stream...
net/base/network_stream_throttler_unittest.cc:8: 
Remove blank line.

https://codereview.chromium.org/1901533002/diff/80001/net/base/network_stream...
net/base/network_stream_throttler_unittest.cc:11: namespace net {
We aren't consistent about it, but may want to put everything in an anonymous
namespace.

https://codereview.chromium.org/1901533002/diff/80001/net/base/network_stream...
net/base/network_stream_throttler_unittest.cc:18:
std::unique_ptr<NetworkStreamThrottler::Throttle> CreateThrottle(
include <memory>

https://codereview.chromium.org/1901533002/diff/80001/net/base/network_stream...
net/base/network_stream_throttler_unittest.cc:41: }
nit:  // namespace net

https://codereview.chromium.org/1901533002/diff/80001/net/http/http_network_t...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1901533002/diff/80001/net/http/http_network_t...
net/http/http_network_transaction.cc:378: LoadState
HttpNetworkTransaction::GetLoadState() const {
Hrm...Should we add a load state and user-visible text for this?  Ideally, it
would <mostly> only happen when we're waiting on some other resource for the
same page, and displaying the state of that other resource, further along the
load path, would take precedence, anyways.

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

https://codereview.chromium.org/1901533002/diff/20001/net/http/http_network_t...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1901533002/diff/20001/net/http/http_network_t...
net/http/http_network_transaction.cc:802: priority_, request_->load_flags &
LOAD_IGNORE_LIMITS);
On 2016/05/09 17:37:48, mmenke wrote:
> On 2016/05/08 00:46:30, Randy Smith - Not in Fridays wrote:
> > On 2016/04/29 16:57:28, mmenke wrote:
> > > 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.  :(
> > 
> > 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?
> 
> 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.

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]

https://codereview.chromium.org/1901533002/diff/20001/net/http/http_network_t...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1901533002/diff/20001/net/http/http_network_t...
net/http/http_network_transaction.cc:802: priority_, request_->load_flags &
LOAD_IGNORE_LIMITS);
On 2016/05/09 17:37:48, mmenke wrote:
> On 2016/05/08 00:46:30, Randy Smith - Not in Fridays wrote:
> > On 2016/04/29 16:57:28, mmenke wrote:
> > > 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.  :(
> > 
> > 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?
> 
> 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.

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]

Responding to what I consider the largest abstract issue on this CL first, since
resolving this will have a lot of implications.  I'll work on the other comments
in the background.  Let me know what you think.

https://codereview.chromium.org/1901533002/diff/20001/net/http/http_network_t...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1901533002/diff/20001/net/http/http_network_t...
net/http/http_network_transaction.cc:802: priority_, request_->load_flags &
LOAD_IGNORE_LIMITS);
On 2016/05/09 17:37:48, mmenke wrote:
> On 2016/05/08 00:46:30, Randy Smith - Not in Fridays wrote:
> > On 2016/04/29 16:57:28, mmenke wrote:
> > > 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.  :(
> > 
> > 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?
> 
> 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.

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?

https://codereview.chromium.org/1901533002/diff/20001/net/http/http_network_t...
net/http/http_network_transaction.cc:802: priority_, request_->load_flags &
LOAD_IGNORE_LIMITS);
On 2016/05/09 18:21:21, mmenke wrote:
> On 2016/05/09 17:37:48, mmenke wrote:
> > On 2016/05/08 00:46:30, Randy Smith - Not in Fridays wrote:
> > > On 2016/04/29 16:57:28, mmenke wrote:
> > > > 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.  :(
> > > 
> > > 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?
> > 
> > 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.
> 
> 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).

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]

https://codereview.chromium.org/1901533002/diff/20001/net/http/http_network_t...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1901533002/diff/20001/net/http/http_network_t...
net/http/http_network_transaction.cc:802: priority_, request_->load_flags &
LOAD_IGNORE_LIMITS);
On 2016/05/09 22:12:54, Randy Smith - Not in Fridays wrote:
> On 2016/05/09 18:21:21, mmenke wrote:
> > On 2016/05/09 17:37:48, mmenke wrote:
> > > On 2016/05/08 00:46:30, Randy Smith - Not in Fridays wrote:
> > > > On 2016/04/29 16:57:28, mmenke wrote:
> > > > > 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.  :(
> > > > 
> > > > 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?
> > > 
> > > 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.
> > 
> > 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).
> 
> 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. 

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.

> 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?

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).

[mmenke, 2016-05-11 18:14:08]

And sorry for the slow response - contrary to my email yesterday, I ended up
taking the entire day off.

[Randy Smith (Not in Mondays), 2016-05-12 20:17:52]

On 2016/05/11 18:12:38, mmenke wrote:
>
https://codereview.chromium.org/1901533002/diff/20001/net/http/http_network_t...
> File net/http/http_network_transaction.cc (right):
> 
>
https://codereview.chromium.org/1901533002/diff/20001/net/http/http_network_t...
> net/http/http_network_transaction.cc:802: priority_, request_->load_flags &
> LOAD_IGNORE_LIMITS);
> On 2016/05/09 22:12:54, Randy Smith - Not in Fridays wrote:
> > On 2016/05/09 18:21:21, mmenke wrote:
> > > On 2016/05/09 17:37:48, mmenke wrote:
> > > > On 2016/05/08 00:46:30, Randy Smith - Not in Fridays wrote:
> > > > > On 2016/04/29 16:57:28, mmenke wrote:
> > > > > > 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.  :(
> > > > > 
> > > > > 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?
> > > > 
> > > > 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.
> > > 
> > > 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).
> > 
> > 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. 
> 
> 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.
> 
> > 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?
> 
> 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).

Quick update on this thread for those following along at home: Matt and I talked
offline, and agreed that in the abstract and as a long-term goal we wanted to
allow the net/ embedder to specify and configure policy for this class, for this
CL we'd land it in net/ and hoist it up in some delegate fashion in a future CL
(specifically when I actually start configuring consumers to use the THROTTLED
priority; the embedder implementation should be in a place parallel to the
consumers that are setting that priority).

[Randy Smith (Not in Mondays), 2016-05-15 21:49:20]

Matt: I think this is ready for another round of review.  I think I've addressed
all your comments.  The test turned out to be easier than I thought as there was
already a test fixture to do what I needed to do, and well worth doing as I
found several problems in doing it.

One question: I couldn't figure out where in the URLRequestJob area I should put
the comment about relying on the lack of reentrancy across header notification;
I did the other comments but not that one.  Could you give me a pointer to where
you'd like that comment?

https://codereview.chromium.org/1901533002/diff/80001/net/base/network_stream...
File net/base/network_stream_throttler_unittest.cc (right):

https://codereview.chromium.org/1901533002/diff/80001/net/base/network_stream...
net/base/network_stream_throttler_unittest.cc:8: 
On 2016/05/09 18:08:20, mmenke wrote:
> Remove blank line.

Done.

https://codereview.chromium.org/1901533002/diff/80001/net/base/network_stream...
net/base/network_stream_throttler_unittest.cc:11: namespace net {
On 2016/05/09 18:08:20, mmenke wrote:
> We aren't consistent about it, but may want to put everything in an anonymous
> namespace.

Done.

https://codereview.chromium.org/1901533002/diff/80001/net/base/network_stream...
net/base/network_stream_throttler_unittest.cc:18:
std::unique_ptr<NetworkStreamThrottler::Throttle> CreateThrottle(
On 2016/05/09 18:08:20, mmenke wrote:
> include <memory>

Done.

https://codereview.chromium.org/1901533002/diff/80001/net/base/network_stream...
net/base/network_stream_throttler_unittest.cc:41: }
On 2016/05/09 18:08:20, mmenke wrote:
> nit:  // namespace net

Done.

https://codereview.chromium.org/1901533002/diff/80001/net/http/http_network_t...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1901533002/diff/80001/net/http/http_network_t...
net/http/http_network_transaction.cc:378: LoadState
HttpNetworkTransaction::GetLoadState() const {
On 2016/05/09 18:08:20, mmenke wrote:
> Hrm...Should we add a load state and user-visible text for this?  Ideally, it
> would <mostly> only happen when we're waiting on some other resource for the
> same page, and displaying the state of that other resource, further along the
> load path, would take precedence, anyways.

Done.

[mmenke, 2016-05-17 18:15:28]

https://codereview.chromium.org/1901533002/diff/160001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1901533002/diff/160001/chrome/app/generated_r...
chrome/app/generated_resources.grd:8728: +      <message
name="IDS_LOAD_STATE_THROTTLED">
Know the others don't, but should have a not to the translator here.

https://codereview.chromium.org/1901533002/diff/160001/net/base/load_states_l...
File net/base/load_states_list.h (right):

https://codereview.chromium.org/1901533002/diff/160001/net/base/load_states_l...
net/base/load_states_list.h:102: LOAD_STATE(THROTTLED, 16)
These are in the priority they're returned in, when we have multiple requests in
different phases.  THROTTLED should be just below IDLE.

https://codereview.chromium.org/1901533002/diff/160001/net/base/network_strea...
File net/base/network_stream_throttler.cc (right):

https://codereview.chromium.org/1901533002/diff/160001/net/base/network_strea...
net/base/network_stream_throttler.cc:9: class NetworkStreamThrottlerImpl :
public NetworkStreamThrottler {
This should be in an anonymous namespace.

https://codereview.chromium.org/1901533002/diff/160001/net/base/network_strea...
net/base/network_stream_throttler.cc:48: NetworkStreamThrottlerImpl* throttler_;
Thous this and delegate be const?  Positioned meaning the pointer doesn't change
- const after the *'s.

Main downside of that is I, at least, can never remember the const placement
rules.

https://codereview.chromium.org/1901533002/diff/160001/net/base/network_strea...
net/base/network_stream_throttler.cc:62: RequestPriority new_priority);
--virtual

https://codereview.chromium.org/1901533002/diff/160001/net/base/network_strea...
net/base/network_stream_throttler.cc:63: virtual void
OnStreamDestroyed(ThrottleImpl* throttle);
--virtual

https://codereview.chromium.org/1901533002/diff/160001/net/base/network_strea...
net/base/network_stream_throttler.cc:92: priority_(priority),
I don't think we need priority.  The PriorityQueue takes care of tracking it for
us.  If we need our own priority logic, then PriorityQueue is not the class we
should be using (For example, it can't do "only have one THROTTLED request going
whenever there's a request of priority HIGH or HIGHER"), then we shouldn't be
using it.

https://codereview.chromium.org/1901533002/diff/160001/net/base/network_strea...
File net/base/network_stream_throttler.h (right):

https://codereview.chromium.org/1901533002/diff/160001/net/base/network_strea...
net/base/network_stream_throttler.h:39: virtual void OnThrottleStateChanged() =
0;
Maybe add a promise that this won't be called during the destruction of the
(one) Throttle associated with the delegate?

https://codereview.chromium.org/1901533002/diff/160001/net/base/network_strea...
net/base/network_stream_throttler.h:50: // which will be signaled by
delegate->OnThrottleStateChanged().
And if it's not constructed in the throttled state, then it will never switch to
the thottled state.

https://codereview.chromium.org/1901533002/diff/160001/net/base/network_strea...
net/base/network_stream_throttler.h:52: // back to the throttled state.)
I'm not sure we want to do that.  QUIC, SPDY, and the socket pools all have
their own limits.  In particular, throttling 6 requests to the same socket pool
once they've already received sockets results in blockholing all future network
requests to a domain, until at least one of those requests is resumed (Or
canceled).

https://codereview.chromium.org/1901533002/diff/160001/net/base/network_strea...
net/base/network_stream_throttler.h:57: virtual bool throttled() const = 0;
virtual functions should not use this naming style.

https://codereview.chromium.org/1901533002/diff/160001/net/base/network_strea...
net/base/network_stream_throttler.h:78: static
std::unique_ptr<NetworkStreamThrottler> CreateThrottler();
I guess this is just to avoid having blah.h and blah_impl.h?

https://codereview.chromium.org/1901533002/diff/160001/net/base/network_strea...
File net/base/network_stream_throttler_unittest.cc (right):

https://codereview.chromium.org/1901533002/diff/160001/net/base/network_strea...
net/base/network_stream_throttler_unittest.cc:36: void OnThrottleStateChanged()
override {}
ADD_FAILURE();?

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
File net/http/http_network_session.h (right):

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
net/http/http_network_session.h:21: #include
"net/base/network_stream_throttler.h"
Move this to the CC file, and forward delcare it here.

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
net/http/http_network_session.h:297: std::unique_ptr<NetworkStreamThrottler>
network_stream_throttler_;
include <memory> (should already be included, but it isn't)

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
File net/http/http_network_session_peer.cc (right):

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
net/http/http_network_session_peer.cc:13: #include
"net/socket/transport_client_socket_pool.h"
Need to include network_stream_throttler header, since this class can call the
destructor.

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
File net/http/http_network_session_peer.h (right):

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
net/http/http_network_session_peer.h:22: class NET_EXPORT_PRIVATE
HttpNetworkSessionPeer {
Ahh, sorry, I had forgotten about this "wonderful" class.

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
net/http/http_network_transaction.cc:820: }
It's a bit unexpected to call this twice, once when we're not actually done
throttling it.  The more standard way to do this would be to move the check into
DoThrottle:

(In DoThrottle)
next_state_ = STATE_THROTTLE_COMPLETE;
if (throttle_->throttled())
  return ERR_IO_PENDING;
return OK;

Then this just sets next_state_ and returns OK, and maybe DCHECKs that
throttled() is false.

Also means there's one less state transition, which is nice.

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:15950: ~TestThrottle() override {
throttler_->OnThrottleDestroyed(this); }
nit:  Blank line after destructor.

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:15952: void
SetPriority(RequestPriority priority) override {}
Suggest a test where we both make sure this is called, and make sure things work
when this method unthrottles a request.

When we hook everything up, I think we'll want a request that does that at the
URLRequest layer as well, but just doing it here is fine for now.

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:15952: void
SetPriority(RequestPriority priority) override {}
Suggest a test where we destroy a HttpNetworkTransaction while still throttled.

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:15955: friend
TestNetworkStreamThrottler;
I don't think this class needs to make TestNetworkStreamThrottler a friend?  In
fact, can just make this a private class of the Throttler, and all its methods
public, no?  Not a big fan of friends unless strictly needed.

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:15963: DCHECK(throttled_);
Maybe an EXPECT instead?

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:15974:
~TestNetworkStreamThrottler() override {}
EXPECT outstanding_throttles_ is empty?

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:15994: void SetThrottling(bool
throttling) { throttling_ = throttling; }
set_initial_throttle_state / initial_throttle_state_?  Or maybe
throttle_new_requests.

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:15996: size_t
outstanding_requests() { return outstanding_throttles_.size(); }
const

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:15996: size_t
outstanding_requests() { return outstanding_throttles_.size(); }
I'd suggest calling it num_ountstanding_requests().  Generally foo_bar returns
foo_bar_,  It's a little weird for foo_bar to return the length of foo_bar_
instead.

Actually, this method isn't currently used.  Get rid of it?

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:16003: std::set<TestThrottle*>
outstanding_throttles_;
Think this is worth a comment (Namely that this includes throttled and
non-throttled throttles, not just still throttled throttles)

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:16003: std::set<TestThrottle*>
outstanding_throttles_;
include <set>

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:16026: session_deps_.net_log =
log.bound().net_log();
Think we're fine without using a BoundTestNetLog here.  Can just remove these
two lines.

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:16030:
EXPECT_TRUE(log.bound().IsCapturing());
Not needed.

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:16031: 
Suggest splitting this into two tests.  Could make a utility method to
initialize the network session appropriately, or make a test fixture for it. 
Could even use the mock in all tests, but probably not a great idea, since we
want to make sure the real one works, too.

[mmenke, 2016-05-17 18:21:02]

On 2016/05/15 21:49:20, Randy Smith - Not in Fridays wrote:
> Matt: I think this is ready for another round of review.  I think I've
addressed
> all your comments.  The test turned out to be easier than I thought as there
was
> already a test fixture to do what I needed to do, and well worth doing as I
> found several problems in doing it.
> 
> One question: I couldn't figure out where in the URLRequestJob area I should
put
> the comment about relying on the lack of reentrancy across header
notification;
> I did the other comments but not that one.  Could you give me a pointer to
where
> you'd like that comment?

Sorry, missed that.  It's both NotifyStartError and NotifyHeadersComplete that
must call into the delegate asynchronously...  And neither one does.  Ok, that's
actually a potential problem.

[mmenke, 2016-05-17 18:22:49]

On 2016/05/17 18:21:02, mmenke wrote:
> On 2016/05/15 21:49:20, Randy Smith - Not in Fridays wrote:
> > Matt: I think this is ready for another round of review.  I think I've
> addressed
> > all your comments.  The test turned out to be easier than I thought as there
> was
> > already a test fixture to do what I needed to do, and well worth doing as I
> > found several problems in doing it.
> > 
> > One question: I couldn't figure out where in the URLRequestJob area I should
> put
> > the comment about relying on the lack of reentrancy across header
> notification;
> > I did the other comments but not that one.  Could you give me a pointer to
> where
> > you'd like that comment?
> 
> Sorry, missed that.  It's both NotifyStartError and NotifyHeadersComplete that
> must call into the delegate asynchronously...  And neither one does.  Ok,
that's
> actually a potential problem.

So I think we will need to handle deletion while unthrottling - of any request
(The one being throttled, or any other request).  Unless there's a guarantee at
a lower level things will be async, and I don't think there is.

[Randy Smith (Not in Mondays), 2016-05-17 23:31:23]

On 2016/05/17 18:22:49, mmenke wrote:
> On 2016/05/17 18:21:02, mmenke wrote:
> > On 2016/05/15 21:49:20, Randy Smith - Not in Fridays wrote:
> > > Matt: I think this is ready for another round of review.  I think I've
> > addressed
> > > all your comments.  The test turned out to be easier than I thought as
there
> > was
> > > already a test fixture to do what I needed to do, and well worth doing as
I
> > > found several problems in doing it.
> > > 
> > > One question: I couldn't figure out where in the URLRequestJob area I
should
> > put
> > > the comment about relying on the lack of reentrancy across header
> > notification;
> > > I did the other comments but not that one.  Could you give me a pointer to
> > where
> > > you'd like that comment?
> > 
> > Sorry, missed that.  It's both NotifyStartError and NotifyHeadersComplete
that
> > must call into the delegate asynchronously...  And neither one does.  Ok,
> that's
> > actually a potential problem.
> 
> So I think we will need to handle deletion while unthrottling - of any request
> (The one being throttled, or any other request).  Unless there's a guarantee
at
> a lower level things will be async, and I don't think there is.

So I think the throttler is safe from that (in this implementation, the warning
comment in ThrottleImpl::NotifyUnthrottled which will guide future coding).  But
I'm not at all sure about the other classes, e.g. HttpNetworkTransaction,
URLRequestJob, and URLRequest.  Are you saying that we should go through the
entire stack above this point and make sure that SetPriority can handle
returning to a deleted |*this|?  Are you further saying we should do that in
this CL?  :-} :-|

I'm a bit confused that we haven't run into this before; I'd think that
PriorityQueues at lower levels of the stack could block and resume at least
somewhat based on SetPriority.  Have we been safe because we haven't imposed
per-priority limits?

[Randy Smith (Not in Mondays), 2016-05-17 23:32:15]

I'm afraid I didn't do the LoadState enum restructure, and I haven't handled the
problem you raised in the other thread (I responded there), but beyond that, I
think I've taken care of all the issues you raised.  PTAL?

https://codereview.chromium.org/1901533002/diff/160001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1901533002/diff/160001/chrome/app/generated_r...
chrome/app/generated_resources.grd:8728: +      <message
name="IDS_LOAD_STATE_THROTTLED">
On 2016/05/17 18:15:26, mmenke wrote:
> Know the others don't, but should have a not to the translator here.

I presume you mean a |desc="..."|?  Done.

https://codereview.chromium.org/1901533002/diff/160001/net/base/load_states_l...
File net/base/load_states_list.h (right):

https://codereview.chromium.org/1901533002/diff/160001/net/base/load_states_l...
net/base/load_states_list.h:102: LOAD_STATE(THROTTLED, 16)
On 2016/05/17 18:15:26, mmenke wrote:
> These are in the priority they're returned in, when we have multiple requests
in
> different phases.  THROTTLED should be just below IDLE.

Is LoadState never used for UMA?  Ok.  Done.

https://codereview.chromium.org/1901533002/diff/160001/net/base/network_strea...
File net/base/network_stream_throttler.cc (right):

https://codereview.chromium.org/1901533002/diff/160001/net/base/network_strea...
net/base/network_stream_throttler.cc:9: class NetworkStreamThrottlerImpl :
public NetworkStreamThrottler {
On 2016/05/17 18:15:26, mmenke wrote:
> This should be in an anonymous namespace.

Done.

https://codereview.chromium.org/1901533002/diff/160001/net/base/network_strea...
net/base/network_stream_throttler.cc:48: NetworkStreamThrottlerImpl* throttler_;
On 2016/05/17 18:15:26, mmenke wrote:
> Thous this and delegate be const?  Positioned meaning the pointer doesn't
change
> - const after the *'s.
> 
> Main downside of that is I, at least, can never remember the const placement
> rules.

That's partially because they're ambiguous; a const directly on either side of
the type name refers to the base, only after the * means the pointer itself.  (I
remembered they were ambiguous, but I mis-remembered *how* until I looked it up
:-}.)  Done.

https://codereview.chromium.org/1901533002/diff/160001/net/base/network_strea...
net/base/network_stream_throttler.cc:62: RequestPriority new_priority);
On 2016/05/17 18:15:27, mmenke wrote:
> --virtual

Done.

https://codereview.chromium.org/1901533002/diff/160001/net/base/network_strea...
net/base/network_stream_throttler.cc:63: virtual void
OnStreamDestroyed(ThrottleImpl* throttle);
On 2016/05/17 18:15:27, mmenke wrote:
> --virtual

Done.

https://codereview.chromium.org/1901533002/diff/160001/net/base/network_strea...
net/base/network_stream_throttler.cc:92: priority_(priority),
On 2016/05/17 18:15:26, mmenke wrote:
> I don't think we need priority.  The PriorityQueue takes care of tracking it
for
> us.  If we need our own priority logic, then PriorityQueue is not the class we
> should be using (For example, it can't do "only have one THROTTLED request
going
> whenever there's a request of priority HIGH or HIGHER"), then we shouldn't be
> using it.

Hmmm.  Done, but I'm a bit conflicted about the possible future situation and I
may want to come back to this if I'm in a position where I can use PriorityQueue
for most of what I want but not all.  I think I'm fine for the next CL (the
behavior I want is "Only N (probably == 2) THROTTLED requests active at a time",
which I think the priority queue *can* do) but I'm probably in trouble for the
one after that ("Only N (probably == 6 or 12) IDLE requests active at a time,
but if a request has been around for a Long Time (TBD) ignore it for these
purposes.")  But I'll worry about that then.

https://codereview.chromium.org/1901533002/diff/160001/net/base/network_strea...
File net/base/network_stream_throttler.h (right):

https://codereview.chromium.org/1901533002/diff/160001/net/base/network_strea...
net/base/network_stream_throttler.h:39: virtual void OnThrottleStateChanged() =
0;
On 2016/05/17 18:15:27, mmenke wrote:
> Maybe add a promise that this won't be called during the destruction of the
> (one) Throttle associated with the delegate?

Done.

https://codereview.chromium.org/1901533002/diff/160001/net/base/network_strea...
net/base/network_stream_throttler.h:50: // which will be signaled by
delegate->OnThrottleStateChanged().
On 2016/05/17 18:15:27, mmenke wrote:
> And if it's not constructed in the throttled state, then it will never switch
to
> the thottled state.

Done.

https://codereview.chromium.org/1901533002/diff/160001/net/base/network_strea...
net/base/network_stream_throttler.h:52: // back to the throttled state.)
On 2016/05/17 18:15:27, mmenke wrote:
> I'm not sure we want to do that.  QUIC, SPDY, and the socket pools all have
> their own limits.  In particular, throttling 6 requests to the same socket
pool
> once they've already received sockets results in blockholing all future
network
> requests to a domain, until at least one of those requests is resumed (Or
> canceled).

Fair enough.  I think we'll need something in that space eventually, but it'll
probably need to be subtler.  Or maybe this mechanism will take over all those
limits; dunno.  It's a ways in the future, either way.  TODO removed.

https://codereview.chromium.org/1901533002/diff/160001/net/base/network_strea...
net/base/network_stream_throttler.h:57: virtual bool throttled() const = 0;
On 2016/05/17 18:15:27, mmenke wrote:
> virtual functions should not use this naming style.

Done.

https://codereview.chromium.org/1901533002/diff/160001/net/base/network_strea...
net/base/network_stream_throttler.h:78: static
std::unique_ptr<NetworkStreamThrottler> CreateThrottler();
On 2016/05/17 18:15:27, mmenke wrote:
> I guess this is just to avoid having blah.h and blah_impl.h?

Yes :-}.  If you feel like I really should split it into a separate file, I
will, but it didn't seem worth it for a single function/an
interface/implementation split just for testing.

https://codereview.chromium.org/1901533002/diff/160001/net/base/network_strea...
File net/base/network_stream_throttler_unittest.cc (right):

https://codereview.chromium.org/1901533002/diff/160001/net/base/network_strea...
net/base/network_stream_throttler_unittest.cc:36: void OnThrottleStateChanged()
override {}
On 2016/05/17 18:15:27, mmenke wrote:
> ADD_FAILURE();?

Done.

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
File net/http/http_network_session.h (right):

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
net/http/http_network_session.h:21: #include
"net/base/network_stream_throttler.h"
On 2016/05/17 18:15:27, mmenke wrote:
> Move this to the CC file, and forward delcare it here.

Done.

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
net/http/http_network_session.h:297: std::unique_ptr<NetworkStreamThrottler>
network_stream_throttler_;
On 2016/05/17 18:15:27, mmenke wrote:
> include <memory> (should already be included, but it isn't)

Done.

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
File net/http/http_network_session_peer.cc (right):

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
net/http/http_network_session_peer.cc:13: #include
"net/socket/transport_client_socket_pool.h"
On 2016/05/17 18:15:27, mmenke wrote:
> Need to include network_stream_throttler header, since this class can call the
> destructor.

Done.

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
File net/http/http_network_session_peer.h (right):

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
net/http/http_network_session_peer.h:22: class NET_EXPORT_PRIVATE
HttpNetworkSessionPeer {
On 2016/05/17 18:15:27, mmenke wrote:
> Ahh, sorry, I had forgotten about this "wonderful" class.

Yeah :-J.  Using it on the basis of "a mechanism that's already in place is
better than a new one that needs to be shoehorned in".  And I could imagine much
worse options.  But the right way (adding HttpNetworkSessionImpl) still seems
like overkill.

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
net/http/http_network_transaction.cc:820: }
On 2016/05/17 18:15:27, mmenke wrote:
> It's a bit unexpected to call this twice, once when we're not actually done
> throttling it.  The more standard way to do this would be to move the check
into
> DoThrottle:
> 
> (In DoThrottle)
> next_state_ = STATE_THROTTLE_COMPLETE;
> if (throttle_->throttled())
>   return ERR_IO_PENDING;
> return OK;
> 
> Then this just sets next_state_ and returns OK, and maybe DCHECKs that
> throttled() is false.
> 
> Also means there's one less state transition, which is nice.

Done.

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:15950: ~TestThrottle() override {
throttler_->OnThrottleDestroyed(this); }
On 2016/05/17 18:15:27, mmenke wrote:
> nit:  Blank line after destructor.

Done.

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:15952: void
SetPriority(RequestPriority priority) override {}
On 2016/05/17 18:15:27, mmenke wrote:
> Suggest a test where we both make sure this is called, and make sure things
work
> when this method unthrottles a request.
> 
> When we hook everything up, I think we'll want a request that does that at the
> URLRequest layer as well, but just doing it here is fine for now.

Good ideas.  Done.

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:15952: void
SetPriority(RequestPriority priority) override {}
On 2016/05/17 18:15:28, mmenke wrote:
> Suggest a test where we destroy a HttpNetworkTransaction while still
throttled.

Done.

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:15955: friend
TestNetworkStreamThrottler;
On 2016/05/17 18:15:27, mmenke wrote:
> I don't think this class needs to make TestNetworkStreamThrottler a friend? 
In
> fact, can just make this a private class of the Throttler, and all its methods
> public, no?  Not a big fan of friends unless strictly needed.

Huh, good point.  Done.  Completely agreed that friends should be an option of
almost last resort (I did recommend them to Julia on a recent CL as it made the
interface much simpler).

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:15963: DCHECK(throttled_);
On 2016/05/17 18:15:28, mmenke wrote:
> Maybe an EXPECT instead?

Done, but note that it's not testing any behavior in production code, only in
this test fixture.

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:15974:
~TestNetworkStreamThrottler() override {}
On 2016/05/17 18:15:28, mmenke wrote:
> EXPECT outstanding_throttles_ is empty?

Done.

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:15994: void SetThrottling(bool
throttling) { throttling_ = throttling; }
On 2016/05/17 18:15:28, mmenke wrote:
> set_initial_throttle_state / initial_throttle_state_?  Or maybe
> throttle_new_requests.

Done.

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:15996: size_t
outstanding_requests() { return outstanding_throttles_.size(); }
On 2016/05/17 18:15:27, mmenke wrote:
> const

Done.

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:15996: size_t
outstanding_requests() { return outstanding_throttles_.size(); }
On 2016/05/17 18:15:27, mmenke wrote:
> I'd suggest calling it num_ountstanding_requests().  Generally foo_bar returns
> foo_bar_,  It's a little weird for foo_bar to return the length of foo_bar_
> instead.

Done. 

> Actually, this method isn't currently used.  Get rid of it?

Good point, but now used for one of the new tests.

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:16003: std::set<TestThrottle*>
outstanding_throttles_;
On 2016/05/17 18:15:28, mmenke wrote:
> include <set>

Done.

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:16003: std::set<TestThrottle*>
outstanding_throttles_;
On 2016/05/17 18:15:28, mmenke wrote:
> Think this is worth a comment (Namely that this includes throttled and
> non-throttled throttles, not just still throttled throttles)

Done.

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:16026: session_deps_.net_log =
log.bound().net_log();
On 2016/05/17 18:15:27, mmenke wrote:
> Think we're fine without using a BoundTestNetLog here.  Can just remove these
> two lines.

Done.

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:16030:
EXPECT_TRUE(log.bound().IsCapturing());
On 2016/05/17 18:15:28, mmenke wrote:
> Not needed.

Done.

https://codereview.chromium.org/1901533002/diff/160001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:16031: 
On 2016/05/17 18:15:28, mmenke wrote:
> Suggest splitting this into two tests.  Could make a utility method to
> initialize the network session appropriately, or make a test fixture for it. 
> Could even use the mock in all tests, but probably not a great idea, since we
> want to make sure the real one works, too.

Done.

[mmenke, 2016-05-17 23:57:43]

On 2016/05/17 23:31:23, Randy Smith - Not in Fridays wrote:
> On 2016/05/17 18:22:49, mmenke wrote:
> > On 2016/05/17 18:21:02, mmenke wrote:
> > > On 2016/05/15 21:49:20, Randy Smith - Not in Fridays wrote:
> > > > Matt: I think this is ready for another round of review.  I think I've
> > > addressed
> > > > all your comments.  The test turned out to be easier than I thought as
> there
> > > was
> > > > already a test fixture to do what I needed to do, and well worth doing
as
> I
> > > > found several problems in doing it.
> > > > 
> > > > One question: I couldn't figure out where in the URLRequestJob area I
> should
> > > put
> > > > the comment about relying on the lack of reentrancy across header
> > > notification;
> > > > I did the other comments but not that one.  Could you give me a pointer
to
> > > where
> > > > you'd like that comment?
> > > 
> > > Sorry, missed that.  It's both NotifyStartError and NotifyHeadersComplete
> that
> > > must call into the delegate asynchronously...  And neither one does.  Ok,
> > that's
> > > actually a potential problem.
> > 
> > So I think we will need to handle deletion while unthrottling - of any
request
> > (The one being throttled, or any other request).  Unless there's a guarantee
> at
> > a lower level things will be async, and I don't think there is.
> 
> So I think the throttler is safe from that (in this implementation, the
warning
> comment in ThrottleImpl::NotifyUnthrottled which will guide future coding). 
But
> I'm not at all sure about the other classes, e.g. HttpNetworkTransaction,
> URLRequestJob, and URLRequest.  Are you saying that we should go through the
> entire stack above this point and make sure that SetPriority can handle
> returning to a deleted |*this|?  Are you further saying we should do that in
> this CL?  :-} :-|

No, I'm saying that the Throttler will have to be able to handle
HttpNetworkTransactions deleted out from under it - i.e., it resumes one
transaction, and tries to pick the next, it has to handle the case where
unthrottling one transaction results in deleting another throttle.

> I'm a bit confused that we haven't run into this before; I'd think that
> PriorityQueues at lower levels of the stack could block and resume at least
> somewhat based on SetPriority.  Have we been safe because we haven't imposed
> per-priority limits?

We have run into problems before - at basically every layer where one thing
makes a call upstream, and then we call into other things that may have been
deleted as a result - SocketPools, SpdySessions, HostResolver, etc.  Not so much
around SetPriority (Which is relatively new), but around one request completing
causing another to be deleted, synchronously).

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

On 2016/05/17 23:57:43, mmenke wrote:
> On 2016/05/17 23:31:23, Randy Smith - Not in Fridays wrote:
> > On 2016/05/17 18:22:49, mmenke wrote:
> > > On 2016/05/17 18:21:02, mmenke wrote:
> > > > On 2016/05/15 21:49:20, Randy Smith - Not in Fridays wrote:
> > > > > Matt: I think this is ready for another round of review.  I think I've
> > > > addressed
> > > > > all your comments.  The test turned out to be easier than I thought as
> > there
> > > > was
> > > > > already a test fixture to do what I needed to do, and well worth doing
> as
> > I
> > > > > found several problems in doing it.
> > > > > 
> > > > > One question: I couldn't figure out where in the URLRequestJob area I
> > should
> > > > put
> > > > > the comment about relying on the lack of reentrancy across header
> > > > notification;
> > > > > I did the other comments but not that one.  Could you give me a
pointer
> to
> > > > where
> > > > > you'd like that comment?
> > > > 
> > > > Sorry, missed that.  It's both NotifyStartError and
NotifyHeadersComplete
> > that
> > > > must call into the delegate asynchronously...  And neither one does. 
Ok,
> > > that's
> > > > actually a potential problem.
> > > 
> > > So I think we will need to handle deletion while unthrottling - of any
> request
> > > (The one being throttled, or any other request).  Unless there's a
guarantee
> > at
> > > a lower level things will be async, and I don't think there is.
> > 
> > So I think the throttler is safe from that (in this implementation, the
> warning
> > comment in ThrottleImpl::NotifyUnthrottled which will guide future coding). 
> But
> > I'm not at all sure about the other classes, e.g. HttpNetworkTransaction,
> > URLRequestJob, and URLRequest.  Are you saying that we should go through the
> > entire stack above this point and make sure that SetPriority can handle
> > returning to a deleted |*this|?  Are you further saying we should do that in
> > this CL?  :-} :-|
> 
> No, I'm saying that the Throttler will have to be able to handle
> HttpNetworkTransactions deleted out from under it - i.e., it resumes one
> transaction, and tries to pick the next, it has to handle the case where
> unthrottling one transaction results in deleting another throttle.

Ok.  I don't think that'll be a problem--the throttler will simply have to avoid
counting on its internal data structures staying persistent across unthrottling
calls.  (It's not a problem now because it doesn't unthrottle :-}.)

> 
> > I'm a bit confused that we haven't run into this before; I'd think that
> > PriorityQueues at lower levels of the stack could block and resume at least
> > somewhat based on SetPriority.  Have we been safe because we haven't imposed
> > per-priority limits?
> 
> We have run into problems before - at basically every layer where one thing
> makes a call upstream, and then we call into other things that may have been
> deleted as a result - SocketPools, SpdySessions, HostResolver, etc.  Not so
much
> around SetPriority (Which is relatively new), but around one request
completing
> causing another to be deleted, synchronously).

[mmenke, 2016-05-18 17:18:36]

On 2016/05/18 12:22:54, Randy Smith - Not in Fridays wrote:
> On 2016/05/17 23:57:43, mmenke wrote:
> > On 2016/05/17 23:31:23, Randy Smith - Not in Fridays wrote:
> > > On 2016/05/17 18:22:49, mmenke wrote:
> > > > On 2016/05/17 18:21:02, mmenke wrote:
> > > > > On 2016/05/15 21:49:20, Randy Smith - Not in Fridays wrote:
> > > > > > Matt: I think this is ready for another round of review.  I think
I've
> > > > > addressed
> > > > > > all your comments.  The test turned out to be easier than I thought
as
> > > there
> > > > > was
> > > > > > already a test fixture to do what I needed to do, and well worth
doing
> > as
> > > I
> > > > > > found several problems in doing it.
> > > > > > 
> > > > > > One question: I couldn't figure out where in the URLRequestJob area
I
> > > should
> > > > > put
> > > > > > the comment about relying on the lack of reentrancy across header
> > > > > notification;
> > > > > > I did the other comments but not that one.  Could you give me a
> pointer
> > to
> > > > > where
> > > > > > you'd like that comment?
> > > > > 
> > > > > Sorry, missed that.  It's both NotifyStartError and
> NotifyHeadersComplete
> > > that
> > > > > must call into the delegate asynchronously...  And neither one does. 
> Ok,
> > > > that's
> > > > > actually a potential problem.
> > > > 
> > > > So I think we will need to handle deletion while unthrottling - of any
> > request
> > > > (The one being throttled, or any other request).  Unless there's a
> guarantee
> > > at
> > > > a lower level things will be async, and I don't think there is.
> > > 
> > > So I think the throttler is safe from that (in this implementation, the
> > warning
> > > comment in ThrottleImpl::NotifyUnthrottled which will guide future
coding). 
> > But
> > > I'm not at all sure about the other classes, e.g. HttpNetworkTransaction,
> > > URLRequestJob, and URLRequest.  Are you saying that we should go through
the
> > > entire stack above this point and make sure that SetPriority can handle
> > > returning to a deleted |*this|?  Are you further saying we should do that
in
> > > this CL?  :-} :-|
> > 
> > No, I'm saying that the Throttler will have to be able to handle
> > HttpNetworkTransactions deleted out from under it - i.e., it resumes one
> > transaction, and tries to pick the next, it has to handle the case where
> > unthrottling one transaction results in deleting another throttle.
> 
> Ok.  I don't think that'll be a problem--the throttler will simply have to
avoid
> counting on its internal data structures staying persistent across
unthrottling
> calls.  (It's not a problem now because it doesn't unthrottle :-}.)

That having been said SetPriority does need to be able to handle a deleted
|this|, if we unthrottle in response to the SetPriority call.  Could add a test
for that now, or later, but it is something we should probably have.

[mmenke, 2016-05-19 18:26:10]

Sorry for slowness, plan to get back to this one tomorrow.

[mmenke, 2016-05-23 15:00:17]

Actually did get to this last week, just didn't publish my comments.

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
net/http/http_network_transaction.cc:449: // at higher levels, this shouldn't
cause any problems.
This comment is actually incorrect (Yea, I thought it was at the time you added
it).

When we actually create the throttler, we'll need to think about what cases we
should handle.  For now, just remove the comment - sorry about sending you
around in circles.  Reentrancy makes my head hurt.

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
net/http/http_network_transaction.cc:811: return ERR_IO_PENDING;
Think we should have a NetLog event for throttling/unthrottling.  Ideally, we
wouldn't log them if we don't throttle.

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:125: last_priority_set_(IDLE) {}
optional:  May want a blank line between the constructor and destructor.  We
often don't have one, but I think the decision should be base on "Does the wall
of blank on green make my head spin less with a line break here", and I'd say
"yes", with a non-empty destructor.

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:134: std::unique_ptr<TestThrottle>
result(
Suggest calling this throttle or test_throttle.  "result" seems a bit weird for
less pure data-y types, to me (i.e. fine for ints, enums, strings).

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:157: int set_priority_count()
const { return set_priority_count_; }
starting non-setters with "set" seems potentially confusing.  Maybe
times_set_priority_called() / times_set_priority_called_?

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:159: void
set_priority_closure(const base::Closure& priority_closure) {
Maybe priority_closure -> priority_change_closure?  Given that there's a
SetPriority method, set_priority is ambiguous (Just consider set_priority_count,
which does something completely different)

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:200: }
nit:  No braces for one-line ifs.

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:208: base::Closure
priority_closure_;
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:356: // Note that the pointer
written into |*throttler| will only be valid
note that this doesn't currently match the argument name.

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:360: TestNetworkStreamThrottler**
throttler_p) {
_ptr is more common than _p, I believe.  I'd just call it throttler, though, and
call the unique_ptr owned_throttler instead - think clear argument names are
more important than clear stack variable names, at least when the stack variable
names have such limited scope.

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:361:
std::unique_ptr<HttpNetworkSession> result(
again, suggest result -> session.

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:16056:
TEST_P(HttpNetworkTransactionTest, Throttling_Empty) {
This isn't creating a null or empty throttle.  Should update comment and rename.
 Also, "_" is pretty uncommon in test names in this file.

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:16057: TestNetworkStreamThrottler*
throttler_p(nullptr);
Suggest "throttler", as above.  Not going to repeat this suggestion.

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:16065: request.load_flags = 0;
While you do need to set method and URL, load_flags defaults to 0, so don't need
to set it.

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:16079: };
We aren't very consistent about it, and it doesn't match the DataProvider
constructors, but I'd suggest writes before reads, since the first write is
chronologically first.

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:16206: EXPECT_EQ(LOW,
throttler_p->last_priority_set());
Suggest resuming and completing the request, just out of paranoia.

[mmenke, 2016-05-23 15:41:53]

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
net/http/http_network_transaction.cc:449: // at higher levels, this shouldn't
cause any problems.
On 2016/05/23 15:00:16, mmenke wrote:
> This comment is actually incorrect (Yea, I thought it was at the time you
added
> it).
> 
> When we actually create the throttler, we'll need to think about what cases we
> should handle.  For now, just remove the comment - sorry about sending you
> around in circles.  Reentrancy makes my head hurt.

Sorry, that should be "I thought it was correct at the time I asked you to add
it".  Figure you'd guess what I meant, but I don't want you to think I'm blaming
you for adding an inaccurate comment I asked you to add.

[Randy Smith (Not in Mondays), 2016-05-24 20:07:35]

On 2016/05/18 17:18:36, mmenke wrote:
> On 2016/05/18 12:22:54, Randy Smith - Not in Fridays wrote:
> > On 2016/05/17 23:57:43, mmenke wrote:
> > > On 2016/05/17 23:31:23, Randy Smith - Not in Fridays wrote:
> > > > On 2016/05/17 18:22:49, mmenke wrote:
> > > > > On 2016/05/17 18:21:02, mmenke wrote:
> > > > > > On 2016/05/15 21:49:20, Randy Smith - Not in Fridays wrote:
> > > > > > > Matt: I think this is ready for another round of review.  I think
> I've
> > > > > > addressed
> > > > > > > all your comments.  The test turned out to be easier than I
thought
> as
> > > > there
> > > > > > was
> > > > > > > already a test fixture to do what I needed to do, and well worth
> doing
> > > as
> > > > I
> > > > > > > found several problems in doing it.
> > > > > > > 
> > > > > > > One question: I couldn't figure out where in the URLRequestJob
area
> I
> > > > should
> > > > > > put
> > > > > > > the comment about relying on the lack of reentrancy across header
> > > > > > notification;
> > > > > > > I did the other comments but not that one.  Could you give me a
> > pointer
> > > to
> > > > > > where
> > > > > > > you'd like that comment?
> > > > > > 
> > > > > > Sorry, missed that.  It's both NotifyStartError and
> > NotifyHeadersComplete
> > > > that
> > > > > > must call into the delegate asynchronously...  And neither one does.

> > Ok,
> > > > > that's
> > > > > > actually a potential problem.
> > > > > 
> > > > > So I think we will need to handle deletion while unthrottling - of any
> > > request
> > > > > (The one being throttled, or any other request).  Unless there's a
> > guarantee
> > > > at
> > > > > a lower level things will be async, and I don't think there is.
> > > > 
> > > > So I think the throttler is safe from that (in this implementation, the
> > > warning
> > > > comment in ThrottleImpl::NotifyUnthrottled which will guide future
> coding). 
> > > But
> > > > I'm not at all sure about the other classes, e.g.
HttpNetworkTransaction,
> > > > URLRequestJob, and URLRequest.  Are you saying that we should go through
> the
> > > > entire stack above this point and make sure that SetPriority can handle
> > > > returning to a deleted |*this|?  Are you further saying we should do
that
> in
> > > > this CL?  :-} :-|
> > > 
> > > No, I'm saying that the Throttler will have to be able to handle
> > > HttpNetworkTransactions deleted out from under it - i.e., it resumes one
> > > transaction, and tries to pick the next, it has to handle the case where
> > > unthrottling one transaction results in deleting another throttle.
> > 
> > Ok.  I don't think that'll be a problem--the throttler will simply have to
> avoid
> > counting on its internal data structures staying persistent across
> unthrottling
> > calls.  (It's not a problem now because it doesn't unthrottle :-}.)
> 
> That having been said SetPriority does need to be able to handle a deleted
> |this|, if we unthrottle in response to the SetPriority call.  Could add a
test
> for that now, or later, but it is something we should probably have.

If this is true for the throttler, it's true for all levels above the throttler
that call SetPriority(), right?  Are those levels currently careful about
handling a deleted this?  If not, should I look at modifying them, or file a
bug?

[Randy Smith (Not in Mondays), 2016-05-24 20:08:08]

On 2016/05/23 15:41:53, mmenke wrote:
>
https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
> File net/http/http_network_transaction.cc (right):
> 
>
https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
> net/http/http_network_transaction.cc:449: // at higher levels, this shouldn't
> cause any problems.
> On 2016/05/23 15:00:16, mmenke wrote:
> > This comment is actually incorrect (Yea, I thought it was at the time you
> added
> > it).
> > 
> > When we actually create the throttler, we'll need to think about what cases
we
> > should handle.  For now, just remove the comment - sorry about sending you
> > around in circles.  Reentrancy makes my head hurt.
> 
> Sorry, that should be "I thought it was correct at the time I asked you to add
> it".  Figure you'd guess what I meant, but I don't want you to think I'm
blaming
> you for adding an inaccurate comment I asked you to add.

No worries; that what I figured you meant.

[mmenke, 2016-05-24 20:11:46]

On 2016/05/24 20:07:35, Randy Smith - Not in Fridays wrote:
> On 2016/05/18 17:18:36, mmenke wrote:
> > On 2016/05/18 12:22:54, Randy Smith - Not in Fridays wrote:
> > That having been said SetPriority does need to be able to handle a deleted
> > |this|, if we unthrottle in response to the SetPriority call.  Could add a
> test
> > for that now, or later, but it is something we should probably have.
> 
> If this is true for the throttler, it's true for all levels above the
throttler
> that call SetPriority(), right?  Are those levels currently careful about
> handling a deleted this?  If not, should I look at modifying them, or file a
> bug?

Higher layers trivially handle it, by doing nothing other than calling
SetPriority (ResourceScheduler itself handles it by starting requests
asynchronously).  The reason it's an issue here is that SetPriority needs to do
a lot more here.

[Randy Smith (Not in Mondays), 2016-05-24 21:38:50]

Matt: I believe I've addressed all your comments.  Also note I have two
questions for you in the form of comments in the latest PS.  PTAL?

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
net/http/http_network_transaction.cc:449: // at higher levels, this shouldn't
cause any problems.
On 2016/05/23 15:00:16, mmenke wrote:
> This comment is actually incorrect (Yea, I thought it was at the time you
added
> it).
> 
> When we actually create the throttler, we'll need to think about what cases we
> should handle.  For now, just remove the comment - sorry about sending you
> around in circles.  Reentrancy makes my head hurt.

Understood :-}.  Comment removed.

You say "When we create the throttler", which I'm assuming means "When we create
a throttler that actually results in throttling requests", since this CL does
indeed create the throttler.  Let me know if I'm misinterpreting.

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
net/http/http_network_transaction.cc:449: // at higher levels, this shouldn't
cause any problems.
On 2016/05/23 15:41:53, mmenke wrote:
> On 2016/05/23 15:00:16, mmenke wrote:
> > This comment is actually incorrect (Yea, I thought it was at the time you
> added
> > it).
> > 
> > When we actually create the throttler, we'll need to think about what cases
we
> > should handle.  For now, just remove the comment - sorry about sending you
> > around in circles.  Reentrancy makes my head hurt.
> 
> Sorry, that should be "I thought it was correct at the time I asked you to add
> it".  Figure you'd guess what I meant, but I don't want you to think I'm
blaming
> you for adding an inaccurate comment I asked you to add.

Acknowledged.

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
net/http/http_network_transaction.cc:811: return ERR_IO_PENDING;
On 2016/05/23 15:00:16, mmenke wrote:
> Think we should have a NetLog event for throttling/unthrottling.  Ideally, we
> wouldn't log them if we don't throttle.

Thanks for catching this.  Done.  I've also added logging for SetPriority(); let
me know if you think that doesn't make sense.

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:125: last_priority_set_(IDLE) {}
On 2016/05/23 15:00:16, mmenke wrote:
> optional:  May want a blank line between the constructor and destructor.  We
> often don't have one, but I think the decision should be base on "Does the
wall
> of blank on green make my head spin less with a line break here", and I'd say
> "yes", with a non-empty destructor.

Done.

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:134: std::unique_ptr<TestThrottle>
result(
On 2016/05/23 15:00:16, mmenke wrote:
> Suggest calling this throttle or test_throttle.  "result" seems a bit weird
for
> less pure data-y types, to me (i.e. fine for ints, enums, strings).

Done.

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:157: int set_priority_count()
const { return set_priority_count_; }
On 2016/05/23 15:00:16, mmenke wrote:
> starting non-setters with "set" seems potentially confusing.  Maybe
> times_set_priority_called() / times_set_priority_called_?

I went with "num_set_priority_calls" to be parallel to the outstanding requests
getter above; let me know if you'd rather a different name.

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:159: void
set_priority_closure(const base::Closure& priority_closure) {
On 2016/05/23 15:00:16, mmenke wrote:
> Maybe priority_closure -> priority_change_closure?  Given that there's a
> SetPriority method, set_priority is ambiguous (Just consider
set_priority_count,
> which does something completely different)

Done.

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:200: }
On 2016/05/23 15:00:16, mmenke wrote:
> nit:  No braces for one-line ifs.

Done.

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:208: base::Closure
priority_closure_;
On 2016/05/23 15:00:16, mmenke wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:356: // Note that the pointer
written into |*throttler| will only be valid
On 2016/05/23 15:00:16, mmenke wrote:
> note that this doesn't currently match the argument name.

Done.

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:360: TestNetworkStreamThrottler**
throttler_p) {
On 2016/05/23 15:00:16, mmenke wrote:
> _ptr is more common than _p, I believe.  I'd just call it throttler, though,
and
> call the unique_ptr owned_throttler instead - think clear argument names are
> more important than clear stack variable names, at least when the stack
variable
> names have such limited scope.

Yeah, that makes sense as a decision algorithm.  Done.

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:361:
std::unique_ptr<HttpNetworkSession> result(
On 2016/05/23 15:00:16, mmenke wrote:
> again, suggest result -> session.

Done.

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:16056:
TEST_P(HttpNetworkTransactionTest, Throttling_Empty) {
On 2016/05/23 15:00:16, mmenke wrote:
> This isn't creating a null or empty throttle.  Should update comment and
rename.
>  Also, "_" is pretty uncommon in test names in this file.

Changed to ThrottlingUnthrottled and updated comment.

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:16057: TestNetworkStreamThrottler*
throttler_p(nullptr);
On 2016/05/23 15:00:16, mmenke wrote:
> Suggest "throttler", as above.  Not going to repeat this suggestion.

Changed everywhere with query/replace.

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:16065: request.load_flags = 0;
On 2016/05/23 15:00:16, mmenke wrote:
> While you do need to set method and URL, load_flags defaults to 0, so don't
need
> to set it.

So I'm uncomfortable with adding new code that's similar but not identical to
code already existing elsewhere in the file for the same purpose.  Would you
like me to update this usage throughout the file?  I'm willing but wanted to
confirm before I went hog wild.

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:16079: };
On 2016/05/23 15:00:16, mmenke wrote:
> We aren't very consistent about it, and it doesn't match the DataProvider
> constructors, but I'd suggest writes before reads, since the first write is
> chronologically first.

Done.

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:16206: EXPECT_EQ(LOW,
throttler_p->last_priority_set());
On 2016/05/23 15:00:16, mmenke wrote:
> Suggest resuming and completing the request, just out of paranoia.

Done.

https://codereview.chromium.org/1901533002/diff/200001/net/http/http_network_...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1901533002/diff/200001/net/http/http_network_...
net/http/http_network_transaction.cc:440: const std::string
priority_string(RequestPriorityToString(priority));
I don't intend the above code to be what lands (why do a std::string
construction when the netlog isn't active?) but wanted to check in with you
about alternative strategies before I implemented them.  What I *want* to do is
to create a variation on NetLog::StringCallback that takes a char*, initialize a
char* variable with the results of RequestPriorityToString, and pass that to
StringCallback, as I could imagine that being useful in other places similar to
this one.  But I wanted to check with you as to whether there were gotchas to
that approach, or whether you'd just prefer me to create a new callback for this
particular event.

https://codereview.chromium.org/1901533002/diff/200001/net/http/http_network_...
net/http/http_network_transaction.cc:448: stream_->SetPriority(priority);
I find myself worried about the future, if we ever implement throttling at other
layers of the stack and one of the above results in unthrottling, synchronous
completion, and callback, like the call below might.  I presume it's a worry for
the future, but if you have any thoughts on the topic, I'm curious about them.

[mmenke, 2016-05-24 21:52:50]

quick responses.

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:16065: request.load_flags = 0;
On 2016/05/24 21:38:49, Randy Smith - Not in Fridays wrote:
> On 2016/05/23 15:00:16, mmenke wrote:
> > While you do need to set method and URL, load_flags defaults to 0, so don't
> need
> > to set it.
> 
> So I'm uncomfortable with adding new code that's similar but not identical to
> code already existing elsewhere in the file for the same purpose.  Would you
> like me to update this usage throughout the file?  I'm willing but wanted to
> confirm before I went hog wild.

Valid point.  I'd say just remove it everywhere, but maybe only after review? 
This is a 16000 line file, and about 117 of the 200+ files here have this
unnecessary line - Reduces the file size by less than 1%, but it all adds up,
and it's not going to happen if we don't do it here, even if it is unrelated to
this CL.

https://codereview.chromium.org/1901533002/diff/200001/net/http/http_network_...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1901533002/diff/200001/net/http/http_network_...
net/http/http_network_transaction.cc:440: const std::string
priority_string(RequestPriorityToString(priority));
On 2016/05/24 21:38:50, Randy Smith - Not in Fridays wrote:
> I don't intend the above code to be what lands (why do a std::string
> construction when the netlog isn't active?) but wanted to check in with you
> about alternative strategies before I implemented them.  What I *want* to do
is
> to create a variation on NetLog::StringCallback that takes a char*, initialize
a
> char* variable with the results of RequestPriorityToString, and pass that to
> StringCallback, as I could imagine that being useful in other places similar
to
> this one.  But I wanted to check with you as to whether there were gotchas to
> that approach, or whether you'd just prefer me to create a new callback for
this
> particular event.

I think you mean takes a RequestPriority, not a char*?  I'm fine with that.

https://codereview.chromium.org/1901533002/diff/200001/net/http/http_network_...
net/http/http_network_transaction.cc:448: stream_->SetPriority(priority);
On 2016/05/24 21:38:50, Randy Smith - Not in Fridays wrote:
> I find myself worried about the future, if we ever implement throttling at
other
> layers of the stack and one of the above results in unthrottling, synchronous
> completion, and callback, like the call below might.  I presume it's a worry
for
> the future, but if you have any thoughts on the topic, I'm curious about them.

Very good point.  I think only one of these should ever have an effect, so we're
safe early returning at each branch....If we have a stream_, we're not waiting
on a stream request.  If we have a stream request, we're not waiting on a
throttle, etc...

Think this requires some more thought.

[mmenke, 2016-05-24 22:59:52]

https://codereview.chromium.org/1901533002/diff/200001/net/http/http_network_...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1901533002/diff/200001/net/http/http_network_...
net/http/http_network_transaction.cc:440: const std::string
priority_string(RequestPriorityToString(priority));
On 2016/05/24 21:52:50, mmenke wrote:
> On 2016/05/24 21:38:50, Randy Smith - Not in Fridays wrote:
> > I don't intend the above code to be what lands (why do a std::string
> > construction when the netlog isn't active?) but wanted to check in with you
> > about alternative strategies before I implemented them.  What I *want* to do
> is
> > to create a variation on NetLog::StringCallback that takes a char*,
initialize
> a
> > char* variable with the results of RequestPriorityToString, and pass that to
> > StringCallback, as I could imagine that being useful in other places similar
> to
> > this one.  But I wanted to check with you as to whether there were gotchas
to
> > that approach, or whether you'd just prefer me to create a new callback for
> this
> > particular event.
> 
> I think you mean takes a RequestPriority, not a char*?  I'm fine with that.

Actually, you can just use an int called "priority", and net-internals will
convert it to a string, magically.

[Randy Smith (Not in Mondays), 2016-05-24 23:20:30]

Ok, incorporated your quick responses; ready for your next review.

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:16065: request.load_flags = 0;
On 2016/05/24 21:52:50, mmenke wrote:
> On 2016/05/24 21:38:49, Randy Smith - Not in Fridays wrote:
> > On 2016/05/23 15:00:16, mmenke wrote:
> > > While you do need to set method and URL, load_flags defaults to 0, so
don't
> > need
> > > to set it.
> > 
> > So I'm uncomfortable with adding new code that's similar but not identical
to
> > code already existing elsewhere in the file for the same purpose.  Would you
> > like me to update this usage throughout the file?  I'm willing but wanted to
> > confirm before I went hog wild.
> 
> Valid point.  I'd say just remove it everywhere, but maybe only after review? 
> This is a 16000 line file, and about 117 of the 200+ files here have this
> unnecessary line - Reduces the file size by less than 1%, but it all adds up,
> and it's not going to happen if we don't do it here, even if it is unrelated
to
> this CL.

Will do (i.e. note to self made to remove all request.load_flags = 0; in this
file after I have your stamp).

https://codereview.chromium.org/1901533002/diff/200001/net/http/http_network_...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1901533002/diff/200001/net/http/http_network_...
net/http/http_network_transaction.cc:440: const std::string
priority_string(RequestPriorityToString(priority));
On 2016/05/24 22:59:52, mmenke wrote:
> On 2016/05/24 21:52:50, mmenke wrote:
> > On 2016/05/24 21:38:50, Randy Smith - Not in Fridays wrote:
> > > I don't intend the above code to be what lands (why do a std::string
> > > construction when the netlog isn't active?) but wanted to check in with
you
> > > about alternative strategies before I implemented them.  What I *want* to
do
> > is
> > > to create a variation on NetLog::StringCallback that takes a char*,
> initialize
> > a
> > > char* variable with the results of RequestPriorityToString, and pass that
to
> > > StringCallback, as I could imagine that being useful in other places
similar
> > to
> > > this one.  But I wanted to check with you as to whether there were gotchas
> to
> > > that approach, or whether you'd just prefer me to create a new callback
for
> > this
> > > particular event.
> > 
> > I think you mean takes a RequestPriority, not a char*?  I'm fine with that.
> 
> Actually, you can just use an int called "priority", and net-internals will
> convert it to a string, magically.

Ok, I'll take your word for it, but FWIW, I looked and couldn't find the code
that would do that.  Done.

https://codereview.chromium.org/1901533002/diff/200001/net/http/http_network_...
net/http/http_network_transaction.cc:448: stream_->SetPriority(priority);
On 2016/05/24 21:52:50, mmenke wrote:
> On 2016/05/24 21:38:50, Randy Smith - Not in Fridays wrote:
> > I find myself worried about the future, if we ever implement throttling at
> other
> > layers of the stack and one of the above results in unthrottling,
synchronous
> > completion, and callback, like the call below might.  I presume it's a worry
> for
> > the future, but if you have any thoughts on the topic, I'm curious about
them.
> 
> Very good point.  I think only one of these should ever have an effect, so
we're
> safe early returning at each branch....If we have a stream_, we're not waiting
> on a stream request.  If we have a stream request, we're not waiting on a
> throttle, etc...
> 
> Think this requires some more thought.

*nod*  I'll put a comment here noting potential future problems and not worry
about it further for this CL.

[mmenke, 2016-05-25 15:32:15]

Mind fixing the build issue, and pinging me?  Think we're on the final pass.

https://codereview.chromium.org/1901533002/diff/200001/net/http/http_network_...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1901533002/diff/200001/net/http/http_network_...
net/http/http_network_transaction.cc:440: const std::string
priority_string(RequestPriorityToString(priority));
On 2016/05/24 23:20:30, Randy Smith - Not in Fridays wrote:
> On 2016/05/24 22:59:52, mmenke wrote:
> > On 2016/05/24 21:52:50, mmenke wrote:
> > > On 2016/05/24 21:38:50, Randy Smith - Not in Fridays wrote:
> > > > I don't intend the above code to be what lands (why do a std::string
> > > > construction when the netlog isn't active?) but wanted to check in with
> you
> > > > about alternative strategies before I implemented them.  What I *want*
to
> do
> > > is
> > > > to create a variation on NetLog::StringCallback that takes a char*,
> > initialize
> > > a
> > > > char* variable with the results of RequestPriorityToString, and pass
that
> to
> > > > StringCallback, as I could imagine that being useful in other places
> similar
> > > to
> > > > this one.  But I wanted to check with you as to whether there were
gotchas
> > to
> > > > that approach, or whether you'd just prefer me to create a new callback
> for
> > > this
> > > > particular event.
> > > 
> > > I think you mean takes a RequestPriority, not a char*?  I'm fine with
that.
> > 
> > Actually, you can just use an int called "priority", and net-internals will
> > convert it to a string, magically.
> 
> Ok, I'll take your word for it, but FWIW, I looked and couldn't find the code
> that would do that.  Done.

Oops, looks like you're right about this!  It's never logged along anywhere, so
we just use RequestPriorityToString.

That having been said, it seems like this should really be logged in
URLRequest::SetPriority...Which...Already logs priority changes.  So I guess we
don't need this.

[mmenke, 2016-05-25 15:33:59]

On 2016/05/25 15:32:15, mmenke wrote:
> Mind fixing the build issue, and pinging me?  Think we're on the final pass.
> 
>
https://codereview.chromium.org/1901533002/diff/200001/net/http/http_network_...
> File net/http/http_network_transaction.cc (right):
> 
>
https://codereview.chromium.org/1901533002/diff/200001/net/http/http_network_...
> net/http/http_network_transaction.cc:440: const std::string
> priority_string(RequestPriorityToString(priority));
> On 2016/05/24 23:20:30, Randy Smith - Not in Fridays wrote:
> > On 2016/05/24 22:59:52, mmenke wrote:
> > > On 2016/05/24 21:52:50, mmenke wrote:
> > > > On 2016/05/24 21:38:50, Randy Smith - Not in Fridays wrote:
> > > > > I don't intend the above code to be what lands (why do a std::string
> > > > > construction when the netlog isn't active?) but wanted to check in
with
> > you
> > > > > about alternative strategies before I implemented them.  What I *want*
> to
> > do
> > > > is
> > > > > to create a variation on NetLog::StringCallback that takes a char*,
> > > initialize
> > > > a
> > > > > char* variable with the results of RequestPriorityToString, and pass
> that
> > to
> > > > > StringCallback, as I could imagine that being useful in other places
> > similar
> > > > to
> > > > > this one.  But I wanted to check with you as to whether there were
> gotchas
> > > to
> > > > > that approach, or whether you'd just prefer me to create a new
callback
> > for
> > > > this
> > > > > particular event.
> > > > 
> > > > I think you mean takes a RequestPriority, not a char*?  I'm fine with
> that.
> > > 
> > > Actually, you can just use an int called "priority", and net-internals
will
> > > convert it to a string, magically.
> > 
> > Ok, I'll take your word for it, but FWIW, I looked and couldn't find the
code
> > that would do that.  Done.
> 
> Oops, looks like you're right about this!  It's never logged along anywhere,
so
> we just use RequestPriorityToString.
> 
> That having been said, it seems like this should really be logged in
> URLRequest::SetPriority...Which...Already logs priority changes.  So I guess
we
> don't need this.

And it is logged alone in URLRequest::SetPriority, which does unnecessarily
create an std::string.  Seems like we should avoid doing that, but tangential to
this CL.

[mmenke, 2016-05-25 15:34:53]

On 2016/05/25 15:33:59, mmenke wrote:
> On 2016/05/25 15:32:15, mmenke wrote:
> > Mind fixing the build issue, and pinging me?  Think we're on the final pass.
> > 
> >
>
https://codereview.chromium.org/1901533002/diff/200001/net/http/http_network_...
> > File net/http/http_network_transaction.cc (right):
> > 
> >
>
https://codereview.chromium.org/1901533002/diff/200001/net/http/http_network_...
> > net/http/http_network_transaction.cc:440: const std::string
> > priority_string(RequestPriorityToString(priority));
> > On 2016/05/24 23:20:30, Randy Smith - Not in Fridays wrote:
> > > On 2016/05/24 22:59:52, mmenke wrote:
> > > > On 2016/05/24 21:52:50, mmenke wrote:
> > > > > On 2016/05/24 21:38:50, Randy Smith - Not in Fridays wrote:
> > > > > > I don't intend the above code to be what lands (why do a std::string
> > > > > > construction when the netlog isn't active?) but wanted to check in
> with
> > > you
> > > > > > about alternative strategies before I implemented them.  What I
*want*
> > to
> > > do
> > > > > is
> > > > > > to create a variation on NetLog::StringCallback that takes a char*,
> > > > initialize
> > > > > a
> > > > > > char* variable with the results of RequestPriorityToString, and pass
> > that
> > > to
> > > > > > StringCallback, as I could imagine that being useful in other places
> > > similar
> > > > > to
> > > > > > this one.  But I wanted to check with you as to whether there were
> > gotchas
> > > > to
> > > > > > that approach, or whether you'd just prefer me to create a new
> callback
> > > for
> > > > > this
> > > > > > particular event.
> > > > > 
> > > > > I think you mean takes a RequestPriority, not a char*?  I'm fine with
> > that.
> > > > 
> > > > Actually, you can just use an int called "priority", and net-internals
> will
> > > > convert it to a string, magically.
> > > 
> > > Ok, I'll take your word for it, but FWIW, I looked and couldn't find the
> code
> > > that would do that.  Done.
> > 
> > Oops, looks like you're right about this!  It's never logged along anywhere,
> so
> > we just use RequestPriorityToString.
> > 
> > That having been said, it seems like this should really be logged in
> > URLRequest::SetPriority...Which...Already logs priority changes.  So I guess
> we
> > don't need this.
> 
> And it is logged alone in URLRequest::SetPriority, which does unnecessarily
> create an std::string.  Seems like we should avoid doing that, but tangential
to
> this CL.

Oh, uses IntCallback, so logged as an int...*anyways*, beyond the scope of this
CL.  Patches welcome!

[Randy Smith (Not in Mondays), 2016-05-25 16:57:48]

There's enough green from the try bots for me to think it's worthwhile tossing
this back to you now :-}.  I suspect the SetPriority glitch in URLRequest was my
fault, so I'll fix it (but not in this CL).  Let me know what you think.

https://codereview.chromium.org/1901533002/diff/200001/net/http/http_network_...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1901533002/diff/200001/net/http/http_network_...
net/http/http_network_transaction.cc:440: const std::string
priority_string(RequestPriorityToString(priority));
On 2016/05/25 15:32:15, mmenke wrote:
> On 2016/05/24 23:20:30, Randy Smith - Not in Fridays wrote:
> > On 2016/05/24 22:59:52, mmenke wrote:
> > > On 2016/05/24 21:52:50, mmenke wrote:
> > > > On 2016/05/24 21:38:50, Randy Smith - Not in Fridays wrote:
> > > > > I don't intend the above code to be what lands (why do a std::string
> > > > > construction when the netlog isn't active?) but wanted to check in
with
> > you
> > > > > about alternative strategies before I implemented them.  What I *want*
> to
> > do
> > > > is
> > > > > to create a variation on NetLog::StringCallback that takes a char*,
> > > initialize
> > > > a
> > > > > char* variable with the results of RequestPriorityToString, and pass
> that
> > to
> > > > > StringCallback, as I could imagine that being useful in other places
> > similar
> > > > to
> > > > > this one.  But I wanted to check with you as to whether there were
> gotchas
> > > to
> > > > > that approach, or whether you'd just prefer me to create a new
callback
> > for
> > > > this
> > > > > particular event.
> > > > 
> > > > I think you mean takes a RequestPriority, not a char*?  I'm fine with
> that.
> > > 
> > > Actually, you can just use an int called "priority", and net-internals
will
> > > convert it to a string, magically.
> > 
> > Ok, I'll take your word for it, but FWIW, I looked and couldn't find the
code
> > that would do that.  Done.
> 
> Oops, looks like you're right about this!  It's never logged along anywhere,
so
> we just use RequestPriorityToString.
> 
> That having been said, it seems like this should really be logged in
> URLRequest::SetPriority...Which...Already logs priority changes.  So I guess
we
> don't need this.

I'm amused by how resistant I feel removing it, but yes, there's never a
many->one mapping from URLRequests to HttpNetworkTransactions, so it's probably
fine to not log it here.  Removed.

[mmenke, 2016-05-25 18:23:52]

On 2016/05/25 16:57:48, Randy Smith - Not in Fridays wrote:
> There's enough green from the try bots for me to think it's worthwhile tossing
> this back to you now :-}.  I suspect the SetPriority glitch in URLRequest was
my
> fault, so I'll fix it (but not in this CL).  Let me know what you think.
> 
>
https://codereview.chromium.org/1901533002/diff/200001/net/http/http_network_...
> File net/http/http_network_transaction.cc (right):
> 
>
https://codereview.chromium.org/1901533002/diff/200001/net/http/http_network_...
> net/http/http_network_transaction.cc:440: const std::string
> priority_string(RequestPriorityToString(priority));
> On 2016/05/25 15:32:15, mmenke wrote:
> > On 2016/05/24 23:20:30, Randy Smith - Not in Fridays wrote:
> > > On 2016/05/24 22:59:52, mmenke wrote:
> > > > On 2016/05/24 21:52:50, mmenke wrote:
> > > > > On 2016/05/24 21:38:50, Randy Smith - Not in Fridays wrote:
> > > > > > I don't intend the above code to be what lands (why do a std::string
> > > > > > construction when the netlog isn't active?) but wanted to check in
> with
> > > you
> > > > > > about alternative strategies before I implemented them.  What I
*want*
> > to
> > > do
> > > > > is
> > > > > > to create a variation on NetLog::StringCallback that takes a char*,
> > > > initialize
> > > > > a
> > > > > > char* variable with the results of RequestPriorityToString, and pass
> > that
> > > to
> > > > > > StringCallback, as I could imagine that being useful in other places
> > > similar
> > > > > to
> > > > > > this one.  But I wanted to check with you as to whether there were
> > gotchas
> > > > to
> > > > > > that approach, or whether you'd just prefer me to create a new
> callback
> > > for
> > > > > this
> > > > > > particular event.
> > > > > 
> > > > > I think you mean takes a RequestPriority, not a char*?  I'm fine with
> > that.
> > > > 
> > > > Actually, you can just use an int called "priority", and net-internals
> will
> > > > convert it to a string, magically.
> > > 
> > > Ok, I'll take your word for it, but FWIW, I looked and couldn't find the
> code
> > > that would do that.  Done.
> > 
> > Oops, looks like you're right about this!  It's never logged along anywhere,
> so
> > we just use RequestPriorityToString.
> > 
> > That having been said, it seems like this should really be logged in
> > URLRequest::SetPriority...Which...Already logs priority changes.  So I guess
> we
> > don't need this.
> 
> I'm amused by how resistant I feel removing it, but yes, there's never a
> many->one mapping from URLRequests to HttpNetworkTransactions, so it's
probably
> fine to not log it here.  Removed.

It's not just the relation of the objects, but the URLRequest and
HttpNetworkTransaction write to the same BoundNetLog.  If they had different
NetLog objects, the double logging would make sense, regardless of their
relationship.

[mmenke, 2016-05-25 19:11:21]

https://codereview.chromium.org/1901533002/diff/240001/net/base/network_strea...
File net/base/network_stream_throttler.cc (right):

https://codereview.chromium.org/1901533002/diff/240001/net/base/network_strea...
net/base/network_stream_throttler.cc:18: bool IsThrottled() const override {
return throttled_; }
I'd suggest de-inlining this, since nothing else is, and inline + virtual seems
a little weird (Unless everything else is), too.

https://codereview.chromium.org/1901533002/diff/240001/net/base/network_strea...
net/base/network_stream_throttler.cc:22: friend NetworkStreamThrottlerImpl;
I'd suggest getting rid of this, and making these methods public - nothing else
has access to this class's private methods, anyways, so the "friend" seems
needless, and it gives the parent class access to its private member variables,
too.

https://codereview.chromium.org/1901533002/diff/240001/net/base/network_strea...
net/base/network_stream_throttler.cc:76: if (throttler_)
throttler_ can't be NULL.

https://codereview.chromium.org/1901533002/diff/240001/net/base/network_strea...
net/base/network_stream_throttler.cc:82: if (throttler_)
throttler_ can't be NULL.

https://codereview.chromium.org/1901533002/diff/240001/net/base/network_strea...
net/base/network_stream_throttler.cc:96: if (delegate_)
delegate_ also can't be NULL.

https://codereview.chromium.org/1901533002/diff/240001/net/base/network_strea...
net/base/network_stream_throttler.cc:122:
stream->set_queue_pointer(priority_queue_.Insert(stream, new_priority));
BUG:  I think this causes problems, if we've already unthrottled the request. 
When we actually do something with the throttler, we'll need to make sure to
have a test for that case.

https://codereview.chromium.org/1901533002/diff/240001/net/base/network_strea...
File net/base/network_stream_throttler.h (right):

https://codereview.chromium.org/1901533002/diff/240001/net/base/network_strea...
net/base/network_stream_throttler.h:12: #include "net/base/priority_queue.h"
This can be in the cc file

https://codereview.chromium.org/1901533002/diff/240001/net/base/network_strea...
net/base/network_stream_throttler.h:26: class NET_EXPORT_PRIVATE
NetworkStreamThrottler {
Throttler::Throttle seems a bit confusing, as it's unclear that a "Throttler"
manages "Throttles.  Also, since we're throttling well above the StreamSocket
(SocketStream?) layer, suggest removing the "Stream" as well.

So...  maybe rename NetworkStreamThrottler to [Network]ThrottleManager, or
HttpTransactionThrottleManager, or somesuch, and keep Throttle the same?  Could
use "Scheduler" instead of ThrottleManager.  Open to other ideas.

https://codereview.chromium.org/1901533002/diff/240001/net/base/network_strea...
net/base/network_stream_throttler.h:30: class NET_EXPORT_PRIVATE Delegate {
Maybe call this ThrottleDelegate, just because it seems more closely tied to the
throttle than to the throttler.

If the Throttler and Throttle weren't so closely tied together, I'd suggest 3
separate header files, and making them all top level classes.  Could even go
that route, if we made NetworkStreamThrottlerImpl have its own header, I
suppose.

https://codereview.chromium.org/1901533002/diff/240001/net/http/http_network_...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1901533002/diff/240001/net/http/http_network_...
net/http/http_network_transaction.cc:612: // initializing as throttled only
transition from throttled->unthrottled.
This sentence is awkward.  Maybe "This DCHECK is dependent on the only
transition being from throttled to unthrottled."

https://codereview.chromium.org/1901533002/diff/240001/net/http/http_network_...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1901533002/diff/240001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:193: void
OnThrottleDestroyed(TestThrottle* throttle) {
Could make sure throttle is in outstanding_throttles_.

https://codereview.chromium.org/1901533002/diff/240001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:16059: // state (and stays in that
state) is not blocked.
Need to fix grammar here (Unclear if is/stays refer to transactions or
throttler, I assume you just need to add a pair of "that"s?)

https://codereview.chromium.org/1901533002/diff/240001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:16092: EXPECT_EQ(OK, rv);
Suggest merging the last 3 lines to EXPECT_EQ(OK, callback.GetResult(rv));

A lot of tests still use the double expect, but the fact is we don't really care
if we get ERR_IO_PENDING the first time, or if it completes synchronously.

The difference matters for other throttler tests, of course.

https://codereview.chromium.org/1901533002/diff/240001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:16121: // Start a request that
will be throttled at start; confirm doesn't continue.
nit:  Maybe "confirm doesn't continue" -> "confirm it doesn't complete"?  (For
all of these - think you have 5 copies of this comment?)

https://codereview.chromium.org/1901533002/diff/240001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:16211:
throttler->set_throttle_new_requests(true);
Suggest this before creating the transaction (Less regression prone, just
remember when you were creating the throttles, in your original CL).  Also,
that's what the other tests all seem to do.

https://codereview.chromium.org/1901533002/diff/240001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:16288: EXPECT_EQ(OK,
callback.WaitForResult());
We should also check trans1 succeeds - that's the one that unthrottles in its
own callback.

https://codereview.chromium.org/1901533002/diff/240001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:16337: // Arrange fo the set
priority call on the above transaction to delete
fo -> for

https://codereview.chromium.org/1901533002/diff/240001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:16345: trans_p = nullptr;  // No
longer a valid pointer.
suggest _p -> _ptr (Seems more common, at least I haven't seen _p used much)

[Randy Smith (Not in Mondays), 2016-05-25 22:46:20]

Description was changed from

==========
Implementation of network level stream throttler.

NetworkStreamThrottler serves throttle objects to consumers (currently
HttpNetworkTransaction) that inform the consumer of their throttled state
and let the throttler track their lifetime.

BUG=600839
R=mmenke@chromium.org
==========

to

==========
Implementation of network level throttler.

NetworkThrottleManager serves throttle objects to consumers (currently
HttpNetworkTransaction) that inform the consumer of their throttled state
and let the throttler track their lifetime.

BUG=600839
R=mmenke@chromium.org
==========

[Randy Smith (Not in Mondays), 2016-05-25 23:48:25]

Ok, I think I got everything (the rename was the tricky part).  PTAL?

https://codereview.chromium.org/1901533002/diff/240001/net/base/network_strea...
File net/base/network_stream_throttler.cc (right):

https://codereview.chromium.org/1901533002/diff/240001/net/base/network_strea...
net/base/network_stream_throttler.cc:18: bool IsThrottled() const override {
return throttled_; }
On 2016/05/25 19:11:20, mmenke wrote:
> I'd suggest de-inlining this, since nothing else is, and inline + virtual
seems
> a little weird (Unless everything else is), too.

Done.

https://codereview.chromium.org/1901533002/diff/240001/net/base/network_strea...
net/base/network_stream_throttler.cc:22: friend NetworkStreamThrottlerImpl;
On 2016/05/25 19:11:20, mmenke wrote:
> I'd suggest getting rid of this, and making these methods public - nothing
else
> has access to this class's private methods, anyways, so the "friend" seems
> needless, and it gives the parent class access to its private member
variables,
> too.

Right, sorry, I think you suggested that before and I decided it was a good
suggestion and just spaced.  Done.

https://codereview.chromium.org/1901533002/diff/240001/net/base/network_strea...
net/base/network_stream_throttler.cc:76: if (throttler_)
On 2016/05/25 19:11:20, mmenke wrote:
> throttler_ can't be NULL.

Done.

https://codereview.chromium.org/1901533002/diff/240001/net/base/network_strea...
net/base/network_stream_throttler.cc:82: if (throttler_)
On 2016/05/25 19:11:20, mmenke wrote:
> throttler_ can't be NULL.

Done.

https://codereview.chromium.org/1901533002/diff/240001/net/base/network_strea...
net/base/network_stream_throttler.cc:95: // TODO(rdsmith): Figure out if/how
this needs to be made asynchronous.
Just calling out so you notice: Removing this line; I didn't intend to land with
this.

https://codereview.chromium.org/1901533002/diff/240001/net/base/network_strea...
net/base/network_stream_throttler.cc:96: if (delegate_)
On 2016/05/25 19:11:20, mmenke wrote:
> delegate_ also can't be NULL.

That comes from the consumer, so it seems worth a DCHECK(), but otherwise, done.

https://codereview.chromium.org/1901533002/diff/240001/net/base/network_strea...
net/base/network_stream_throttler.cc:122:
stream->set_queue_pointer(priority_queue_.Insert(stream, new_priority));
On 2016/05/25 19:11:20, mmenke wrote:
> BUG:  I think this causes problems, if we've already unthrottled the request. 
> When we actually do something with the throttler, we'll need to make sure to
> have a test for that case.

Help me understand better?  We don't want to call
delegate->OnThrottleStateChanged() twice, agreed (or if we do, we need to change
the interface contract and probably the name).  That's not an issue with this
code because we don't call it even once :-}.  But If we make sure to not call it
twice, what's the problem with playing with the priority queue?  (Agreed, we'll
need to store state *somewhere* about whether or not we've called that method,
though throttled_ may be enough.)  (Actually, when don't I just make that
change, presuming it's what you mean?  :-}  Done.)

We want the other elements in the priority queue to react to priority changes of
a stream after unthrottling but before completion.  As a specific example, when
I actually implement THROTTLED semantics, if a request starts out in THROTTLED
and makes it past the throttling point in HttpNetworkTransaction, we still want
it to contribute to blocking other THROTTLED requests from making it past that
point.  If it's then moved into a different priority bucket, we want it to stop
contributing to blocking those other requests.

https://codereview.chromium.org/1901533002/diff/240001/net/base/network_strea...
File net/base/network_stream_throttler.h (right):

https://codereview.chromium.org/1901533002/diff/240001/net/base/network_strea...
net/base/network_stream_throttler.h:12: #include "net/base/priority_queue.h"
On 2016/05/25 19:11:20, mmenke wrote:
> This can be in the cc file

Done.

https://codereview.chromium.org/1901533002/diff/240001/net/base/network_strea...
net/base/network_stream_throttler.h:26: class NET_EXPORT_PRIVATE
NetworkStreamThrottler {
On 2016/05/25 19:11:20, mmenke wrote:
> Throttler::Throttle seems a bit confusing, as it's unclear that a "Throttler"
> manages "Throttles.  Also, since we're throttling well above the StreamSocket
> (SocketStream?) layer, suggest removing the "Stream" as well.
> 
> So...  maybe rename NetworkStreamThrottler to [Network]ThrottleManager, or
> HttpTransactionThrottleManager, or somesuch, and keep Throttle the same? 
Could
> use "Scheduler" instead of ThrottleManager.  Open to other ideas.

I'm fine with renaming, and fine with NetworkThrottleManager.  Done.

https://codereview.chromium.org/1901533002/diff/240001/net/base/network_strea...
net/base/network_stream_throttler.h:30: class NET_EXPORT_PRIVATE Delegate {
On 2016/05/25 19:11:20, mmenke wrote:
> Maybe call this ThrottleDelegate, just because it seems more closely tied to
the
> throttle than to the throttler.

Done.

> If the Throttler and Throttle weren't so closely tied together, I'd suggest 3
> separate header files, and making them all top level classes.  Could even go
> that route, if we made NetworkStreamThrottlerImpl have its own header, I
> suppose.

Given the current configuration, I'd rather not go this way; it seems like a lot
of file proliferation for very little gain.  But if you want me to, I will.

https://codereview.chromium.org/1901533002/diff/240001/net/http/http_network_...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1901533002/diff/240001/net/http/http_network_...
net/http/http_network_transaction.cc:612: // initializing as throttled only
transition from throttled->unthrottled.
On 2016/05/25 19:11:20, mmenke wrote:
> This sentence is awkward.  Maybe "This DCHECK is dependent on the only
> transition being from throttled to unthrottled."

Done.

https://codereview.chromium.org/1901533002/diff/240001/net/http/http_network_...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1901533002/diff/240001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:193: void
OnThrottleDestroyed(TestThrottle* throttle) {
On 2016/05/25 19:11:20, mmenke wrote:
> Could make sure throttle is in outstanding_throttles_.

Done.

https://codereview.chromium.org/1901533002/diff/240001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:16059: // state (and stays in that
state) is not blocked.
On 2016/05/25 19:11:20, mmenke wrote:
> Need to fix grammar here (Unclear if is/stays refer to transactions or
> throttler, I assume you just need to add a pair of "that"s?)

I think I've clarified; let me know if you still think there's a problem.

https://codereview.chromium.org/1901533002/diff/240001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:16092: EXPECT_EQ(OK, rv);
On 2016/05/25 19:11:20, mmenke wrote:
> Suggest merging the last 3 lines to EXPECT_EQ(OK, callback.GetResult(rv));
> 
> A lot of tests still use the double expect, but the fact is we don't really
care
> if we get ERR_IO_PENDING the first time, or if it completes synchronously.
> 
> The difference matters for other throttler tests, of course.

Done.

https://codereview.chromium.org/1901533002/diff/240001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:16121: // Start a request that
will be throttled at start; confirm doesn't continue.
On 2016/05/25 19:11:20, mmenke wrote:
> nit:  Maybe "confirm doesn't continue" -> "confirm it doesn't complete"?  (For
> all of these - think you have 5 copies of this comment?)

Done.

https://codereview.chromium.org/1901533002/diff/240001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:16211:
throttler->set_throttle_new_requests(true);
On 2016/05/25 19:11:20, mmenke wrote:
> Suggest this before creating the transaction (Less regression prone, just
> remember when you were creating the throttles, in your original CL).  Also,
> that's what the other tests all seem to do.

Good catch; this is implicitly relying on the implementation of
HttpNetworkTransaction, not in a good way.  Done.

https://codereview.chromium.org/1901533002/diff/240001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:16288: EXPECT_EQ(OK,
callback.WaitForResult());
On 2016/05/25 19:11:20, mmenke wrote:
> We should also check trans1 succeeds - that's the one that unthrottles in its
> own callback.

Done.

https://codereview.chromium.org/1901533002/diff/240001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:16337: // Arrange fo the set
priority call on the above transaction to delete
On 2016/05/25 19:11:20, mmenke wrote:
> fo -> for

Done.

https://codereview.chromium.org/1901533002/diff/240001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:16345: trans_p = nullptr;  // No
longer a valid pointer.
On 2016/05/25 19:11:20, mmenke wrote:
> suggest _p -> _ptr (Seems more common, at least I haven't seen _p used much)

Done.

[mmenke, 2016-05-25 23:58:05]

https://codereview.chromium.org/1901533002/diff/240001/net/base/network_strea...
File net/base/network_stream_throttler.cc (right):

https://codereview.chromium.org/1901533002/diff/240001/net/base/network_strea...
net/base/network_stream_throttler.cc:122:
stream->set_queue_pointer(priority_queue_.Insert(stream, new_priority));
On 2016/05/25 23:48:23, Randy Smith - Not in Fridays wrote:
> On 2016/05/25 19:11:20, mmenke wrote:
> > BUG:  I think this causes problems, if we've already unthrottled the
request. 
> > When we actually do something with the throttler, we'll need to make sure to
> > have a test for that case.
> 
> Help me understand better?  We don't want to call
> delegate->OnThrottleStateChanged() twice, agreed (or if we do, we need to
change
> the interface contract and probably the name).  That's not an issue with this
> code because we don't call it even once :-}.  But If we make sure to not call
it
> twice, what's the problem with playing with the priority queue?  (Agreed,
we'll
> need to store state *somewhere* about whether or not we've called that method,
> though throttled_ may be enough.)  (Actually, when don't I just make that
> change, presuming it's what you mean?  :-}  Done.)
> 
> We want the other elements in the priority queue to react to priority changes
of
> a stream after unthrottling but before completion.  As a specific example,
when
> I actually implement THROTTLED semantics, if a request starts out in THROTTLED
> and makes it past the throttling point in HttpNetworkTransaction, we still
want
> it to contribute to blocking other THROTTLED requests from making it past that
> point.  If it's then moved into a different priority bucket, we want it to
stop
> contributing to blocking those other requests.

Suppose this request has already been unthrottled...HttpNetworkTransaction still
has a pointer to it, and calls this method.  It's one of the X active items in
the priority queue.  We remove it from the priority queue, the priority queue
start another queued item, and then we add it back to the priority queue.  We're
now running X+1 items, which is too many.  The priority queue thinks we're still
running X.  Then one of those items completes, and we remove it from the queue. 
It tries to start this throttle again.  The HttpNetworkTransaction breaks badly,
since it doesn't expect NotifyUnthrottled to be called twice.

[Randy Smith (Not in Mondays), 2016-05-26 14:56:39]

https://codereview.chromium.org/1901533002/diff/240001/net/base/network_strea...
File net/base/network_stream_throttler.cc (right):

https://codereview.chromium.org/1901533002/diff/240001/net/base/network_strea...
net/base/network_stream_throttler.cc:122:
stream->set_queue_pointer(priority_queue_.Insert(stream, new_priority));
On 2016/05/25 23:58:04, mmenke wrote:
> On 2016/05/25 23:48:23, Randy Smith - Not in Fridays wrote:
> > On 2016/05/25 19:11:20, mmenke wrote:
> > > BUG:  I think this causes problems, if we've already unthrottled the
> request. 
> > > When we actually do something with the throttler, we'll need to make sure
to
> > > have a test for that case.
> > 
> > Help me understand better?  We don't want to call
> > delegate->OnThrottleStateChanged() twice, agreed (or if we do, we need to
> change
> > the interface contract and probably the name).  That's not an issue with
this
> > code because we don't call it even once :-}.  But If we make sure to not
call
> it
> > twice, what's the problem with playing with the priority queue?  (Agreed,
> we'll
> > need to store state *somewhere* about whether or not we've called that
method,
> > though throttled_ may be enough.)  (Actually, when don't I just make that
> > change, presuming it's what you mean?  :-}  Done.)
> > 
> > We want the other elements in the priority queue to react to priority
changes
> of
> > a stream after unthrottling but before completion.  As a specific example,
> when
> > I actually implement THROTTLED semantics, if a request starts out in
THROTTLED
> > and makes it past the throttling point in HttpNetworkTransaction, we still
> want
> > it to contribute to blocking other THROTTLED requests from making it past
that
> > point.  If it's then moved into a different priority bucket, we want it to
> stop
> > contributing to blocking those other requests.
> 
> Suppose this request has already been unthrottled...HttpNetworkTransaction
still
> has a pointer to it, and calls this method.  It's one of the X active items in
> the priority queue.  We remove it from the priority queue, the priority queue
> start another queued item, and then we add it back to the priority queue. 
We're
> now running X+1 items, which is too many.  

That's a problem, and one we're going to need to deal with, either by rewriting
the priority queue to support reprioritizations or not implementing this
functionality through that queue.  I'm inclined to think it's a problem for the
next CL, though.

> The priority queue thinks we're still
> running X.  Then one of those items completes, and we remove it from the
queue. 
> It tries to start this throttle again.  The HttpNetworkTransaction breaks
badly,
> since it doesn't expect NotifyUnthrottled to be called twice.

It tries to start the throttle again via calling NotifyUnthrottled on the
ThrottleImpl, which returns vacuously because of the change I just added in
response to this comment.  So I think this isn't a problem anymore?

[mmenke, 2016-05-26 15:05:54]

https://codereview.chromium.org/1901533002/diff/240001/net/base/network_strea...
File net/base/network_stream_throttler.cc (right):

https://codereview.chromium.org/1901533002/diff/240001/net/base/network_strea...
net/base/network_stream_throttler.cc:122:
stream->set_queue_pointer(priority_queue_.Insert(stream, new_priority));
On 2016/05/26 14:56:39, Randy Smith - Not in Fridays wrote:
> On 2016/05/25 23:58:04, mmenke wrote:
> > On 2016/05/25 23:48:23, Randy Smith - Not in Fridays wrote:
> > > On 2016/05/25 19:11:20, mmenke wrote:
> > > > BUG:  I think this causes problems, if we've already unthrottled the
> > request. 
> > > > When we actually do something with the throttler, we'll need to make
sure
> to
> > > > have a test for that case.
> > > 
> > > Help me understand better?  We don't want to call
> > > delegate->OnThrottleStateChanged() twice, agreed (or if we do, we need to
> > change
> > > the interface contract and probably the name).  That's not an issue with
> this
> > > code because we don't call it even once :-}.  But If we make sure to not
> call
> > it
> > > twice, what's the problem with playing with the priority queue?  (Agreed,
> > we'll
> > > need to store state *somewhere* about whether or not we've called that
> method,
> > > though throttled_ may be enough.)  (Actually, when don't I just make that
> > > change, presuming it's what you mean?  :-}  Done.)
> > > 
> > > We want the other elements in the priority queue to react to priority
> changes
> > of
> > > a stream after unthrottling but before completion.  As a specific example,
> > when
> > > I actually implement THROTTLED semantics, if a request starts out in
> THROTTLED
> > > and makes it past the throttling point in HttpNetworkTransaction, we still
> > want
> > > it to contribute to blocking other THROTTLED requests from making it past
> that
> > > point.  If it's then moved into a different priority bucket, we want it to
> > stop
> > > contributing to blocking those other requests.
> > 
> > Suppose this request has already been unthrottled...HttpNetworkTransaction
> still
> > has a pointer to it, and calls this method.  It's one of the X active items
in
> > the priority queue.  We remove it from the priority queue, the priority
queue
> > start another queued item, and then we add it back to the priority queue. 
> We're
> > now running X+1 items, which is too many.  
> 
> That's a problem, and one we're going to need to deal with, either by
rewriting
> the priority queue to support reprioritizations or not implementing this
> functionality through that queue.  I'm inclined to think it's a problem for
the
> next CL, though.

Oops...I had thought we were using PrioritizedDispatcher, rather than
PriorityQueue (Actually, I hadn't realized they were different classes).

> > The priority queue thinks we're still
> > running X.  Then one of those items completes, and we remove it from the
> queue. 
> > It tries to start this throttle again.  The HttpNetworkTransaction breaks
> badly,
> > since it doesn't expect NotifyUnthrottled to be called twice.
> 
> It tries to start the throttle again via calling NotifyUnthrottled on the
> ThrottleImpl, which returns vacuously because of the change I just added in
> response to this comment.  So I think this isn't a problem anymore?

I'd actually rather a DCHECK there - calling NotifyUnthrottled twice in a row
seems more like a bug to me.  Logic to prevent that should be in the caller of
that method.

[mmenke, 2016-05-26 16:23:26]

LGTM.  Just one optional suggestion, and my previous suggestion of DCHECK.

https://codereview.chromium.org/1901533002/diff/260001/net/http/http_network_...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1901533002/diff/260001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:195:
outstanding_throttles_.end());
optional:  Could use count() instead.  I just prefer it because it fits on one
line, and is marginally more readable.  find/end is nice in that those methods
are more common when using iterators.  Anyhow, either is fine.

[Randy Smith (Not in Mondays), 2016-05-28 02:27:58]

rdsmith@chromium.org changed reviewers:
+ avi@chromium.org

[Randy Smith (Not in Mondays), 2016-05-28 02:28:00]

Switched over to DCHECK.  Thanks!

Avi, could you take a look at core_tab_helper.cc?

https://codereview.chromium.org/1901533002/diff/260001/net/http/http_network_...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1901533002/diff/260001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:195:
outstanding_throttles_.end());
On 2016/05/26 16:23:26, mmenke wrote:
> optional:  Could use count() instead.  I just prefer it because it fits on one
> line, and is marginally more readable.  find/end is nice in that those methods
> are more common when using iterators.  Anyhow, either is fine.

Done.

[Randy Smith (Not in Mondays), 2016-05-28 02:35:12]

On 2016/05/28 02:28:00, Randy Smith - Not in Fridays wrote:
> Switched over to DCHECK.  Thanks!
> 
> Avi, could you take a look at core_tab_helper.cc?
> 
>
https://codereview.chromium.org/1901533002/diff/260001/net/http/http_network_...
> File net/http/http_network_transaction_unittest.cc (right):
> 
>
https://codereview.chromium.org/1901533002/diff/260001/net/http/http_network_...
> net/http/http_network_transaction_unittest.cc:195:
> outstanding_throttles_.end());
> On 2016/05/26 16:23:26, mmenke wrote:
> > optional:  Could use count() instead.  I just prefer it because it fits on
one
> > line, and is marginally more readable.  find/end is nice in that those
methods
> > are more common when using iterators.  Anyhow, either is fine.
> 
> Done.

Whoops, one final change, doesn't touch core_tab_helper.cc.  As per Matt's
request in
https://codereview.chromium.org/1901533002/diff/180001/net/http/http_network_...
I've removed all request.load_flags = 0 from the test file.

[Avi (use Gerrit), 2016-05-28 02:43:06]

core_tab_helper.cc LGTM

Nitpicked typos everywhere else.

https://codereview.chromium.org/1901533002/diff/280001/net/base/network_throt...
File net/base/network_throttle_manager.cc (right):

https://codereview.chromium.org/1901533002/diff/280001/net/base/network_throt...
net/base/network_throttle_manager.cc:20: // Caller must arrange that |*delegate|
and |*throttler| outlives
"outlive"; two subjects need the plural verb.

https://codereview.chromium.org/1901533002/diff/280001/net/base/network_throt...
net/base/network_throttle_manager.cc:39: // may result in re-entrant calls into
the throtter or
"into the throttler"?

https://codereview.chromium.org/1901533002/diff/280001/net/base/network_throt...
File net/base/network_throttle_manager.h (right):

https://codereview.chromium.org/1901533002/diff/280001/net/base/network_throt...
net/base/network_throttle_manager.h:36: // or another throttle) or the
destruciton of a Throttle, and if
typo: destruction

https://codereview.chromium.org/1901533002/diff/280001/net/http/http_network_...
File net/http/http_network_transaction.h (right):

https://codereview.chromium.org/1901533002/diff/280001/net/http/http_network_...
net/http/http_network_transaction.h:407: // Communciate lifetime of transaction
to the throttler, and
Typo: Communicate

[Randy Smith (Not in Mondays), 2016-05-30 13:58:22]

All done; thanks for the copyediting!

https://codereview.chromium.org/1901533002/diff/280001/net/base/network_throt...
File net/base/network_throttle_manager.cc (right):

https://codereview.chromium.org/1901533002/diff/280001/net/base/network_throt...
net/base/network_throttle_manager.cc:20: // Caller must arrange that |*delegate|
and |*throttler| outlives
On 2016/05/28 02:43:06, Avi wrote:
> "outlive"; two subjects need the plural verb.

Done.

https://codereview.chromium.org/1901533002/diff/280001/net/base/network_throt...
net/base/network_throttle_manager.cc:39: // may result in re-entrant calls into
the throtter or
On 2016/05/28 02:43:06, Avi wrote:
> "into the throttler"?

Presuming you're pointing out the absence of the "l".  Done.

https://codereview.chromium.org/1901533002/diff/280001/net/base/network_throt...
File net/base/network_throttle_manager.h (right):

https://codereview.chromium.org/1901533002/diff/280001/net/base/network_throt...
net/base/network_throttle_manager.h:36: // or another throttle) or the
destruciton of a Throttle, and if
On 2016/05/28 02:43:06, Avi wrote:
> typo: destruction

Done.

https://codereview.chromium.org/1901533002/diff/280001/net/http/http_network_...
File net/http/http_network_transaction.h (right):

https://codereview.chromium.org/1901533002/diff/280001/net/http/http_network_...
net/http/http_network_transaction.h:407: // Communciate lifetime of transaction
to the throttler, and
On 2016/05/28 02:43:06, Avi wrote:
> Typo: Communicate

Done.

[Randy Smith (Not in Mondays), 2016-08-10 21:14:56]

The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 21:15:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-10 21:27:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-10 21:27:55]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Randy Smith (Not in Mondays), 2016-08-10 21:59:09]

The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 22:00:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-10 23:27:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-10 23:27:12]

Dry run: This issue passed the CQ dry run.

[Randy Smith (Not in Mondays), 2016-08-22 00:43:28]

The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-22 00:43:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-22 02:00:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-22 02:00:13]

Dry run: This issue passed the CQ dry run.

[Randy Smith (Not in Mondays), 2016-09-22 22:23:51]

The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-22 22:24:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-22 22:39:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-22 22:39:55]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Randy Smith (Not in Mondays), 2016-09-23 21:21:01]

Patchset #29 (id:560001) has been deleted

[Randy Smith (Not in Mondays), 2016-09-23 21:25:51]

Merge fix from dependent PS.

[Randy Smith (Not in Mondays), 2016-09-23 21:29:51]

Adapt to new netlog type list.

[Randy Smith (Not in Mondays), 2016-09-23 21:30:08]

The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 21:30:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 21:42:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-23 21:42:33]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Randy Smith (Not in Mondays), 2016-09-27 18:13:04]

The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-27 18:13:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-27 18:46:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-27 18:46:28]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Randy Smith (Not in Mondays), 2016-10-20 20:48:57]

The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 20:49:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-20 21:15:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-20 21:15:16]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Randy Smith (Not in Mondays), 2016-10-21 17:22:10]

The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-21 17:22:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-21 17:49:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-21 17:50:00]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Randy Smith (Not in Mondays), 2016-10-21 18:18:06]

The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run

[Randy Smith (Not in Mondays), 2016-10-21 18:18:56]

rdsmith@chromium.org changed reviewers:
+ rch@chromium.org

[Randy Smith (Not in Mondays), 2016-10-21 18:18:58]

Ryan, could you take a look at PS33->34?  I needed to tweak a Quic test to get
this to work.

[commit-bot: I haz the power, 2016-10-21 18:19:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ryan Hamilton, 2016-10-21 18:37:28]

On 2016/10/21 18:18:58, Randy Smith - Not in Mondays wrote:
> Ryan, could you take a look at PS33->34?  I needed to tweak a Quic test to get
> this to work.

LGTM!

[commit-bot: I haz the power, 2016-10-21 20:14:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-21 20:14:39]

Dry run: This issue passed the CQ dry run.

[Randy Smith (Not in Mondays), 2016-10-21 20:15:37]

The CQ bit was checked by rdsmith@chromium.org

[Randy Smith (Not in Mondays), 2016-10-21 20:15:37]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org,
avi@chromium.org
Link to the patchset: https://codereview.chromium.org/1901533002/#ps680001
(title: "Destroy transaction before session in QuicUploadWriteError.")

[commit-bot: I haz the power, 2016-10-21 20:16:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-21 20:38:16]

Description was changed from

==========
Implementation of network level throttler.

NetworkThrottleManager serves throttle objects to consumers (currently
HttpNetworkTransaction) that inform the consumer of their throttled state
and let the throttler track their lifetime.

BUG=600839
R=mmenke@chromium.org
==========

to

==========
Implementation of network level throttler.

NetworkThrottleManager serves throttle objects to consumers (currently
HttpNetworkTransaction) that inform the consumer of their throttled state
and let the throttler track their lifetime.

BUG=600839
R=mmenke@chromium.org
==========

[commit-bot: I haz the power, 2016-10-21 20:38:22]

Committed patchset #34 (id:680001)

[commit-bot: I haz the power, 2016-10-21 20:40:40]

Description was changed from

==========
Implementation of network level throttler.

NetworkThrottleManager serves throttle objects to consumers (currently
HttpNetworkTransaction) that inform the consumer of their throttled state
and let the throttler track their lifetime.

BUG=600839
R=mmenke@chromium.org
==========

to

==========
Implementation of network level throttler.

NetworkThrottleManager serves throttle objects to consumers (currently
HttpNetworkTransaction) that inform the consumer of their throttled state
and let the throttler track their lifetime.

BUG=600839
R=mmenke@chromium.org

Committed: https://crrev.com/1d343be5a6202b9f5acaa8c04131fa159d0627c0
Cr-Commit-Position: refs/heads/master@{#426879}
==========

[commit-bot: I haz the power, 2016-10-21 20:40:41]

Patchset 34 (id:??) landed as
https://crrev.com/1d343be5a6202b9f5acaa8c04131fa159d0627c0
Cr-Commit-Position: refs/heads/master@{#426879}