Close idle connections / SPDY sessions when needed

Attempting to re-land the feature.

Revert 127730 - Revert 127717 - Revert 118788 - Revert 113405 - Revert 113305 - Revert 113300 - Revert 112134 - Revert 112130 - Close idle connections / SPDY sessions when needed

Due to the idle connection state being held by different socket pools, it's possible for one socket pool to hold an idle socket in a lower layer socket pool. From the lower level socket pool's perspective, the socket is being "actively" used. From the higher socket pool's (including SpdySession, which is more of a connection manager) perspective, the connection is idle and can be closed if we have hit a limit.

Normally this isn't a big deal, except when we have a lot of idle SPDY connections and are connecting via a proxy, so we have low connection limits through the proxy server. We address this problem by allowing lower-level socket pools to tell higher level socket pools to close a socket.

Fixed ASAN test failures by removing .Times(1) and .Times(2) from CloseMultipleIdleSocketsHeldByLayeredPoolWhenNeeded unittest (this removes the tests relying on the order of std::set in CloseOneIdleConnectionInLayeredPool). ASAN is prob
ably causing the memory allocator to allocate the pools differently. The std::set is ordered by LayeredPool* which is the address of the LayeredPool (willchan).

Added NET_EXPORT for layered_pool class defintion to fix windows shared compile.

BUG=62364, 92244, 109876, 110368
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=127717

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

[Ryan Hamilton, 2012-03-12 18:32:08]

eroman: can you look at this CL?  It's is will's original CL + a fix you made in
http://code.google.com/p/chromium/issues/detail?id=109876 and a fix proposed in
http://code.google.com/p/chromium/issues/detail?id=110368.  I've verified that I
was able to reproduce the hang in 110368, and that it went away with the fix you
proposed.

[eroman, 2012-03-12 22:43:53]

Could you have either willchan or mmenke review this change? They are more
familiar with the code :-)

Cheers.

[Ryan Hamilton, 2012-03-12 22:49:14]

willchan/mmenke: could one of you please review this cl?  It's basically a CL
that was already landed + 1 fix which landed + 1 fix which was proposed (and
fixed the hang that mmenke identified)

[mmenke, 2012-03-12 22:52:24]

On 2012/03/12 22:49:14, Ryan Hamilton wrote:
> willchan/mmenke: could one of you please review this cl?  It's basically a CL
> that was already landed + 1 fix which landed + 1 fix which was proposed (and
> fixed the hang that mmenke identified)

I'm happy to review this, but I may not get to it until tomorrow
afternoon/evening.

[willchan no longer on Chromium, 2012-03-12 22:59:50]

I hope to look at it later, but sheriffing is keeping me busy.

On Mon, Mar 12, 2012 at 3:52 PM, <mmenke@chromium.org> wrote:

> On 2012/03/12 22:49:14, Ryan Hamilton wrote:
>
>> willchan/mmenke: could one of you please review this cl?  It's basically
>> a CL
>> that was already landed + 1 fix which landed + 1 fix which was proposed
>> (and
>> fixed the hang that mmenke identified)
>>
>
> I'm happy to review this, but I may not get to it until tomorrow
> afternoon/evening.
>
>
https://chromiumcodereview.**appspot.com/9667016/<https://chromiumcodereview....
>

[mmenke, 2012-03-14 14:44:48]

Still need to take a closer look at this, just a couple preliminary comments.

http://codereview.chromium.org/9667016/diff/1001/net/socket/client_socket_poo...
File net/socket/client_socket_pool_base.cc (right):

http://codereview.chromium.org/9667016/diff/1001/net/socket/client_socket_poo...
net/socket/client_socket_pool_base.cc:210: DCHECK(higher_layer_pools_.empty());
Perhaps this should be a CHECK.  It'll most likely result in us crashing in
RemoveLayeredPool, anyways, and best to crash early to get the nicest stack.

http://codereview.chromium.org/9667016/diff/1001/net/socket/client_socket_poo...
net/socket/client_socket_pool_base.cc:362: if (idle_socket_count() > 0) {
Comment for the future:  Wonder if we really need different code for each case
here.  Only reason for them now seems to be preconnects.

http://codereview.chromium.org/9667016/diff/1001/net/socket/client_socket_poo...
net/socket/client_socket_pool_base.cc:798: void
ClientSocketPoolBaseHelper::CheckForStalledSocketGroups() {
Should we check if higher level pools are stalled here?  I could see a case
where we have no idle sockets, and queue up more requests, then start finishing
up with the sockets on higher level pools, but keep them idle, despite having
pending requests.

http://codereview.chromium.org/9667016/diff/1001/net/socket/client_socket_poo...
net/socket/client_socket_pool_base.cc:829: CHECK((group && group_name) ||
(!group && !group_name));
Allowing |group| and |group_name| to be NULL should be documented in the
function description in the header.

http://codereview.chromium.org/9667016/diff/1001/net/socket/client_socket_poo...
net/socket/client_socket_pool_base.cc:935: if
(it->second->IsStalled(max_sockets_per_group_))
I don't think this is correct.  Either the description or the code is wrong.

The description says "Returns true if a new request may hit a per-pool (not
per-host) max socket limit", but this is returning true if a request *has
already* hit the max socket limit, unless "new request" means a request that's
already been added as a "pending request".

[Ryan Hamilton, 2012-03-15 21:44:31]

Still need to work on a unit test to verify that the hang has been fixed.

https://chromiumcodereview.appspot.com/9667016/diff/1001/net/socket/client_so...
File net/socket/client_socket_pool_base.cc (right):

https://chromiumcodereview.appspot.com/9667016/diff/1001/net/socket/client_so...
net/socket/client_socket_pool_base.cc:210: DCHECK(higher_layer_pools_.empty());
On 2012/03/14 14:44:48, Matt Menke wrote:
> Perhaps this should be a CHECK.  It'll most likely result in us crashing in
> RemoveLayeredPool, anyways, and best to crash early to get the nicest stack.

Done.

https://chromiumcodereview.appspot.com/9667016/diff/1001/net/socket/client_so...
net/socket/client_socket_pool_base.cc:362: if (idle_socket_count() > 0) {
On 2012/03/14 14:44:48, Matt Menke wrote:
> Comment for the future:  Wonder if we really need different code for each case
> here.  Only reason for them now seems to be preconnects.

Added your comment as a NOTE

https://chromiumcodereview.appspot.com/9667016/diff/1001/net/socket/client_so...
net/socket/client_socket_pool_base.cc:798: void
ClientSocketPoolBaseHelper::CheckForStalledSocketGroups() {
On 2012/03/14 14:44:48, Matt Menke wrote:
> Should we check if higher level pools are stalled here?  I could see a case
> where we have no idle sockets, and queue up more requests, then start
finishing
> up with the sockets on higher level pools, but keep them idle, despite having
> pending requests.

Possibly, though I think this CL is complicated enough that we should do it in a
subsequent CL, assuming that this doesn't get reverted :>

Will and I discussed that, for example, when an SSL socket is released to the
pool, we need to check to see if the underlying TCP socket pull is stalled, and
if so, we should close this socket.

I can't quite wrap my brain around your suggestion to see if it will take care
of this case or not, but your example definitely needs to be addressed
eventually.

https://chromiumcodereview.appspot.com/9667016/diff/1001/net/socket/client_so...
net/socket/client_socket_pool_base.cc:829: CHECK((group && group_name) ||
(!group && !group_name));
On 2012/03/14 14:44:48, Matt Menke wrote:
> Allowing |group| and |group_name| to be NULL should be documented in the
> function description in the header.

Done.

https://chromiumcodereview.appspot.com/9667016/diff/1001/net/socket/client_so...
net/socket/client_socket_pool_base.cc:935: if
(it->second->IsStalled(max_sockets_per_group_))
On 2012/03/14 14:44:48, Matt Menke wrote:
> I don't think this is correct.  Either the description or the code is wrong.
> 
> The description says "Returns true if a new request may hit a per-pool (not
> per-host) max socket limit", but this is returning true if a request *has
> already* hit the max socket limit, unless "new request" means a request that's
> already been added as a "pending request".

Will and I discussed this in great detail yesterday and today.  The logic is
correct...  We want this to return true if there is currently a request that is
blocked because of the pool's (not the groups) max_sockets_ limit.  I will
update the method description to reflect this.

[mmenke, 2012-03-16 18:01:15]

Sorry I've been slow with this review.  Trying to be very careful, given the
issues with it last time.

https://chromiumcodereview.appspot.com/9667016/diff/10005/net/http/http_netwo...
File net/http/http_network_transaction_spdy21_unittest.cc (right):

https://chromiumcodereview.appspot.com/9667016/diff/10005/net/http/http_netwo...
net/http/http_network_transaction_spdy21_unittest.cc:167:
SpdySession::set_default_protocol(SSLClientSocket::kProtoSPDY21);
nit:  Suggest you call this "CloseIdle..."

https://chromiumcodereview.appspot.com/9667016/diff/10005/net/http/http_proxy...
File net/http/http_proxy_client_socket_pool.h (right):

https://chromiumcodereview.appspot.com/9667016/diff/10005/net/http/http_proxy...
net/http/http_proxy_client_socket_pool.h:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
nit:  This year is wrong.  I'm not sure if the CQ will automatically fix them
(Which it supposedly normally does), if you change them from the right year to
the wrong one.

https://chromiumcodereview.appspot.com/9667016/diff/10005/net/socket/client_s...
File net/socket/client_socket_pool_base.cc (right):

https://chromiumcodereview.appspot.com/9667016/diff/10005/net/socket/client_s...
net/socket/client_socket_pool_base.cc:386: group = GetOrCreateGroup(group_name);
Think we should probably have a unit test for this.

https://chromiumcodereview.appspot.com/9667016/diff/10005/net/socket/client_s...
net/socket/client_socket_pool_base.cc:934: // If we are not using out
max_sockets_, then clearly we are not stalled
nit:  "using our |max_sockets_|" or just "using |max_sockets_|"

https://chromiumcodereview.appspot.com/9667016/diff/10005/net/socket/client_s...
net/socket/client_socket_pool_base.cc:937: // So in order to be stalled we need
to be using max_sockets_ AND
nit:  |max_sockets_|

https://chromiumcodereview.appspot.com/9667016/diff/10005/net/socket/client_s...
net/socket/client_socket_pool_base.cc:942: // max_sockets_per_group_, then the
request is stalled on the group,
nit:  |max_sockets_per_group_| both times.

https://chromiumcodereview.appspot.com/9667016/diff/10005/net/spdy/spdy_sessi...
File net/spdy/spdy_session.cc (right):

https://chromiumcodereview.appspot.com/9667016/diff/10005/net/spdy/spdy_sessi...
net/spdy/spdy_session.cc:1224: connection_->IsPoolStalled()) {
Think we should have a test for this, or even land it as part of a separate CL,
out of paranoia.  This is basically the behavior I suggested we give to other
socket pools.

[Ryan Hamilton, 2012-03-16 22:21:03]

https://chromiumcodereview.appspot.com/9667016/diff/10005/net/http/http_netwo...
File net/http/http_network_transaction_spdy21_unittest.cc (right):

https://chromiumcodereview.appspot.com/9667016/diff/10005/net/http/http_netwo...
net/http/http_network_transaction_spdy21_unittest.cc:167:
SpdySession::set_default_protocol(SSLClientSocket::kProtoSPDY21);
On 2012/03/16 18:01:15, Matt Menke wrote:
> nit:  Suggest you call this "CloseIdle..."

I'm not sure I understand what you are suggesting to be renamed?  Not
"SpdySession::set_default_protocol", right?

https://chromiumcodereview.appspot.com/9667016/diff/10005/net/http/http_proxy...
File net/http/http_proxy_client_socket_pool.h (right):

https://chromiumcodereview.appspot.com/9667016/diff/10005/net/http/http_proxy...
net/http/http_proxy_client_socket_pool.h:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
On 2012/03/16 18:01:15, Matt Menke wrote:
> nit:  This year is wrong.  I'm not sure if the CQ will automatically fix them
> (Which it supposedly normally does), if you change them from the right year to
> the wrong one.

Done.

https://chromiumcodereview.appspot.com/9667016/diff/10005/net/socket/client_s...
File net/socket/client_socket_pool_base.cc (right):

https://chromiumcodereview.appspot.com/9667016/diff/10005/net/socket/client_s...
net/socket/client_socket_pool_base.cc:386: group = GetOrCreateGroup(group_name);
On 2012/03/16 18:01:15, Matt Menke wrote:
> Think we should probably have a unit test for this.

Done.  Holy crap was it convoluted exercise the right codepath.

https://chromiumcodereview.appspot.com/9667016/diff/10005/net/socket/client_s...
net/socket/client_socket_pool_base.cc:934: // If we are not using out
max_sockets_, then clearly we are not stalled
On 2012/03/16 18:01:15, Matt Menke wrote:
> nit:  "using our |max_sockets_|" or just "using |max_sockets_|"

Done.

https://chromiumcodereview.appspot.com/9667016/diff/10005/net/socket/client_s...
net/socket/client_socket_pool_base.cc:937: // So in order to be stalled we need
to be using max_sockets_ AND
On 2012/03/16 18:01:15, Matt Menke wrote:
> nit:  |max_sockets_|

Done.

https://chromiumcodereview.appspot.com/9667016/diff/10005/net/socket/client_s...
net/socket/client_socket_pool_base.cc:942: // max_sockets_per_group_, then the
request is stalled on the group,
On 2012/03/16 18:01:15, Matt Menke wrote:
> nit:  |max_sockets_per_group_| both times.

Done.

https://chromiumcodereview.appspot.com/9667016/diff/10005/net/spdy/spdy_sessi...
File net/spdy/spdy_session.cc (right):

https://chromiumcodereview.appspot.com/9667016/diff/10005/net/spdy/spdy_sessi...
net/spdy/spdy_session.cc:1224: connection_->IsPoolStalled()) {
On 2012/03/16 18:01:15, Matt Menke wrote:
> Think we should have a test for this, or even land it as part of a separate
CL,
> out of paranoia.  This is basically the behavior I suggested we give to other
> socket pools.

Removed for now.

[mmenke, 2012-03-16 22:25:09]

https://chromiumcodereview.appspot.com/9667016/diff/10005/net/http/http_netwo...
File net/http/http_network_transaction_spdy21_unittest.cc (right):

https://chromiumcodereview.appspot.com/9667016/diff/10005/net/http/http_netwo...
net/http/http_network_transaction_spdy21_unittest.cc:167:
SpdySession::set_default_protocol(SSLClientSocket::kProtoSPDY21);
On 2012/03/16 22:21:03, Ryan Hamilton wrote:
> On 2012/03/16 18:01:15, Matt Menke wrote:
> > nit:  Suggest you call this "CloseIdle..."
> 
> I'm not sure I understand what you are suggesting to be renamed?  Not
> "SpdySession::set_default_protocol", right?

Errm...Oops...Moved the comment from a previous revision, though not sure how it
ended up here...

I meant rename "CloseOldSpdySessionToOpenNewOne" to
"CloseIdleSpdySessionToOpenNewOne"

[Ryan Hamilton, 2012-03-16 22:34:56]

https://chromiumcodereview.appspot.com/9667016/diff/10005/net/http/http_netwo...
File net/http/http_network_transaction_spdy21_unittest.cc (right):

https://chromiumcodereview.appspot.com/9667016/diff/10005/net/http/http_netwo...
net/http/http_network_transaction_spdy21_unittest.cc:167:
SpdySession::set_default_protocol(SSLClientSocket::kProtoSPDY21);
On 2012/03/16 22:25:09, Matt Menke wrote:
> On 2012/03/16 22:21:03, Ryan Hamilton wrote:
> > On 2012/03/16 18:01:15, Matt Menke wrote:
> > > nit:  Suggest you call this "CloseIdle..."
> > 
> > I'm not sure I understand what you are suggesting to be renamed?  Not
> > "SpdySession::set_default_protocol", right?
> 
> Errm...Oops...Moved the comment from a previous revision, though not sure how
it
> ended up here...
> 
> I meant rename "CloseOldSpdySessionToOpenNewOne" to
> "CloseIdleSpdySessionToOpenNewOne"

Good idea.  Done.

[mmenke, 2012-03-19 18:43:35]

Will:  Is there a reason we only delete empty groups in CleanupIdleSockets,
rather than in ReleaseSocket?  Just to keep things simple?

http://codereview.chromium.org/9667016/diff/21004/net/socket/client_socket_po...
File net/socket/client_socket_pool_base_unittest.cc (right):

http://codereview.chromium.org/9667016/diff/21004/net/socket/client_socket_po...
net/socket/client_socket_pool_base_unittest.cc:394: TestConnectJob::JobType
job_type = job_type_;
nit:  Maybe an EXPECT_TRUE(!job_types_ || !job_types_->empty());?

http://codereview.chromium.org/9667016/diff/21004/net/socket/client_socket_po...
net/socket/client_socket_pool_base_unittest.cc:397: job_types_->pop_back();
Think going front to back is much clearer.  Wasn't able to figure out how this
test worked until I realized the second failing job you push onto the vector is
for the third socket request, not the second.

http://codereview.chromium.org/9667016/diff/21004/net/socket/client_socket_po...
net/socket/client_socket_pool_base_unittest.cc:3471: .WillOnce(Return(false));
nit:  To make this test more closely match the next one, suggest moving the
EXPECT_CALL down one line.

http://codereview.chromium.org/9667016/diff/21004/net/socket/client_socket_po...
net/socket/client_socket_pool_base_unittest.cc:3506: BoundNetLog()));
nit:  Fix indent.

http://codereview.chromium.org/9667016/diff/21004/net/socket/client_socket_po...
net/socket/client_socket_pool_base_unittest.cc:3512:
&MockLayeredPool::ReleaseOneConnection));
nit:  -1 indent.

http://codereview.chromium.org/9667016/diff/21004/net/socket/client_socket_po...
net/socket/client_socket_pool_base_unittest.cc:3515: ClientSocketHandle handle2;
Suggest you name these handle3 and handle4, so they match the |job_types|
vector.

http://codereview.chromium.org/9667016/diff/21004/net/socket/client_socket_po...
net/socket/client_socket_pool_base_unittest.cc:3515: ClientSocketHandle handle2;
Suggest you add a comment here saying that this connection attempt will fail
when the next request causes the MockLayeredPool to delete the socket it's
holding.  And that this request is needed to trigger the destruction of the
"group2" Group.

Does seem a little weird that we're not deleting empty groups instantly.

http://codereview.chromium.org/9667016/diff/21004/net/socket/client_socket_po...
net/socket/client_socket_pool_base_unittest.cc:3519: HIGHEST,
I don't think this or the next request has to be at HIGHEST priority to trigger
the bug.

http://codereview.chromium.org/9667016/diff/21004/net/socket/client_socket_po...
net/socket/client_socket_pool_base_unittest.cc:3532: BoundNetLog()));
nit:  Fix indent.

[Ryan Hamilton, 2012-03-19 19:04:15]

http://codereview.chromium.org/9667016/diff/21004/net/socket/client_socket_po...
File net/socket/client_socket_pool_base_unittest.cc (right):

http://codereview.chromium.org/9667016/diff/21004/net/socket/client_socket_po...
net/socket/client_socket_pool_base_unittest.cc:394: TestConnectJob::JobType
job_type = job_type_;
On 2012/03/19 18:43:35, Matt Menke wrote:
> nit:  Maybe an EXPECT_TRUE(!job_types_ || !job_types_->empty());?

Done.

http://codereview.chromium.org/9667016/diff/21004/net/socket/client_socket_po...
net/socket/client_socket_pool_base_unittest.cc:397: job_types_->pop_back();
On 2012/03/19 18:43:35, Matt Menke wrote:
> Think going front to back is much clearer.  Wasn't able to figure out how this
> test worked until I realized the second failing job you push onto the vector
is
> for the third socket request, not the second.

Done.

http://codereview.chromium.org/9667016/diff/21004/net/socket/client_socket_po...
net/socket/client_socket_pool_base_unittest.cc:3471: .WillOnce(Return(false));
On 2012/03/19 18:43:35, Matt Menke wrote:
> nit:  To make this test more closely match the next one, suggest moving the
> EXPECT_CALL down one line.

Done.

http://codereview.chromium.org/9667016/diff/21004/net/socket/client_socket_po...
net/socket/client_socket_pool_base_unittest.cc:3506: BoundNetLog()));
On 2012/03/19 18:43:35, Matt Menke wrote:
> nit:  Fix indent.

Done.

http://codereview.chromium.org/9667016/diff/21004/net/socket/client_socket_po...
net/socket/client_socket_pool_base_unittest.cc:3512:
&MockLayeredPool::ReleaseOneConnection));
On 2012/03/19 18:43:35, Matt Menke wrote:
> nit:  -1 indent.

Done.

http://codereview.chromium.org/9667016/diff/21004/net/socket/client_socket_po...
net/socket/client_socket_pool_base_unittest.cc:3515: ClientSocketHandle handle2;
On 2012/03/19 18:43:35, Matt Menke wrote:
> Suggest you name these handle3 and handle4, so they match the |job_types|
> vector.

Done.  Good idea.

http://codereview.chromium.org/9667016/diff/21004/net/socket/client_socket_po...
net/socket/client_socket_pool_base_unittest.cc:3515: ClientSocketHandle handle2;
On 2012/03/19 18:43:35, Matt Menke wrote:
> Suggest you add a comment here saying that this connection attempt will fail
> when the next request causes the MockLayeredPool to delete the socket it's
> holding.  And that this request is needed to trigger the destruction of the
> "group2" Group.
> 
> Does seem a little weird that we're not deleting empty groups instantly.

Done.

http://codereview.chromium.org/9667016/diff/21004/net/socket/client_socket_po...
net/socket/client_socket_pool_base_unittest.cc:3519: HIGHEST,
On 2012/03/19 18:43:35, Matt Menke wrote:
> I don't think this or the next request has to be at HIGHEST priority to
trigger
> the bug.

Done.  Yes, you're right.  This was a leftover from an earlier attempt to
trigger the bug.

http://codereview.chromium.org/9667016/diff/21004/net/socket/client_socket_po...
net/socket/client_socket_pool_base_unittest.cc:3532: BoundNetLog()));
On 2012/03/19 18:43:35, Matt Menke wrote:
> nit:  Fix indent.

Done.

[willchan no longer on Chromium, 2012-03-19 19:16:30]

On 2012/03/19 18:43:35, Matt Menke wrote:
> Will:  Is there a reason we only delete empty groups in CleanupIdleSockets,
> rather than in ReleaseSocket?  Just to keep things simple?

Looks like a bug.

> 
>
http://codereview.chromium.org/9667016/diff/21004/net/socket/client_socket_po...
> File net/socket/client_socket_pool_base_unittest.cc (right):
> 
>
http://codereview.chromium.org/9667016/diff/21004/net/socket/client_socket_po...
> net/socket/client_socket_pool_base_unittest.cc:394: TestConnectJob::JobType
> job_type = job_type_;
> nit:  Maybe an EXPECT_TRUE(!job_types_ || !job_types_->empty());?
> 
>
http://codereview.chromium.org/9667016/diff/21004/net/socket/client_socket_po...
> net/socket/client_socket_pool_base_unittest.cc:397: job_types_->pop_back();
> Think going front to back is much clearer.  Wasn't able to figure out how this
> test worked until I realized the second failing job you push onto the vector
is
> for the third socket request, not the second.
> 
>
http://codereview.chromium.org/9667016/diff/21004/net/socket/client_socket_po...
> net/socket/client_socket_pool_base_unittest.cc:3471: .WillOnce(Return(false));
> nit:  To make this test more closely match the next one, suggest moving the
> EXPECT_CALL down one line.
> 
>
http://codereview.chromium.org/9667016/diff/21004/net/socket/client_socket_po...
> net/socket/client_socket_pool_base_unittest.cc:3506: BoundNetLog()));
> nit:  Fix indent.
> 
>
http://codereview.chromium.org/9667016/diff/21004/net/socket/client_socket_po...
> net/socket/client_socket_pool_base_unittest.cc:3512:
> &MockLayeredPool::ReleaseOneConnection));
> nit:  -1 indent.
> 
>
http://codereview.chromium.org/9667016/diff/21004/net/socket/client_socket_po...
> net/socket/client_socket_pool_base_unittest.cc:3515: ClientSocketHandle
handle2;
> Suggest you name these handle3 and handle4, so they match the |job_types|
> vector.
> 
>
http://codereview.chromium.org/9667016/diff/21004/net/socket/client_socket_po...
> net/socket/client_socket_pool_base_unittest.cc:3515: ClientSocketHandle
handle2;
> Suggest you add a comment here saying that this connection attempt will fail
> when the next request causes the MockLayeredPool to delete the socket it's
> holding.  And that this request is needed to trigger the destruction of the
> "group2" Group.
> 
> Does seem a little weird that we're not deleting empty groups instantly.
> 
>
http://codereview.chromium.org/9667016/diff/21004/net/socket/client_socket_po...
> net/socket/client_socket_pool_base_unittest.cc:3519: HIGHEST,
> I don't think this or the next request has to be at HIGHEST priority to
trigger
> the bug.
> 
>
http://codereview.chromium.org/9667016/diff/21004/net/socket/client_socket_po...
> net/socket/client_socket_pool_base_unittest.cc:3532: BoundNetLog()));
> nit:  Fix indent.

[willchan no longer on Chromium, 2012-03-19 19:16:53]

On 2012/03/19 19:16:30, willchan wrote:
> On 2012/03/19 18:43:35, Matt Menke wrote:
> > Will:  Is there a reason we only delete empty groups in CleanupIdleSockets,
> > rather than in ReleaseSocket?  Just to keep things simple?
> 
> Looks like a bug.

Be careful when fixing it though, the logic is quite subtle there.

> 
> > 
> >
>
http://codereview.chromium.org/9667016/diff/21004/net/socket/client_socket_po...
> > File net/socket/client_socket_pool_base_unittest.cc (right):
> > 
> >
>
http://codereview.chromium.org/9667016/diff/21004/net/socket/client_socket_po...
> > net/socket/client_socket_pool_base_unittest.cc:394: TestConnectJob::JobType
> > job_type = job_type_;
> > nit:  Maybe an EXPECT_TRUE(!job_types_ || !job_types_->empty());?
> > 
> >
>
http://codereview.chromium.org/9667016/diff/21004/net/socket/client_socket_po...
> > net/socket/client_socket_pool_base_unittest.cc:397: job_types_->pop_back();
> > Think going front to back is much clearer.  Wasn't able to figure out how
this
> > test worked until I realized the second failing job you push onto the vector
> is
> > for the third socket request, not the second.
> > 
> >
>
http://codereview.chromium.org/9667016/diff/21004/net/socket/client_socket_po...
> > net/socket/client_socket_pool_base_unittest.cc:3471:
.WillOnce(Return(false));
> > nit:  To make this test more closely match the next one, suggest moving the
> > EXPECT_CALL down one line.
> > 
> >
>
http://codereview.chromium.org/9667016/diff/21004/net/socket/client_socket_po...
> > net/socket/client_socket_pool_base_unittest.cc:3506: BoundNetLog()));
> > nit:  Fix indent.
> > 
> >
>
http://codereview.chromium.org/9667016/diff/21004/net/socket/client_socket_po...
> > net/socket/client_socket_pool_base_unittest.cc:3512:
> > &MockLayeredPool::ReleaseOneConnection));
> > nit:  -1 indent.
> > 
> >
>
http://codereview.chromium.org/9667016/diff/21004/net/socket/client_socket_po...
> > net/socket/client_socket_pool_base_unittest.cc:3515: ClientSocketHandle
> handle2;
> > Suggest you name these handle3 and handle4, so they match the |job_types|
> > vector.
> > 
> >
>
http://codereview.chromium.org/9667016/diff/21004/net/socket/client_socket_po...
> > net/socket/client_socket_pool_base_unittest.cc:3515: ClientSocketHandle
> handle2;
> > Suggest you add a comment here saying that this connection attempt will fail
> > when the next request causes the MockLayeredPool to delete the socket it's
> > holding.  And that this request is needed to trigger the destruction of the
> > "group2" Group.
> > 
> > Does seem a little weird that we're not deleting empty groups instantly.
> > 
> >
>
http://codereview.chromium.org/9667016/diff/21004/net/socket/client_socket_po...
> > net/socket/client_socket_pool_base_unittest.cc:3519: HIGHEST,
> > I don't think this or the next request has to be at HIGHEST priority to
> trigger
> > the bug.
> > 
> >
>
http://codereview.chromium.org/9667016/diff/21004/net/socket/client_socket_po...
> > net/socket/client_socket_pool_base_unittest.cc:3532: BoundNetLog()));
> > nit:  Fix indent.

