net: Socket pools prioritize requests with ignore_limits

net:  Socket pools prioritize requests with ignore_limits.
Also, when a socket request is cancelled when there are
more ConnectJobs than socket requests for a group, no
ConnectJobs are cancelled.

These two changes are needed to prevent a lockup due to
a socket created for sync XHR's socket being giving to another socket request of equal or higher priority.

This issue may well still exist with SPDY sessions.

BUG=155536
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=205491

[mmenke, 2013-06-03 19:44:47]

Simonjam's patch to increase the priority of sync requests can still run into
the problem if there are already 7 requests of equal priority out.  This
probably won't happen, but seems best not to leave it to chance.

SPDY has its own limits, so may be able to run into this same issue.  I'm not
sure there's a perfect fix for it, SPDY side, since the server sets the request
limit.  We could cancel requests or reserve a slot, but if we reserve the slot,
we don't really have any guarantee that an earlier ignore_limits request won't
block on a later one.

[eroman, 2013-06-04 20:42:21]

lgtm

https://codereview.chromium.org/15927019/diff/12001/net/socket/client_socket_...
File net/socket/client_socket_pool_base.cc (right):

https://codereview.chromium.org/15927019/diff/12001/net/socket/client_socket_...
net/socket/client_socket_pool_base.cc:195: // If not ignoring the socket pool
limits, skip over all requests that do.
rather than splitting things up into 2 loops, I suggest combining them into one.
perhaps like this:

================
while (it != pending_requests->end() &&
       r->ignore_limits() <= (*it)->ignore_limits() &&
       r->priority() <= (*it)->priority()) {
  ++it;
}
pending_requests->insert(it, r);
================

...or for better clarity, have a function which compares 2 requests's "effective
priority".

https://codereview.chromium.org/15927019/diff/12001/net/socket/client_socket_...
net/socket/client_socket_pool_base.cc:205: //                |ignore_limits| set
have the highest priority?
nit: wording. i think "have the" --> "to have the"]

https://codereview.chromium.org/15927019/diff/12001/net/socket/client_socket_...
File net/socket/client_socket_pool_base.h (right):

https://codereview.chromium.org/15927019/diff/12001/net/socket/client_socket_...
net/socket/client_socket_pool_base.h:447: // sockets.  Sockets which ignore the
socket pool limits are first.  Then
[no action necessary] my impression is that it is more common to use one space
after periods in chromium code.

nevertheless, +1 for adding more comments.

https://codereview.chromium.org/15927019/diff/12001/net/socket/client_socket_...
File net/socket/client_socket_pool_base_unittest.cc (right):

https://codereview.chromium.org/15927019/diff/12001/net/socket/client_socket_...
net/socket/client_socket_pool_base_unittest.cc:3883: // Test that when a socket
pool and group are their limits, a request with
"are at"

[mmenke, 2013-06-05 04:32:24]

Thanks for the feedback!

Just out of paranoia, probably best if you review the comparison function I
added before I land this.

https://codereview.chromium.org/15927019/diff/12001/net/socket/client_socket_...
File net/socket/client_socket_pool_base.cc (right):

https://codereview.chromium.org/15927019/diff/12001/net/socket/client_socket_...
net/socket/client_socket_pool_base.cc:195: // If not ignoring the socket pool
limits, skip over all requests that do.
On 2013/06/04 20:42:21, eroman wrote:
> rather than splitting things up into 2 loops, I suggest combining them into
one.
> perhaps like this:
> 
> ================
> while (it != pending_requests->end() &&
>        r->ignore_limits() <= (*it)->ignore_limits() &&
>        r->priority() <= (*it)->priority()) {
>   ++it;
> }
> pending_requests->insert(it, r);
> ================
> 
> ...or for better clarity, have a function which compares 2 requests's
"effective
> priority".

Done, went with the function approach and a 3-value comparison function (Could
use two value, but !RequestHigherEffectivePriority is a little weird).  Think
the bool inequality of the other approach is a little harder to follow, though
my check wasn't much better.

https://codereview.chromium.org/15927019/diff/12001/net/socket/client_socket_...
net/socket/client_socket_pool_base.cc:205: //                |ignore_limits| set
have the highest priority?
On 2013/06/04 20:42:21, eroman wrote:
> nit: wording. i think "have the" --> "to have the"]

It's actually correct:  "(requests with |ignore_limits| set) have the highest
priority".  I've removed the word "set", and it still has the same meaning.

https://codereview.chromium.org/15927019/diff/12001/net/socket/client_socket_...
File net/socket/client_socket_pool_base.h (right):

https://codereview.chromium.org/15927019/diff/12001/net/socket/client_socket_...
net/socket/client_socket_pool_base.h:447: // sockets.  Sockets which ignore the
socket pool limits are first.  Then
On 2013/06/04 20:42:21, eroman wrote:
> [no action necessary] my impression is that it is more common to use one space
> after periods in chromium code.
> 
> nevertheless, +1 for adding more comments.

You're definitely right about single space comments being more common, but there
are a large number of double space comments all over the place, including in
this file.  In this case, I just moved the comment from the cc file and updated
it - and it was using double spaces before (Which is, admittedly, what I
generally use myself as well).

While I think there are some places where comment consistency matters
(Capitalization of SPDY, for instance), I think it's a bit too late to worry
about comment spacing.  That having been said, I have gone ahead and switched to
single spaces in this CL.

https://codereview.chromium.org/15927019/diff/12001/net/socket/client_socket_...
File net/socket/client_socket_pool_base_unittest.cc (right):

https://codereview.chromium.org/15927019/diff/12001/net/socket/client_socket_...
net/socket/client_socket_pool_base_unittest.cc:3883: // Test that when a socket
pool and group are their limits, a request with
On 2013/06/04 20:42:21, eroman wrote:
> "are at"

Done.

[eroman, 2013-06-10 17:48:00]

lgtm

https://codereview.chromium.org/15927019/diff/25002/net/socket/client_socket_...
File net/socket/client_socket_pool_base.cc (right):

https://codereview.chromium.org/15927019/diff/25002/net/socket/client_socket_...
net/socket/client_socket_pool_base.cc:45: // Requests with |ignore_limits| set
have higher effective pririoty than those
typo priority

https://codereview.chromium.org/15927019/diff/25002/net/socket/client_socket_...
net/socket/client_socket_pool_base.cc:219: CompareEffectiveRequestPriority(*r,
*(*it)) <= 0) {
Given that the result is only compared <=0, would it be sufficient to define
"CompareEffectiveRequestPriority" as a 'less than' comparator instead?

[mmenke, 2013-06-10 18:17:26]

https://codereview.chromium.org/15927019/diff/25002/net/socket/client_socket_...
File net/socket/client_socket_pool_base.cc (right):

https://codereview.chromium.org/15927019/diff/25002/net/socket/client_socket_...
net/socket/client_socket_pool_base.cc:45: // Requests with |ignore_limits| set
have higher effective pririoty than those
On 2013/06/10 17:48:00, eroman wrote:
> typo priority

Done.

https://codereview.chromium.org/15927019/diff/25002/net/socket/client_socket_...
net/socket/client_socket_pool_base.cc:219: CompareEffectiveRequestPriority(*r,
*(*it)) <= 0) {
On 2013/06/10 17:48:00, eroman wrote:
> Given that the result is only compared <=0, would it be sufficient to define
> "CompareEffectiveRequestPriority" as a 'less than' comparator instead?

I could, but to maintain FIFO order, I'd have to either make a less-or-equal
comparator, or I'd have to negate it.  Or I suppose I could walk through the
list backwards.

Negating a comparator seems ugly to me.  Making it less-or-equal seems a bit
confusing.  Walking backwards also doesn't seem as clear as this is.  Thoughts?

[eroman, 2013-06-10 18:31:49]

https://codereview.chromium.org/15927019/diff/25002/net/socket/client_socket_...
File net/socket/client_socket_pool_base.cc (right):

https://codereview.chromium.org/15927019/diff/25002/net/socket/client_socket_...
net/socket/client_socket_pool_base.cc:219: CompareEffectiveRequestPriority(*r,
*(*it)) <= 0) {
On 2013/06/10 18:17:26, mmenke wrote:
> On 2013/06/10 17:48:00, eroman wrote:
> > Given that the result is only compared <=0, would it be sufficient to define
> > "CompareEffectiveRequestPriority" as a 'less than' comparator instead?
> 
> I could, but to maintain FIFO order, I'd have to either make a less-or-equal
> comparator, or I'd have to negate it.  Or I suppose I could walk through the
> list backwards.
> 
> Negating a comparator seems ugly to me.  Making it less-or-equal seems a bit
> confusing.  Walking backwards also doesn't seem as clear as this is. 
Thoughts?

I guess you could do something like
  !LessThan(b,a)

(both negating, and also flipping the order of |a| and |b|).

However I agree that is harder to read. As is is fine. I would also be in favor
of having a boolean comparator that returns either "greater than" or "less than
equal to" for simplicity.

I haven't thought this through, but <algorithm>'s std::upper_bound() might also
be applicable. (Although probably even harder to read).

LGTM whichever option you choose.

[mmenke1, 2013-06-10 18:34:28]

On 2013/06/10 18:31:49, eroman wrote:
>
https://codereview.chromium.org/15927019/diff/25002/net/socket/client_socket_...
> File net/socket/client_socket_pool_base.cc (right):
> 
>
https://codereview.chromium.org/15927019/diff/25002/net/socket/client_socket_...
> net/socket/client_socket_pool_base.cc:219: CompareEffectiveRequestPriority(*r,
> *(*it)) <= 0) {
> On 2013/06/10 18:17:26, mmenke wrote:
> > On 2013/06/10 17:48:00, eroman wrote:
> > > Given that the result is only compared <=0, would it be sufficient to
define
> > > "CompareEffectiveRequestPriority" as a 'less than' comparator instead?
> > 
> > I could, but to maintain FIFO order, I'd have to either make a less-or-equal
> > comparator, or I'd have to negate it.  Or I suppose I could walk through the
> > list backwards.
> > 
> > Negating a comparator seems ugly to me.  Making it less-or-equal seems a bit
> > confusing.  Walking backwards also doesn't seem as clear as this is. 
> Thoughts?
> 
> I guess you could do something like
>   !LessThan(b,a)
> 
> (both negating, and also flipping the order of |a| and |b|).
> 
> However I agree that is harder to read. As is is fine. I would also be in
favor
> of having a boolean comparator that returns either "greater than" or "less
than
> equal to" for simplicity.
> 
> I haven't thought this through, but <algorithm>'s std::upper_bound() might
also
> be applicable. (Although probably even harder to read).
> 
> LGTM whichever option you choose.

I'll just leave it as-is, then.  I think the current code makes the FIFO
ordering clearest.

Thanks for the feedback!

[commit-bot: I haz the power, 2013-06-10 18:35:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmenke@chromium.org/15927019/32001

[commit-bot: I haz the power, 2013-06-11 11:06:34]

Change committed as 205491