Add a new net::Error value: ERR_CONNECTION_TIMED_OUT.

Add a new net::Error value: ERR_CONNECTION_TIMED_OUT.
TEST=net_unittests
BUG=none

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

[tony, 2009-08-05 17:33:07]

We use ERR_TIMED_OUT in browser code to tell link doctor (error pages) that the
connection timed out.  Do we need to update those to ERR_TCP_CONNECT_TIMED_OUT? 
See chrome/browser/renderer/render_view.cc and
chrome/browser/renderer/localized_error.cc.

[wtc, 2009-08-05 17:50:01]

http://codereview.chromium.org/159904/diff/1/2
File net/base/net_error_list.h (right):

http://codereview.chromium.org/159904/diff/1/2#newcode113
Line 113: NET_ERROR(TCP_CONNECT_TIMED_OUT, -118)
We use ERR_TIMED_OUT for the error "connect() timed out".
ERR_TIMED_OUT is equivalent to the errno ETIMEDOUT.  See
http://www.opengroup.org/onlinepubs/000095399/functions/connect.html

Why do we need a new error code?

If we need a context-specific timeout error code for
connect(), the error code's name and the error message
should be similar to the related error codes -100 ~ -104,
something like:

  // A connection attempt timed out.
  NET_ERROR(CONNECTION_TIMED_OUT, -118)

[ericroman, 2009-08-05 18:11:59]

Splitting the error codes was a review comment I made on 160499.

Basically, I wanted to be able to distinguish (via error code) whether it
was the tcp connect that timed out, or the host resolve.

When only tcp connects were using that error code, it was unambiguous, but
now it is uncertain.

On Aug 5, 2009 10:50 AM, <wtc@chromium.org> wrote:


http://codereview.chromium.org/159904/diff/1/2
File net/base/net_error_list.h (right):

http://codereview.chromium.org/159904/diff/1/2#newcode113
Line 113: NET_ERROR(TCP_CONNECT_TIMED_OUT, -118)
We use ERR_TIMED_OUT for the error "connect() timed out".
ERR_TIMED_OUT is equivalent to the errno ETIMEDOUT.  See
http://www.opengroup.org/onlinepubs/000095399/functions/connect.html

Why do we need a new error code?

If we need a context-specific timeout error code for
connect(), the error code's name and the error message
should be similar to the related error codes -100 ~ -104,
something like:

 // A connection attempt timed out.
 NET_ERROR(CONNECTION_TIMED_OUT, -118)

http://codereview.chromium.org/159904

[ericroman, 2009-08-05 18:16:04]

Perhaps we could add some net::IsTimeoutError(int) helper to identify these
timeout types...

On Aug 5, 2009 11:11 AM, "Eric Roman" <ericroman@google.com> wrote:

Splitting the error codes was a review comment I made on 160499.

Basically, I wanted to be able to distinguish (via error code) whether it
was the tcp connect that timed out, or the host resolve.

When only tcp connects were using that error code, it was unambiguous, but
now it is uncertain.

> > On Aug 5, 2009 10:50 AM, <wtc@chromium.org> wrote: > > >
http://codereview.chromium.org/159904/...

[willchan no longer on Chromium, 2009-08-06 03:54:11]

Thanks Tony for pointing out the other cases.  I've updated the code to check
for the new error code.

http://codereview.chromium.org/159904/diff/1/2
File net/base/net_error_list.h (right):

http://codereview.chromium.org/159904/diff/1/2#newcode113
Line 113: NET_ERROR(TCP_CONNECT_TIMED_OUT, -118)
On 2009/08/05 17:50:02, wtc wrote:
> We use ERR_TIMED_OUT for the error "connect() timed out".
> ERR_TIMED_OUT is equivalent to the errno ETIMEDOUT.  See
> http://www.opengroup.org/onlinepubs/000095399/functions/connect.html
> 
> Why do we need a new error code?

Eric explained it.  Currently we use it for SSL and TCP connects.  I also made a
change for ClientSocketPool where if a ConnectJob (whatever the ConnectJob is)
takes too long, then the ClientSocketPool will time it out.  So it's somewhat
more ambiguous now.

> 
> If we need a context-specific timeout error code for
> connect(), the error code's name and the error message
> should be similar to the related error codes -100 ~ -104,
> something like:
> 
>   // A connection attempt timed out.
>   NET_ERROR(CONNECTION_TIMED_OUT, -118)

I've changed it to this now.  I had reservations about using CONNECTION instead
of CONNECT since I am worried that someone might interpret it as the connection
itself timing out (for a reason like ERR_CONNECTION_ABORTED on not receiving an
ACK).  I've changed it anyhow to your suggestion.  Hopefully people won't be
confused.

[eroman, 2009-08-06 04:08:01]

lgtm

[wtc, 2009-08-06 18:04:02]

LGTM.  See my comments below... I have some suggested
changes before you check this in.

http://codereview.chromium.org/159904/diff/1/2
File net/base/net_error_list.h (right):

http://codereview.chromium.org/159904/diff/1/2#newcode113
Line 113: NET_ERROR(TCP_CONNECT_TIMED_OUT, -118)
On 2009/08/06 03:54:11, willchan wrote:
>
> I had reservations about using CONNECTION instead
> of CONNECT since I am worried that someone might interpret it as the
connection
> itself timing out (for a reason like ERR_CONNECTION_ABORTED on not receiving
an
> ACK).  I've changed it anyhow to your suggestion.  Hopefully people won't be
> confused.

I noticed this issue, too.  In the end I decided to be
consistent with the existing ERR_CONNECTION_REFUSED and
ERR_CONNECTION_FAILED error codes, and address the
confusion by using "connection attempt" in the error
message.

http://codereview.chromium.org/159904/diff/3001/3003
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/159904/diff/3001/3003#newcode2924
Line 2924: // connection failure.  ERR_CONNECTION_FAILED can be dropped once we
no
Hmm... are we still using ERR_CONNECTION_FAILED?

If not, we can consider removing that error code (but leave
its error code value reserved for now).

http://codereview.chromium.org/159904/diff/3001/3003#newcode2932
Line 2932: ec != net::ERR_CONNECTION_TIMED)
You have a typo here :-)

Also, I think we should replace ERR_TIMED_OUT with
ERR_CONNECTION_TIMED_OUT here, rather than adding
ERR_CONNECTION_TIMED_OUT.  Note that all the error
codes here are connection setup errors (except
ERR_NAME_NOT_RESOLVED).

http://codereview.chromium.org/159904/diff/3001/3005
File net/socket/tcp_client_socket_libevent.cc (right):

http://codereview.chromium.org/159904/diff/3001/3005#newcode49
Line 49: return ERR_CONNECTION_TIMED_OUT;
Rather than changing MapPosixError, it woule be better to
add a MapConnectError function that we call after
connect() failed, and define MapConnectError like this:

  int MapConnectError(int err) {
    switch (err) {
      case ETIMEDOUT:
        return ERR_CONNECTION_TIMED_OUT;
      default:
        return MapPosixError(err);
    }
  }

http://codereview.chromium.org/159904/diff/3001/3006
File net/socket/tcp_client_socket_win.cc (right):

http://codereview.chromium.org/159904/diff/3001/3006#newcode49
Line 49: return ERR_CONNECTION_TIMED_OUT;
See my comment about MapPosixError above.

[willchan no longer on Chromium, 2009-08-06 18:47:50]

I've addressed all the comments.  Please take another look to make sure I didn't
miss anything.

[wtc, 2009-08-06 21:00:53]

Will, you haven't uploaded a new patch set :-)

[willchan no longer on Chromium, 2009-08-06 21:29:38]

Woops, it's uploaded now :)

On Thu, Aug 6, 2009 at 2:00 PM, <wtc@chromium.org> wrote:
> Will, you haven't uploaded a new patch set :-)
>
>
> http://codereview.chromium.org/159904
>

[wtc, 2009-08-06 21:39:20]

LGTM.  Two minot issues below.

http://codereview.chromium.org/159904/diff/4005/4009
File net/socket/tcp_client_socket_libevent.cc (right):

http://codereview.chromium.org/159904/diff/4005/4009#newcode321
Line 321: result = MapPosixError(error_code);
Call MapConnectError here to map error code of an async
connect failure.

http://codereview.chromium.org/159904/diff/4005/4010
File net/socket/tcp_client_socket_win.cc (right):

http://codereview.chromium.org/159904/diff/4005/4010#newcode551
Line 551: result = MapWinsockError(error_code);
Call MapConnectError here.