Add timeouts for ConnectJobs. Limit ConnectJobs per group to number of Requests per group + 1.

Add timeouts for ConnectJobs.  Limit ConnectJobs per group to number of Requests per group + 1.
In the histograms for Net.SocketRequestTime, late binding has a much longer tail.  This is presumably because I didn't implement timeouts and CancelRequest() never cancelled jobs.  I'm limiting TCPConnectJobs to 60 seconds and cancelling jobs if there are too many more than there are requests.

[willchan no longer on Chromium, 2009-08-01 08:03:17]

This is still missing tests for timeouts.  I added a test for the change in
behavior of CancelRequest().  I'll add tests for timeouts later.

[willchan no longer on Chromium, 2009-08-02 04:27:39]

On 2009/08/01 08:03:17, willchan wrote:
> This is still missing tests for timeouts.  I added a test for the change in
> behavior of CancelRequest().  I'll add tests for timeouts later.

[willchan no longer on Chromium, 2009-08-02 04:27:50]

On 2009/08/02 04:27:39, willchan wrote:
> On 2009/08/01 08:03:17, willchan wrote:
> > This is still missing tests for timeouts.  I added a test for the change in
> > behavior of CancelRequest().  I'll add tests for timeouts later.

The tests are added now.

[eroman, 2009-08-02 11:09:30]

General question: What is the rationale behind "30 seconds"?

...It seems fairly reasonable.

I guess 20 seconds is the maximum after which winsock times-out tcp connects.
(Leaving 10 seconds extra for DNS).

[eroman, 2009-08-02 11:11:42]

(I will dive into this review tomorrow or monday).

[eroman, 2009-08-03 20:03:24]

http://codereview.chromium.org/160499/diff/9/10
File net/socket/client_socket_pool_base.cc (right):

http://codereview.chromium.org/160499/diff/9/10#newcode59
Line 59: delegate()->OnConnectJobComplete(ERR_TIMED_OUT, this);
Can you NULL delegate before calling this?

That way if the delegate does NOT delete us (which is relied on here to cancel
the requests), we will crash in trying to notify the delegate of a later
completion.

Also, I think using a different error code would be good. Since now if we get
bug reports of ERR_TIMED_OUT, we don't know if it was a timeout doing TCP
connect or doing host resolve (ERR_TIMED_OUT only mapped to TCP connect before
if i recall).

http://codereview.chromium.org/160499/diff/9/10#newcode182
Line 182: RemoveConnectJob(handle, *group.jobs.begin(), &group);
So i guess this just cancels a random job, because |jobs| is a set?

http://codereview.chromium.org/160499/diff/9/11
File net/socket/client_socket_pool_base.h (right):

http://codereview.chromium.org/160499/diff/9/11#newcode46
Line 46: // Pass in a default constructed base::TimeDelta for |timeout_duration|
to
maybe be more explicit here and say that timeout_duration of 0 corresponds with
"no timeout".

http://codereview.chromium.org/160499/diff/9/11#newcode87
Line 87: // Timer to time out lengthy connections
net: end in period. Might reword this to something like: "Timer to abort jobs
that take too long".

http://codereview.chromium.org/160499/diff/9/12
File net/socket/client_socket_pool_base_unittest.cc (right):

http://codereview.chromium.org/160499/diff/9/12#newcode316
Line 316: int WaitForResult() {
Too bad ConnectJob is using delegate rather than a CompletionCallback....

If ConnectJobConnect() took a completion callback this would simply be a matter
of using TestCompletionCallback.

http://codereview.chromium.org/160499/diff/9/12#newcode318
Line 318: while (!have_result_) {
Isn't this an uninitialized memory read error?

http://codereview.chromium.org/160499/diff/9/12#newcode368
Line 368: TEST(ConnectJobTest, NoTimeout) {
Can you mention this doesn't timeout because it completes synchronously? (could
still take longer than 1 microsecond).

http://codereview.chromium.org/160499/diff/9/12#newcode386
Line 386: base::TimeDelta::FromMicroseconds(1),
Is this a race? (We assume it takes longer than 1 microseconds).

How about putting a tiny sleep in the job to guarantee that if  aliens from
outer space run this on their super computers it still passes.

http://codereview.chromium.org/160499/diff/9/13
File net/socket/tcp_client_socket_pool.cc (right):

http://codereview.chromium.org/160499/diff/9/13#newcode22
Line 22: // time, including both DNS and TCP connect() times.
minor-nit: Instead of "DNS" may want to use the more general "host resolves"
term (since the source of the name lookup could come from other than DNS).

http://codereview.chromium.org/160499/diff/9/13#newcode23
Line 23: static const int kTCPConnectJobTimeoutInSeconds = 30;
How was 30 seconds arrived at... could it cause problems for people with very
slow dependencies? What are the tail end of DNS resolve times?

[willchan no longer on Chromium, 2009-08-03 20:45:46]

http://codereview.chromium.org/160499/diff/9/10
File net/socket/client_socket_pool_base.cc (right):

http://codereview.chromium.org/160499/diff/9/10#newcode59
Line 59: delegate()->OnConnectJobComplete(ERR_TIMED_OUT, this);
On 2009/08/03 20:03:24, eroman wrote:
> Can you NULL delegate before calling this?
> 
> That way if the delegate does NOT delete us (which is relied on here to cancel
> the requests), we will crash in trying to notify the delegate of a later
> completion.
> 
> Also, I think using a different error code would be good. Since now if we get
> bug reports of ERR_TIMED_OUT, we don't know if it was a timeout doing TCP
> connect or doing host resolve (ERR_TIMED_OUT only mapped to TCP connect before
> if i recall).

Done.

http://codereview.chromium.org/160499/diff/9/10#newcode182
Line 182: RemoveConnectJob(handle, *group.jobs.begin(), &group);
On 2009/08/03 20:03:24, eroman wrote:
> So i guess this just cancels a random job, because |jobs| is a set?

Yep, I left a TODO to choose a job at the lowest LoadState.  But within a
LoadState, I won't bother distinguishing (at least for now).

http://codereview.chromium.org/160499/diff/9/11
File net/socket/client_socket_pool_base.h (right):

http://codereview.chromium.org/160499/diff/9/11#newcode46
Line 46: // Pass in a default constructed base::TimeDelta for |timeout_duration|
to
On 2009/08/03 20:03:24, eroman wrote:
> maybe be more explicit here and say that timeout_duration of 0 corresponds
with
> "no timeout".

Done.

http://codereview.chromium.org/160499/diff/9/11#newcode87
Line 87: // Timer to time out lengthy connections
On 2009/08/03 20:03:24, eroman wrote:
> net: end in period. Might reword this to something like: "Timer to abort jobs
> that take too long".

Done.

http://codereview.chromium.org/160499/diff/9/12
File net/socket/client_socket_pool_base_unittest.cc (right):

http://codereview.chromium.org/160499/diff/9/12#newcode318
Line 318: while (!have_result_) {
On 2009/08/03 20:03:24, eroman wrote:
> Isn't this an uninitialized memory read error?

Yeah...I'm just getting lucky that the memory is zero.  I've initialized it in
the constructor now.

http://codereview.chromium.org/160499/diff/9/12#newcode368
Line 368: TEST(ConnectJobTest, NoTimeout) {
On 2009/08/03 20:03:24, eroman wrote:
> Can you mention this doesn't timeout because it completes synchronously?
(could
> still take longer than 1 microsecond).

Done.

http://codereview.chromium.org/160499/diff/9/12#newcode386
Line 386: base::TimeDelta::FromMicroseconds(1),
On 2009/08/03 20:03:24, eroman wrote:
> Is this a race? (We assume it takes longer than 1 microseconds).
> 
> How about putting a tiny sleep in the job to guarantee that if  aliens from
> outer space run this on their super computers it still passes.

Done.

http://codereview.chromium.org/160499/diff/9/13
File net/socket/tcp_client_socket_pool.cc (right):

http://codereview.chromium.org/160499/diff/9/13#newcode22
Line 22: // time, including both DNS and TCP connect() times.
On 2009/08/03 20:03:24, eroman wrote:
> minor-nit: Instead of "DNS" may want to use the more general "host resolves"
> term (since the source of the name lookup could come from other than DNS).

Done.

http://codereview.chromium.org/160499/diff/9/13#newcode23
Line 23: static const int kTCPConnectJobTimeoutInSeconds = 30;
On 2009/08/03 20:03:24, eroman wrote:
> How was 30 seconds arrived at... could it cause problems for people with very
> slow dependencies? What are the tail end of DNS resolve times?

I figured 30 seconds was pretty long to wait already.  Perhaps I'm too
aggressive?  I guess I thought I was being conservative, but perhaps there are
some people for whom this could be normal?

According to my histogram data 99.99% of (uncancelled) TCPConnectJobs finish
within 30 seconds.  Roughly 750 / 8 594 644 = .0872636493% of TCPConnectJobs
(DNS + TCP connect()) do not complete in time.  I've bumped this up to 60
seconds now, which according to the histograms should cover all except 80 /
8,594,644 = .00930812259% of TCPConnectJobs.

[eroman, 2009-08-03 22:01:14]

lgtm

[willchan no longer on Chromium, 2009-08-03 22:57:52]

I've fixed the tests to run properly.  I put the ConnectJobTests in the
ClientSocketPoolBaseTest fixture because I needed some members from it.

[eroman, 2009-08-03 23:02:11]

lgtm.