126755 - DNS: multiple IP addresss failover (when timeout occurs)

Status: VERIFIED FIXED

(Core :: Networking: HTTP, defect, P2)

Description

[Munechika Sumikawa]

bugzilla#86917(multiple ip trying) still has a bug.
	http://bugzilla.mozilla.org/show_bug.cgi?id=86917

multiple ip trying works correctly when connection is refused. However it does
not work when connection is timed out.

I'll attach the patch. In this patch, new mCurrentState is
eSocketState_WaitConnect after timed out but I'm not sure if this is correct.

Could you review it and commit it?

Comment 1

[Munechika Sumikawa]

Attachment: fix timeout falling back bug

Here is the patch.

Comment 2

[Matthias Versen [:Matti]]

-> Darin the Http master

Comment 3

[Darin Fisher]

-> moz 1.0

thanks for the patch...

Comment 4

[Darin Fisher]

Attachment: revised patch

- factored out some of the common code.
- ensure that we don't try the next address if we've already fired
OnStartRequest. 
  we fire OnStartRequest once we've made a successful connection.

there may be some issues with this patch and SSL since SSL doesn't always
complete a connection until the first Read or Write on the socket.  i assume
though that it will have made at least the initial TCP/IP handshake by the time
doConnection succeeds.	otherwise, this patch may need to be rethought.

Comment 5

[Darin Fisher]

wtc: the initial call to PR_Connect on a NSS PRFileDesc corresponds to a TCP/IP
SYNC right?  so, we'll know by the time we try to read or write on the socket
that we've successfully transmitted some packets to the server, right?

Comment 6

[Darin Fisher]

Munechika: can you test this patch?  i'm not exactly sure how best to simulate a
TCP/IP timeout error.

Comment 7

[Wan-Teh Chang]

Darin wrote:
> the initial call to PR_Connect on a NSS PRFileDesc corresponds
> to a TCP/IP SYNC right?

This is true for a NSS PRFileDesc.

> so, we'll know by the time we try to read or write on the socket
> that we've successfully transmitted some packets to the server,
> right?

I think so.  I don't understand why you asked this question though.

Comment 8

[Wan-Teh Chang]

Darin,

This is how the ssl_SecureConnect() function is implemented:
http://lxr.mozilla.org/mozilla/source/security/nss/lib/ssl/sslsecur.c#856

It invokes the connect method on the lower-layer PRFileDesc.
So I think your assumption is valid:
  i assume though that it will have made at least the initial
  TCP/IP handshake by the time doConnection succeeds.

Comment 9

[Darin Fisher]

wtc: the point of my question was to make sure that we cannot advance to the
read/write state without knowing for sure that the socket connection has been
established.  i know this to be true of normal sockets, but wasn't 100% sure
about NSS sockets.  thanks for verifying things for me :-)

Comment 10

[Munechika Sumikawa]

Darin, I've confirmed your revised patch works correctly in both
situation:
  1. fallback when connection refused
  2. fallback when timeout

Comment 11

[Darin Fisher]

Munechika: excellent!  thank you for testing this patch :-)

Comment 12

[Doug Turner (:dougt)]

Comment on attachment 73122 [details] [diff] [review]
revised patch

Do you really want to attempt another IP if mStatus is an error other than
NS_ERROR_CONNECTION_REFUSED?

Comment 13

[Darin Fisher]

dougt: sure... how about connection attempt timed out?  or no route to host
(which  is currently mapped to NS_ERROR_CONNECTION_REFUSED).  the point is that
if we have trouble connecting to a particular IP address, and if DNS provides
other IP addresses, we should try those other IP addresses before giving up.

Comment 14

[Darin Fisher]

talked to dougt over AIM... probably is best to only enter TryNextAddress do to
specific errors, not just any error.

... will put together another patch.

Comment 15

[Darin Fisher]

Attachment: v3 patch

Comment 16

[Doug Turner (:dougt)]

Comment on attachment 74354 [details] [diff] [review]
v3 patch

thanks r=dougt

Comment 17

[rpotts (gone)]

Comment on attachment 74354 [details] [diff] [review]
v3 patch

sr=[e-mail]

Comment 18

[Asa Dotzler [:asa]]

Comment on attachment 74354 [details] [diff] [review]
v3 patch

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 19

[Darin Fisher]

fixed-on-trunk

Comment 20

[benc]

VERIFIED:
linux 2002040908 (right before the branch date)
I configured a dual entry that points to a dead address and a real-life server:

nslookup news.cpd.mcom.com
Server:  regress.nscp.aoltw.net
Address:  10.169.115.222

Name:    news.cpd.mcom.com
Addresses:  10.169.115.221, 64.124.237.133

It took me a while to verify the behavior, but I see that it tries to connect to
internal (dead) address, then it goes to the real-deal on the public network.

I'm not sure if I'm seeing too many ARPs, so I'm posting the list here, and I'll
try to dig up my TCP/IP illustrated book later. 
 
# snoop arp
Using device /dev/le (promiscuous mode)
   carbuncle -> (broadcast)  ARP C Who is 10.169.115.221, 10.169.115.221 ?
   carbuncle -> (broadcast)  ARP C Who is 10.169.115.221, 10.169.115.221 ?
   carbuncle -> (broadcast)  ARP C Who is 10.169.115.221, 10.169.115.221 ?
   carbuncle -> (broadcast)  ARP C Who is 10.169.115.221, 10.169.115.221 ?
   carbuncle -> (broadcast)  ARP C Who is 10.169.115.221, 10.169.115.221 ?
   carbuncle -> (broadcast)  ARP C Who is 10.169.115.221, 10.169.115.221 ?
   carbuncle -> (broadcast)  ARP C Who is 10.169.115.221, 10.169.115.221 ?
   carbuncle -> (broadcast)  ARP C Who is 10.169.115.221, 10.169.115.221 ?
   carbuncle -> (broadcast)  ARP C Who is 10.169.115.221, 10.169.115.221 ?
   carbuncle -> (broadcast)  ARP C Who is 10.169.115.221, 10.169.115.221 ?
   carbuncle -> (broadcast)  ARP C Who is 10.169.115.221, 10.169.115.221 ?
   carbuncle -> (broadcast)  ARP C Who is 10.169.115.221, 10.169.115.221 ?
   carbuncle -> (broadcast)  ARP C Who is 10.169.115.221, 10.169.115.221 ?
   carbuncle -> (broadcast)  ARP C Who is 10.169.115.221, 10.169.115.221 ?
   carbuncle -> (broadcast)  ARP C Who is 10.169.115.221, 10.169.115.221 ?
   carbuncle -> (broadcast)  ARP C Who is 10.169.115.221, 10.169.115.221 ?