Don't let crypton-connection resolve hostname

[sol]

Instead use the existing resolver.

@Yuras said in #579:

Fixes #569 and #544 (EDIT: no, it doesn't), likely fixes commercialhaskell/stack#5994

The problems is that crypton-connection tries addresses one by one, so if the fist address returned by getaddrinfo(3) is a misconfigured ipv6 address, it'll spend quite a bit of time trying to connect to ipv6 address, and only then try the next address.

Note that http-client tries connections concurrently.

I tested only getTlsConnection part and not getTlsProxyConnection since I don't actually have any proxy on hands. I let maintainer decide what to do next.

[sol]

@Yuras regarding your original comment:

-- We won't send TLS bye in case of exception, but that's OK

That relates to NC.connectionClose, I guess. So I assume we don't properly terminate the connection, but we close the socket, which is technically all we care about.

Comparing connectTo to withSocket, there seems to be a lot of things going on in connectTo, including things related to SOCKS. So I can't trivially convince myself that this won't break the proxy code path (+ apparently that code path is also not covered by the test suite). I'll either have to look at it again when I got more sleep, or somebody has to test it. 😅

https://github.com/kazu-yamamoto/crypton-connection/blob/1af72aad1733046195e6f0025adcb7944fa75dad/Network/Connection.hs#L219

[sol]

Another question is, whether we would want to address this in crypton-connection instead, not sure?

@kazu-yamamoto

[Yuras]

That relates to NC.connectionClose, I guess. So I assume we don't properly terminate the connection, but we close the socket, which is technically all we care about.

This is true. Though one can argue we are relying on the current implementation of crypton-connection. Nothing stops connectFromSocket from allocating resources meant to be deallocating in connectionClose. Note that connectFromSocket already calls Network.TLS.contextNew when connectionUseSecure is set (not our case fortunately.)

So I can't trivially convince myself that this won't break the proxy code path

Fair. Though connectTo only looks complicated, we call it with OtherProxy, in which case it's only doing name resolution and calls connectFromSocket. But I understand your concern.

[Yuras]

whether we would want to address this in crypton-connection instead,

It definitely should be fixed in crypton-connection as well. But IMO it doesn't make much sense for http-client to use two different name resolutions. Since we already have our own implementation, then let's use it everywhere.

[kazu-yamamoto]

@sol I forked connection to crypton-connection just to solve the dependency problem and don't know the internal at all. So, I cannot answer your question.

@Yuras If you will send a PR to crypton-connection, I would love to merge it.