http-client icon indicating copy to clipboard operation
http-client copied to clipboard

why was `AI_NUMERICHOST` removed in `http-client-openssl`?

Open MangoIV opened this issue 1 year ago • 4 comments

in withSocket we always pass port to NS.getAddrInfo, but we don't pass a hint that the underlying c function is called with a numeric serviceName, that was once the case, so I wonder why that would be removed.

If it was accidental, the fix would be as simple as

diff --git a/http-client/Network/HTTP/Client/Connection.hs b/http-client/Network/HTTP/Client/Connection.hs
index 823072bd..c29407f6 100644
--- a/http-client/Network/HTTP/Client/Connection.hs
+++ b/http-client/Network/HTTP/Client/Connection.hs
@@ -193,7 +193,7 @@ withSocket :: (Socket -> IO ())
            -> (Socket -> IO a)
            -> IO a
 withSocket tweakSocket hostAddress' host' port' f = do
-    let hints = NS.defaultHints { NS.addrSocketType = NS.Stream }
+    let hints = NS.defaultHints { NS.addrSocketType = NS.Stream, NS.addrFlags = [ NS.AI_NUMERICSERV, NS.AI_ADDRCONFIG ] }
     addrs <- case hostAddress' of
         Nothing ->
             NS.getAddrInfo (Just hints) (Just $ strippedHostName host') (Just $ show port')

MangoIV avatar Jan 29 '24 15:01 MangoIV

relevant commit: https://github.com/snoyberg/http-client/commit/3bdfed4696deefc88356e3f8527bbb675036cc17

MangoIV avatar Jan 29 '24 15:01 MangoIV

@vshabanov I think you made that change, perhaps you remember why the hints were removed? perhaps this is also an implicit change because of the network package? though I don't understand why they would do numeric servicenames by default...

MangoIV avatar Jan 29 '24 15:01 MangoIV

I suspect it was accidental. Since it's always a number it doesn't make much sense to enforce it. But it could probably speedup things.

Not sure about AI_ADDRCONFIG. In theory it could speedup things too (no need to try IPv6 on IPv4 machine), but it can probably break address resolution in some configurations.

withSocket code is more generic/fail-safe. I wouldn't touch it unless there's a reason.

vshabanov avatar Feb 01 '24 21:02 vshabanov

Yeah I would propose adding back the AI_NUMERICHOST hint to the settings in http-client as I don’t think this could ever go wrong (the only kind of service name we ever pass are ports)

MangoIV avatar Feb 03 '24 09:02 MangoIV