Substitute DNS Resolver

[DarrenTsung]

This relates to #1174.

It would be great if we could substitute a resolver instead of using to_socket_addrs and getaddrinfo. It seems there was already a good amount of discussion around this in #1174, but I'm not sure what the conclusion was.

It seems like there are three options:

1. Implement custom connector with Connect instead of HttpConnector
2. Add resolver type to HttpConnector (feat(client): make dns resolver pluggable #1174)
3. Modifying tokio's ClientProto(?). I admit I'm not sure what this actually means for end-users.

Honestly I think 2) makes a lot of sense because we get to avoid any duplication and maintenance efforts to keep a custom connector up-to-date / bug-free with HttpConnector. But again, I'm not sure if 3) is better?

Context for this is: we're trying to update from an old fork of hyper and had previously swapped out the resolver in the fork :).

@seanmonstar thoughts?

[seanmonstar]

The ClientProto stuff is long gone, nothing to worry about there :)

I've seen others ask to replace the resolve in HttpConnector also, and it would be nice to be do so. The only thing that has held me back is that it would be another trait to need to stabilize. I don't have a good design for one, and haven't spent time trying to solve this, since so far I've figured that just copying the TCP connecting bits from the HttpConnector is pretty simple.

I'd be open to working this out, though!

[pimeys]

I'd follow this issue and maybe even help:

tokio-rs/tokio#363

Would be pretty nice to just use Tokio's DNS resolver in Hyper when it's ready. There are some unofficial crates to use in your custom connector that should not be that hard to add to your stack, e.g:

https://github.com/sbstp/tokio-dns

[DarrenTsung]

Ah okay great, forget about the ClientProto stuff then.

I think thinking about a stable trait design is a fair point and might be more involved than I expected given the wide range of use-cases hyper needs to support.

However, if a trait-bound on HttpConnector is the goal, then the only logic we can replace is here, right?

                        let work = dns::Work::new(host, port);
                        state = State::Resolving(oneshot::spawn(work, executor));

The input on some hypothetical resolve function would be: fn resolve(host: String, port: u16) as per dns::Work::new() and it would return a Future<Item=Iterator<Item=SocketAddr>, Error=io::Error> (This is the same trait structure as #1174).

Okay, but maybe that's not the end goal, what other possible use cases might be brought up?

• Return results synchronously instead of a Future
  • This would be a strange use-case for hyper as "async I/O" is a point in the main page, is it fair to call this one a "write a custom HttpConnector"?
• Take in more context than host: String and port: u16
  • Looking at several different DNS crates (trust-dns, abstract-ns) as well as the standard ToSocketAddrs trait, this set of arguments seems reasonable and standard. Maybe instead of String it could take an IpAddr?
• Using a trait provided by another crate (like abstract-ns as discussed in feat(client): make dns resolver pluggable #1174)
  • This would be a good point, I could see some sort of ToSocketAddrsFuture trait that's implemented and shared amongst crates so there's no need for new types to bridge implementations.

Let me know if I'm missing anything, or if you're worried about any other use-cases! The only thing I can see is possibly supporting a generic ToSocketAddrsFuture (or something) trait with other crates, which would include research into what crates are used, what should be on a generic trait, etc. It expands the scope from a trait for HttpConnector to a trait of any DNS resolver to plug into, which is quite a leap.

Anyways I think an initial step is for me to duplicate the HttpConnector and add in DNS substitution. That way I can continue without being blocked and if supporting a larger scope is required it can be prototyped outside of Hyper without requiring releases for the crate as a whole. If no larger scope is required though, it would be good to get the trait bound merged in!

[seanmonstar]

Maybe instead of String it could take an IpAddr?

If we already had an IpAddr, we wouldn't need to do resolution, right? :D In fact, hyper does skip resolution if the hostname parses as an IpAddr.

I think it'd be reasonable to try to include a Resolve trait specifically for HttpConnector. What does it look like? Should it only be allowed to return io::Error, or should it allow any error, like Connect does?

trait Resolve {
    type Future: Future<Item=Self::Addrs, Error=io::Error>;
    type Addrs: Iterator<Item=SocketAddr>;
    fn resolve(&self, host: &str, port: u16) -> Self::Future;
}

Does it make sense to include a port as input? Or should the HttpConnect just override the any port when it receives the SocketAddr?

[DarrenTsung]

Didn't see your reply @pimeys, thanks for the links! I'll definitely keep an eye on tokio-dns, and would be happy to wait for it if that's the right solution.

@seanmonstar: Ah, yeah oops, that would not be necessary :P.

Does Connect allow users to define any error? From the code it looks to have some sort of bound on io:Error like the proposed trait, although you could just create io::Error from your Error.

Ah, okay, I'm looking at the 0.11 branch, on master branch it does allow any error. Given that HttpConnector is an implementation of Connect and defines its error type as io::Error it would make sense to me to have the Resolve trait (bounded on HttpConnector) as io::Error as well.

Good point with the port:

• Looking at trust-dns, the implementation for lookup_ip does not take in a port and returns IpAddrs
• tokio-dns::Resolveronly operates on the host and returns IpAddrs as well.
• abstract_ns::Resolve operates on only a string as well, it returns SocketAddrs though.

Given the API for these crates, I would lean towards avoiding passing in the port and have the future return Iterator<Item=IpAddr> and constructing the SocketAddrs from the IpAddrs and port.

trait Resolve {
    type Future: Future<Item=Self::Addrs, Error=io::Error>;
    type Addrs: Iterator<Item=IpAddr>;
    fn resolve(&self, host: &str) -> Self::Future;
}

Let me know what you think!

[DarrenTsung]

As an update on this, I've created a duplicate of hyper 0.11's HttpConnector and have swapped out the DNS resolver on a branch to use trust-dns and c-ares (after running into some internal trust-dns issues (https://github.com/bluejekyll/trust-dns/issues/470)).

Both implementations would match the latest proposed trait without any problems 👍.

[seanmonstar]

Great! How does the generic on HttpConnector play out in practice? Annoying? Ignorable?

I've wanted to be able to change out the default resolver for trust-dns for a while, and besides wanting to wait for it to be as reliable as getaddrinfo, the other problem is some of the constructors for HttpConnector would break. This change could actually make the switch compatible.

Basically, it could be similar to HashMap that has a default hasher that doesn't otherwise expose what it is, which allowed switching from SipHash-2-4 to 1-3. There could be a DefaultResolver newtype that hides its details, besides a GAI resolver type.

I'll be of limited availability for the next week, but I'd welcome this change for the 0.12 release if someone would like to make a PR!

[seanmonstar]

This may slip out of 0.12, as it isn't high on my priorities, and I'd like to release this week.

[DarrenTsung]

Oh, sorry for ignoring this thread!

Happy to contribute a PR once we're satisfied with the code, we're still on 0.11 so it would require some changes to work with 0.12.

[seanmonstar]

I got around to fleshing this out in #1674.