Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

why was AI_NUMERICHOST removed in http-client-openssl? #526

Open
MangoIV opened this issue Jan 29, 2024 · 4 comments
Open

why was AI_NUMERICHOST removed in http-client-openssl? #526

MangoIV opened this issue Jan 29, 2024 · 4 comments

Comments

@MangoIV
Copy link

MangoIV commented Jan 29, 2024

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
Copy link
Author

MangoIV commented Jan 29, 2024

relevant commit: 3bdfed4

@MangoIV
Copy link
Author

MangoIV commented Jan 29, 2024

@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...

@vshabanov
Copy link
Contributor

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.

@MangoIV
Copy link
Author

MangoIV commented Feb 3, 2024

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants