try all addresses returned by getAddrInfo #133
Conversation
| -- single AddrInfo; call connectAddrInfo directly so that specific | ||
| -- exception is thrown in event of failure |
There was a problem hiding this comment.
You could maybe generalise this by having 'tryAddrInfos' leave out the catch if there's only one element in the list, so that the user always sees one of the connection failures. They might also want to know all addresses that were tried, but that would require more effort with wrapping exceptions etc.
There was a problem hiding this comment.
I prefer to keep all of the dispatch based on number-of-AddrInfos in one place (currently in the case addrinfos of ...).
It is possible to move all that logic to tryAddrInfos, but then you lose the distinction between "no AddrInfos returned by getAddrInfoand "There were multiple addresses, but they all failed". In my PR these cases have differentfail` error messages, and I think that's a good thing.
There was a problem hiding this comment.
OK - maybe just include all the addresses in the error message for "they all failed" so the user can then debug by trying them explicitly? It's also ok if you want to leave it as is.
There was a problem hiding this comment.
@hsenag I updated the code to show the tried addresses in the error messages.
There was a problem hiding this comment.
I prefer to keep all of the dispatch based on number-of-AddrInfos in one place (currently in the
case addrinfos of ...).
It is possible to move all that logic totryAddrInfos, but then you lose the distinction between "no AddrInfos returned by getAddrInfoand "There were multiple addresses, but they all failed". In my PR these cases have differentfail` error messages, and I think that's a good thing.
I don't quite understand that you mean, but I'll note that getAddrInfo never returns an empty list. If there are no addresses it throws and exception, instead.
The only way I can think to test it is to start up servers on localhost and try and connect. But it will be brittle or unreliable because some systems executing the tests may be ipv4-only or ipv6-only. Even ignoring that there's significant complexity to test it. I don't want to write it :) |
frasertweedale
force-pushed
the
fix/try-multiple-addrinfo
branch
3 times, most recently
from
5c41d77 to
d39b45c
Compare
Network/TCP.hs
Outdated
|
|
||
| -- try multiple addresses; return Just connected socket or Nothing | ||
| tryAddrInfos [] = return Nothing | ||
| tryAddrInfos (h:t) = fmap Just (connectAddrInfo h) `catchIO_` tryAddrInfos t |
There was a problem hiding this comment.
You should use try rather than running the remaining connections in an async-exception masked catch handler.
Network/TCP.hs
Outdated
|
|
||
| let | ||
| connectAddrInfo a = do | ||
| s <- socket (addrFamily a) Stream defaultProtocol |
There was a problem hiding this comment.
There's bracketOnError for this resource pattern
|
There are standards for connecting to network services in the ipv4/ipv6 world we live in now. One of those is Happy Eyeballs Version 2: Better Connectivity Using Concurrency. This might be more effort that we're willing to go through for HTTP, but I wanted to call it out. I've implemented something close to this in the network connection library I use for my IRC client hookup:openSocket The algorithm tries not to get slowed down (too much) when a user has slow or stalled connectivity on any one address. |
|
@glguy yep, the commit message mentions happy eyeballs, and the fact that this ain't it :) I am... not keen to tackle that today. Ideally it would implemented in the network package anyhow. As for your other requests, updated commit coming soon. |
Currently, openTCPConnection only considered the first address returned by getAddrInfo. This can cause connection failures. For example, if the first address returned is an IPv6 address, but the HTTP server only listens on IPv4, then the connection will be refused, and the library gives up. Update openTCPConnection to try each address returned by getAddrInfo. This is a naïve algorithm; better approaches are possible (e.g. see https://en.wikipedia.org/wiki/Happy_Eyeballs). This change may regress performance of this library if getAddrInfo returns two or more addresses and several connection attempts time out (as opposed to "connection refused"). But most users would probably prefer to succeed slowly than to fail unnecessarily.
frasertweedale
force-pushed
the
fix/try-multiple-addrinfo
branch
from
d39b45c to
5d132dc
Compare
|
Apologies for not merging this sooner, for some reason I had thought there was some open discussion about it. I'll deal with it in the next few days, please feel free to remind me if I don't. |
|
@hsenag no worries at all! |
Currently, openTCPConnection only considered the first address
returned by getAddrInfo. This can cause connection failures. For
example, if the first address returned is an IPv6 address, but the
HTTP server only listens on IPv4, then the connection will be
refused, and the library gives up.
Update openTCPConnection to try each address returned by
getAddrInfo. This is a naïve algorithm; better approaches are
possible (e.g. see https://en.wikipedia.org/wiki/Happy_Eyeballs).
This change may regress performance of this library if getAddrInfo
returns two or more addresses and several connection attempts time
out (as opposed to "connection refused"). But most users would
probably prefer to succeed slowly than to fail unnecessarily.