[Ryan Hamilton, 2012-03-19 19:18:12]

On 2012/03/19 19:16:53, willchan wrote:
> On 2012/03/19 19:16:30, willchan wrote:
> > On 2012/03/19 18:43:35, Matt Menke wrote:
> > > Will:  Is there a reason we only delete empty groups in
CleanupIdleSockets,
> > > rather than in ReleaseSocket?  Just to keep things simple?
> > 
> > Looks like a bug.
> 
> Be careful when fixing it though, the logic is quite subtle there.

Should I fix it as part of this CL (which is rather complicated enough, I think)
or in a later one?

[willchan no longer on Chromium, 2012-03-19 19:18:50]

On Mon, Mar 19, 2012 at 12:18 PM, <rch@chromium.org> wrote:

> On 2012/03/19 19:16:53, willchan wrote:
>
>> On 2012/03/19 19:16:30, willchan wrote:
>> > On 2012/03/19 18:43:35, Matt Menke wrote:
>> > > Will:  Is there a reason we only delete empty groups in
>>
> CleanupIdleSockets,
>
>> > > rather than in ReleaseSocket?  Just to keep things simple?
>> >
>> > Looks like a bug.
>>
>
>  Be careful when fixing it though, the logic is quite subtle there.
>>
>
> Should I fix it as part of this CL (which is rather complicated enough, I
> think)
> or in a later one?
>

I'd vote a later one. You're asking for trouble cleaning up that code in
this changelist.


>
>
http://codereview.chromium.**org/9667016/<http://codereview.chromium.org/9667...
>

[mmenke, 2012-03-19 19:21:00]

On 2012/03/19 19:18:50, willchan wrote:
> I'd vote a later one. You're asking for trouble cleaning up that code in
> this changelist.

I strongly agree with Will.  It would make that one test a little simpler, but
this CL is complicated enough as is.

[Ryan Hamilton, 2012-03-19 19:25:29]

On 2012/03/19 19:21:00, Matt Menke wrote:
> On 2012/03/19 19:18:50, willchan wrote:
> > I'd vote a later one. You're asking for trouble cleaning up that code in
> > this changelist.
> 
> I strongly agree with Will.  It would make that one test a little simpler, but
> this CL is complicated enough as is.

Excellent.  That was preference as well.

[mmenke, 2012-03-19 19:29:28]

Everything looks fine to me, I just want to convince myself
CloseOneIdleConnection() is correct before I signed off on this.  Hope to get
back to you tonight or tomorrow morning.

http://codereview.chromium.org/9667016/diff/26002/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

http://codereview.chromium.org/9667016/diff/26002/net/spdy/spdy_session.cc#ne...
net/spdy/spdy_session.cc:1185: // Since the underlying socket it only returned
when |this| is destroyed
nit:  it->is

[Ryan Hamilton, 2012-03-19 22:34:26]

http://codereview.chromium.org/9667016/diff/26002/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

http://codereview.chromium.org/9667016/diff/26002/net/spdy/spdy_session.cc#ne...
net/spdy/spdy_session.cc:1185: // Since the underlying socket it only returned
when |this| is destroyed
On 2012/03/19 19:29:28, Matt Menke wrote:
> nit:  it->is

Done.

[mmenke, 2012-03-20 14:30:54]

LGTM.

[commit-bot: I haz the power, 2012-03-20 16:18:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/9667016/28009

[commit-bot: I haz the power, 2012-03-20 17:37:31]

Change committed as 127717