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

dns: expose lookup_host function #1870

Merged
merged 21 commits into from
Dec 21, 2019
Merged

dns: expose lookup_host function #1870

merged 21 commits into from
Dec 21, 2019

Conversation

davidbarsky
Copy link
Member

This PR is a followup to #1684. This PR implements the design that @carllerche outlined in #1684 (comment):

I think returning a SocketAddr in this case is fine as we need to be able to take "tokio.rs:80" and map that to a SocketAddress.
Ok, so I think the conclusion is that the function should return a new type: LookupHost and that type should implement async fn next_addr(&mut self) -> Option<io::Result<SocketAddr>>.
This API is not intended to cover all DNS use cases. Anything beyond the basic use case should be done w/ a specialized library.

Note that this PR is not yet complete; I'm posting this because I'd like to get some feedback and open some discussion. Namely, I'd like to still address:

  • Examples in doc comments of lookup_host.
  • Implementing Future for LookupHost so that lookup_host can be called like: let addr = lookup_host("tokio.rs").await?.

Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. A couple of questions & requests inline.

pub struct LookupHost<T>
where
T: sealed::ToSocketAddrsPriv,
T: ToSocketAddrs<Future = sealed::MaybeReady>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you elaborate on why the bounds on Future and Iter are added?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unable to compile otherwise. I'd be happy to reduce this to just sealed::MaybeReady and sealed::OneOrMore, but I'm not sure of the proper incantation—trait bounds like these aren't my strong suit. I'd appreciate some pointers.

tokio/src/net/addr.rs Outdated Show resolved Hide resolved
tokio/src/net/addr.rs Outdated Show resolved Hide resolved
fut: <T as sealed::ToSocketAddrsPriv>::Future,
},
Addrs {
addrs: sealed::OneOrMore,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My guess is, to make it compile, this needs to be updated to: <T as sealed::ToSocketAddrsPriv>::Iter.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really sure that's the case. That being said, what sort of signature are you looking for here?

/// Performs a DNS resolution.
///
/// This API is not intended to cover all DNS use cases.
/// Anything beyond the basic use case should be done with a specialized library.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on this statement, I'm not sure why this needs to have such a "complicated" API surface.

One understanding from my side (correct me if wrong):

All mainstream DNS resolvers return all resolved addresses synchronously. Therefore just returning Vec<SocketAddr> or Vec<LookupResult> (if we need more information) should be fine.

One experience report from my side:

Most applications only care about 1 DNS result. However in the rare situations where I wanted more I always wanted to have all of them synchronously, so that I can filter the list and decide for the best result. An async iterator wouldn't really have bought me a lot here.

Based on these observations I think just providing async fn lookup_host(host: &str) -> Vec<LookupResult> as a default is fine.

If you want application writers to have more control and e.g. to avoid allocations, you could provide an optional LookupOptions struct to the function which e.g. allows to configure a predicate for filtering results before they are returned to the user. E.g.

struct LookupOptions {
    /// max amount of results to return
    limit: usize,
    /// filters results before returned
    filter: Fn(&LookupResult) -> bool,
}

async fn lookup_host_ext(host: &str, options: LookupOptions) -> Vec<LookupResult>
/// Not a great name, but you get the idea
async fn lookup_host_first_ext(host: &str, options: LookupOptions) -> LookupResult

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Matthias247 one argument I heard for returning multiple is that, some applications, want to get the IPv6 address and have to iterate through the results.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that's the filtering use-case I mentioned. But given the state of DNS resolvers I think that doing the synchronous lookup and then a .filter should be fine for those use-cases. And if there gets support integrated for DNS resolvers which operate asynchronously (merge results from multiple sources, where on is available earliers), then adding the second API that I mentioned which applies filtering internally also seems fine.

@carllerche
Copy link
Member

After all that discussion, I say we just ship something and if it doesn't work, revisit it in 0.3 or 0.4 (before 1.0).

I pushed to this PR, what do you think?

@davidbarsky
Copy link
Member Author

Yeah, I think I mentioned that something smaller like this could be a better fit on Discord. Happy to have this merged as is.

@carllerche carllerche merged commit de5ec6e into master Dec 21, 2019
@carllerche carllerche deleted the david/expose-lookup branch January 10, 2020 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants