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

Add the Miou implementation of happy-eyeballs #41

Merged
merged 2 commits into from
Aug 23, 2024

Conversation

dinosaure
Copy link
Contributor

This implementation provide an happy-eyeballs implementation with Miou with the idea proposed in #38 (with a protection to avoid to inject multiple a time a DNS resolver). This implementation is able to provide what is needed for dns-client then and handle UDP, TCP and TLS DNS resolution (the last one with tls-miou available here mirleft/ocaml-tls#494).

I tested the implementation with httpcats and see if I was able to resolve domain-names with UDP, TCP and TLS. Minor some timeout on uncensoreddns (used for TLS), everything seems to work.

The idea of this implementation is to launch a task in parallel which will care about its internal state and how to connect sockets to a given IP address. The daemon tries multiple connections and take the first one connected - other sub-tasks are cancelled and their file-descriptors are properly closed. The daemon works with a internal queue which aggregate user's actions and events from sub-tasks (when a socket is connected, we fill this queue with such event). The daemon can be suspended a certain amount of times (when happy-eyeballs returns `Act) or suspended as long as nothing (user's actions or events) appears (when happy-eyeballs returns `Suspend). Finally, the transfer of the file-descriptor is done via an atomic value. The user initiate a busy-loop and receive an error (if the happy-eyeballs say so) or the socket connected.

A documentation was written to explain the purpose of inject_resolver and to precise that the user must kill the daemon at the end of the process.

@dinosaure
Copy link
Contributor Author

I added a new patch which takes the advantages of Miou_unix.Ownership and ensure that, in any situation, we don't leak file-descriptors. I added some comments to explain how Miou handles file-descriptors and cancellation.

@dinosaure
Copy link
Contributor Author

Actually, happy-eyeballs-miou-unix requires dns to have the Transport.S interface. Since my previous work, we can:

  1. keep the PR as is
  2. defer all the implementation directly into Provide a Miou implementation of dns-client mirage/ocaml-dns#352

@dinosaure
Copy link
Contributor Author

I re-implemented happy-eyeballs with Miou (and no pin are required) to reflect the recent change made on the package and ocaml-dns. I think this PR is good to review and merge now.

@dinosaure
Copy link
Contributor Author

Ok, we are done on this side. I checked that we don't have a file-descriptor leak. The Miou.Ownership design works like a charm and I improved this version by the deletion of a busy-waiting loop and replace it directly by Miou.Computation which take the advantage of the low-level API of Miou (inspired by picos). A little program is provided: happy-eyeballs-miou-unix.connect which can helps us to see how the library works via logs.

@dinosaure dinosaure merged commit deac31f into robur-coop:main Aug 23, 2024
3 checks passed
@dinosaure dinosaure deleted the miou branch August 23, 2024 12:28
dinosaure added a commit to dinosaure/opam-repository that referenced this pull request Aug 23, 2024
CHANGES:

* Add the miou implementation of happy-eyeballs (@dinosaure, @hannesm, robur-coop/happy-eyeballs#41)
* Fix the CirrusCI (@hannesm, robur-coop/happy-eyeballs#43)
dinosaure added a commit to dinosaure/opam-repository that referenced this pull request Aug 23, 2024
CHANGES:

* Add the miou implementation of happy-eyeballs (@dinosaure, @hannesm, robur-coop/happy-eyeballs#41)
* Fix the CirrusCI (@hannesm, robur-coop/happy-eyeballs#43)
avsm pushed a commit to avsm/opam-repository that referenced this pull request Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant