search

aesiniath / http-streams

Haskell HTTP client library for use with io-streams
https://hackage.haskell.org/package/http-streams
BSD 3-Clause "New" or "Revised" License
50 stars 48 forks source link

Remove AI_ADDRCONFIG flag from getAddrInfo hints #116

Closed iliastsi closed 4 years ago

iliastsi commented 4 years ago

Do not use AI_ADDRCONFIG flag in getAddrInfo hints as it cannot handle '127.0.0.1' on IPv6-only machines.

istathar commented 4 years ago

Ok. Why would we have had it there in the first place? I assume there was a reason. I'd like to know what behaviour that was supplying before we remove the flag.

istathar commented 4 years ago

So I dug back, and AF_ADDRCONFIG was added in https://github.com/istathar/http-streams/commit/1d22ad130747680142542bd26e137c6183579a3b. It seems likely similar to the kind of issue that appeared in Firefox for example. Documentation for getaddrinfo() https://man7.org/linux/man-pages/man3/getaddrinfo.3.html talks about the interaction between this flag, loopback, and IPv6 addresses.

iliastsi commented 4 years ago

This PR was made in a hurry, I should have added more context, sorry about that. So, the problem we are facing is that on IPv6-only nodes, getaddrinfo() with AI_ADDRCONFIG will fail to yield any results for nodenames such as 127.0.0.1 and localhost4. Since Debian introduced IPv6 only builders, we noticed that tests for various Haskell packages started to fail because of this (see bug reports for yesod-core and haskell-http-conduit as an example). So, we tried to remove AI_ADDRCONFIG from Haskell packages (see http-client, streaming-commons and irc-core for some examples). Also, this is a great reading about why AI_ADDRCONFIG is poorly implemented on linux.

istathar commented 4 years ago

That was a very comprehensive reply. Thanks. I'll dig into it a bit more but Michael having removed it from an equivalent library makes it pretty obvious it's a safe change (I'm not confident I can replicate whatever the problem context was previously, but I'll at least test in the environments I have to hand first).

istathar commented 4 years ago

I've merged this patch. If anyone has problems with the behaviour going forward open an issue, reference this pull request, and we'll see if we can figure out how to make everyone happy.