robur-coop / happy-eyeballs

An implementation of happy eyeballs (RFC 8305) in OCaml with lwt
ISC License
22 stars 4 forks source link

Fix connection establishment. #34

Closed hannesm closed 1 year ago

hannesm commented 1 year ago

Consider a connection to IP addressed [ ::1 ; 127.1 ].

This should give some preference to ::1, but should attempt a connection to both ::1 and 127.1.

Earlier, the sequence of actions was as follows:

This means the connection to the IPv6 address was only waiting for v6_connect_timeout time, and then being cancelled. There were no two connection being attempted to be established at the same time.

Some DNS resolvers on the azure cloud (where Github actions are hosted) do not reply on port 853 (DNS-over-TLS), but just drop the packets. This confused the happy-eyeballs/dns-client stack, and it resulted in a timeout (as spotted by semgrep).

With this PR, the above scenario is changed to:

And the same is applied to using a single IP address and multiple ports (i.e. for the DNS resolver probing port 853 and 53).

Sponsored-By: Semgrep Inc

hannesm commented 1 year ago

Related is https://github.com/mirage/ocaml-dns/pull/340

hannesm commented 1 year ago

Diff is best viewed with ignoring whitespace changes: https://github.com/roburio/happy-eyeballs/pull/34/files?w=1

A test case is at https://github.com/roburio/http-lwt-client/tree/broken (CI run https://github.com/roburio/http-lwt-client/actions/runs/5269595997/jobs/9527948274) with the released versions of dns-client and happy-eyeballs.

This PR (and the dns-client PR) lead to success in the CI https://github.com/roburio/http-lwt-client/tree/more-action (CI run https://github.com/roburio/http-lwt-client/actions/runs/5269527187/jobs/9527782281)

Of course, reviews are highly welcome, esp. in respect to resources. Now, the "cancel connection attempts" is done on timeout/failure (all destinations are exceeded)/success -- so there's no longer an explicit Connect_cancelled action, but the implementor is required to take care of their resources.