Non-blocking connect() attempts may fail synchronously

Non-blocking connect() attempts may fail synchronously in some cases.  When
this occurs, connect() should be retried with another address if possible and
appropriate.

On Mac OS X 10.6 ("Snow Leopard"), getaddrinfo() returns IPv6 addresses even
when inappropriate due to the use of AI_ADDRCONFIG.  connect() fails
immediately when trying to connect to an IPv6 address from a system that only
has IPv4 connectivity.  The existing net::TCPClientSocketLibevent is not
prepared to deal with immediate connect() failures, so it fails without
trying additional addresses.  Some sites, such as python.org, publish both
IPv4 and IPv6 addresses.  On Snow Leopard, name resolution always returns
the IPv6 addresses first, rendering such sites impossible to connect to unless
reachable by IPv6.

This change restores the previous behavior of setting AI_ADDRCONFIG when
calling getaddrinfo() on Mac OS X.  AI_ADDRCONFIG was removed in a previous
attempt to fix this bug.  AI_ADDRCONFIG is now documented in Snow Leopard.
The associated comment, written for Mac OS X 10.5 ("Leopard"), is no longer
correct.  In most cases, the presence or absence of this flag seems to have no
impact on the system resolver's behavior, but I believe that its presence is
correct per the documentation.  A separate bug will be filed with Apple.

BUG=12711
TEST=http://python.org/ on Snow Leopard should load on a machine where only
     IPv4 is available; it (and all other sites) should continue to function
     properly on Leopard

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

[Mark Mentovai, 2009-09-11 21:03:58]

Wan-Teh, I wonder if we also want to add ENETDOWN to the list of errors in
ShouldTryAnotherAddressForError.

[wtc, 2009-09-11 21:31:55]

LGTM!  Thanks for tracking this down.

Note: I found that the use of 'addrinfo', without 'struct',
is prevalent in tcp_client_socket_libevent.cc, so you can
ignore that comment below.

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

http://codereview.chromium.org/196094/diff/1/2#newcode83
Line 83: bool ShouldTryAnotherAddressForError(int err) {
Can we shorten this function to ShouldTryNextAddress? :-)

http://codereview.chromium.org/196094/diff/1/2#newcode141
Line 141: // host.  Recurse back into Connect, using the next address in the
list.
I wonder if we should use a loop instead of recursion.
I admit that I am not proficient with recursions.

http://codereview.chromium.org/196094/diff/1/2#newcode142
Line 142: const addrinfo* next = ai->ai_next;
Nit: please use 'struct addrinfo' because it is a C struct.

http://codereview.chromium.org/196094/diff/1/2#newcode151
Line 151: if (errno != EINPROGRESS) {
Another minor optimization: errno == EINPROGRESS is
the most common case.  So after connect() returns -1,
we should first check if errno == EINPROGRESS, and
then do the ShouldTryAnotherAddressForError check.

[Mark Mentovai, 2009-09-11 22:10:22]

New version up.

[wtc, 2009-09-11 22:32:34]

Looks good!

http://codereview.chromium.org/196094/diff/6001/6002
File net/socket/tcp_client_socket_libevent.cc (right):

http://codereview.chromium.org/196094/diff/6001/6002#newcode91
Line 91: case ENETDOWN:
Nit: list ENETDOWN after EHOSTUNREACH because they are
related.

http://codereview.chromium.org/196094/diff/6001/6002#newcode126
Line 126: bool try_next_address;
It seems that you can write the loop like:
  for (;;) {
    ..
    if (error_code == EINPROGRESS)
      break;
    ...
  }
and get rid of the try_next_address boolean.