Control the amount of time to leave an unused socket idle before closing it.

Control the amount of time to leave an unused socket idle before closing it.
This adds constructor arguments for socket idle timeouts.  This allows me to control it for testing, and also makes it possible to run experiments on how long to enable it for.
Currently I've set the timeout for unused sockets to 10 seconds, since that will cover 90% of the TCP RSTs we're seeing.  We can probably increase this, but I'm waiting on histogram data to decide what to change it to.
BUG=http://crbug.com/18192

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

[willchan no longer on Chromium, 2009-08-28 19:29:43]

Note that the timeout granularity is accurate to within 10 seconds or so, since
kCleanupInterval is set to 10 seconds.  It used to be 5, but apparently
wtc/eroman upped it to 10 for some reason, so I didn't bother changing this to
improve the granularity.  That means the real timeout for unused idle sockets
will be between 10-20 seconds.

[wtc, 2009-08-29 00:09:03]

LGTM.

My gut feeling is that 10 seconds are right for
unused idle socket timeout, and 300 seconds (5 minutes)
are too long for used idle socket timeout.  I remember
IE uses 60 second timeout for that.

http://codereview.chromium.org/176021/diff/1/2
File net/socket/client_socket_pool_base.cc (right):

http://codereview.chromium.org/176021/diff/1/2#newcode344
Line 344: base::TimeTicks now, base::TimeDelta timeout) const {
Nit: the style guide says we're supposed to list one
parameter per line when this form (the 3rd form) is used:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

http://codereview.chromium.org/176021/diff/1/3
File net/socket/client_socket_pool_base.h (right):

http://codereview.chromium.org/176021/diff/1/3#newcode381
Line 381: // The time to wait until closing idle sockets.
Nit: put these two new members right after the max_sockets_
and max_sockets_per_group_ members because they all come from
constructor arguments.

http://codereview.chromium.org/176021/diff/1/3#newcode398
Line 398: static const int kUnusedIdleSocketTimeout = 10;
Nit: add a blank line below.

It is uncommon to declare static constants in header
files like this.  Should they be moved into some class
or become enums?

http://codereview.chromium.org/176021/diff/1/3#newcode400
Line 400: static const int kUsedIdleSocketTimeout = 300;  // 5 minutes
Is this our current used idle socket timeout?  It's
much longer than I thought.

http://codereview.chromium.org/176021/diff/1/4
File net/socket/client_socket_pool_base_unittest.cc (right):

http://codereview.chromium.org/176021/diff/1/4#newcode304
Line 304: void CleanupIdleSockets() { base_.CleanupIdleSockets(false); }
Just curious -- why doesn't this method take a 'force'
parameter?

Perhaps rename it CleanupTimedoutIdleSockets if you
don't want a parameter?

http://codereview.chromium.org/176021/diff/1/4#newcode375
Line 375: int max_sockets, int max_sockets_per_group,
Nit: one parameter per line in this form:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

http://codereview.chromium.org/176021/diff/1/4#newcode1784
Line 1784: base::TimeDelta(),  // Timeout unused sockets immediately.
Nit: Timeout => Time out (two words)

Same nit for the "timeout" in next line

[willchan no longer on Chromium, 2009-08-29 01:14:28]

Try bots are running again.  Will commit if all looks good.

http://codereview.chromium.org/176021/diff/1/2
File net/socket/client_socket_pool_base.cc (right):

http://codereview.chromium.org/176021/diff/1/2#newcode344
Line 344: base::TimeTicks now, base::TimeDelta timeout) const {
On 2009/08/29 00:09:03, wtc wrote:
> Nit: the style guide says we're supposed to list one
> parameter per line when this form (the 3rd form) is used:
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

Done.

http://codereview.chromium.org/176021/diff/1/3
File net/socket/client_socket_pool_base.h (right):

http://codereview.chromium.org/176021/diff/1/3#newcode381
Line 381: // The time to wait until closing idle sockets.
On 2009/08/29 00:09:03, wtc wrote:
> Nit: put these two new members right after the max_sockets_
> and max_sockets_per_group_ members because they all come from
> constructor arguments.

Done.

http://codereview.chromium.org/176021/diff/1/3#newcode398
Line 398: static const int kUnusedIdleSocketTimeout = 10;
On 2009/08/29 00:09:03, wtc wrote:
> Nit: add a blank line below.
> 
> It is uncommon to declare static constants in header
> files like this.  Should they be moved into some class
> or become enums?

I wanted to make them part of a class, but the public class here is a template,
so it would require multiple copies.  I think the static const int is fine,
because the compiler will inline the constant, since no one is ever taking the
address of the constant, so it's the same as an enum, and is more obviously a
constant since it has the const qualifier.

http://codereview.chromium.org/176021/diff/1/3#newcode400
Line 400: static const int kUsedIdleSocketTimeout = 300;  // 5 minutes
On 2009/08/29 00:09:03, wtc wrote:
> Is this our current used idle socket timeout?  It's
> much longer than I thought.

Yep, it is.  I'll have histogram data later to see how effective this is.

http://codereview.chromium.org/176021/diff/1/4
File net/socket/client_socket_pool_base_unittest.cc (right):

http://codereview.chromium.org/176021/diff/1/4#newcode304
Line 304: void CleanupIdleSockets() { base_.CleanupIdleSockets(false); }
On 2009/08/29 00:09:03, wtc wrote:
> Just curious -- why doesn't this method take a 'force'
> parameter?
> 
> Perhaps rename it CleanupTimedoutIdleSockets if you
> don't want a parameter?

Renamed.

http://codereview.chromium.org/176021/diff/1/4#newcode375
Line 375: int max_sockets, int max_sockets_per_group,
On 2009/08/29 00:09:03, wtc wrote:
> Nit: one parameter per line in this form:
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

Done.

http://codereview.chromium.org/176021/diff/1/4#newcode1784
Line 1784: base::TimeDelta(),  // Timeout unused sockets immediately.
On 2009/08/29 00:09:03, wtc wrote:
> Nit: Timeout => Time out (two words)
> 
> Same nit for the "timeout" in next line

Done.