search

robur-coop / happy-eyeballs

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

Reverse the dependency between dns-client-lwt and happy-eyeballs-lwt (dns-client-mirage and happy-eyeballs-mirage) #38

Closed dinosaure closed 3 months ago

dinosaure commented 10 months ago

As discussed with @hannesm and @reynir, this patch is a possible way to unlock the cycle between happy-eyeballs, happy-eyeballs-lwt, dns and dns-client-lwt. The idea is to use an internal function to resolve domain-name instead of Dns_client_lwt. The user is able to create a Dns_client_lwt from an Happy_eyeballs_lwt instance (and, by this way, we delete a duplicated code to handle connections). Then, the user is able to "inject" its getaddrinfo from its new Dns_client_lwt instance into its Happy_eyeballs_lwt instance. By this way, Happy_eyeballs_lwt is able to handle connections and domain-name resolution without a duplication of the code.

hannesm commented 10 months ago

This looks good, and could be applied to happy-eyeballs-mirage similarly.

The question is though, who's going to call inject? Earlier, happy-eyeballs-lwt brought that functionality. Now, either dns-client-lwt needs to do that, or something else.

The issue is for clients of this package -- they'd now need to dependn on happy-eyeballs-lwt and dns-client-lwt and call the inject.

Maybe one way around is that move this to happy-eyeballs-lwt-no-resolver and create a different package happy-eyeballs-lwt which depends on happy-eyeballs-lwt-no-resolver and dns-client-lwt, providing the old API? (I've no clue, am open for other suggestions, but would like to avoid all dependees needing to call the inject manually).

hannesm commented 10 months ago

couldn't resist -- 5f904a9 is what I have in mind... also 2b7c93b and imho we're good then :)

dinosaure commented 10 months ago

I don't really like the double getaddrinfo, I prefer to have a simple GADT (type _ record = A : Ipaddr.V4.Set.t record | AAAA : Ipaddr.V6.Set.t record and use it to implement the getaddrinfo. Also, the injection is probably not needed for the happy-eyeballs-lwt but it is needed for MirageOS.

The point about the injection here is: for lwt.unix, the default implementation works but we must not have such default implementation for MirageOS (or we will bring unix.cmxa). It's why I started with a dummy implementation. Then, the mutation must exists because, currently Dns_client_lwt requires an happy-eyeballs-lwt instance (to initiate connections to resolvers), so you must create and provide an happy-eyeballs-mirage instance to create, then, the dns-client-mirage instance. And to keep the same instance anywhere, you must inject the getaddrinfo (with the new dns-client-mirage instance) into the happy-eyeballs-mirage one and give it to the user to be able to connect and resolve.

hannesm commented 3 months ago

Moving forward, indeed we discovered that inject is needed.

I pushed a commit here which addresses the review comments. Thanks again for your work :)

hannesm commented 3 months ago

I pushed another commit which also reverses the dns-mirage/happy-eyeballs-mirage dependency: since there's no "default" getaddrinfo, I used an mutable getaddrinfo option in the type t. There'll be an error if you attempt to resolve a hostname without a previous injection.

WDYT @dinosaure? I'd be keen to get this merged and released, to proceed with the dns release. It will require changes in the users of this module.

dinosaure commented 3 months ago

LGTM :+1: