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

use aiodns.DNSResolver.gethostbyname() #1136

Merged
merged 1 commit into from
Aug 30, 2016

Conversation

Martiusweb
Copy link
Contributor

aiodns 1.1 has been released this weekend, it adds the support of DNSResolver.gethostbyname(). This PR updates aiohttp.resolver.AsyncResolver to use it.

The main difference between DNSResolver.query() and gethostbyname() is that the latter works more like getaddrinfo() and reads /etc/hosts before it tries to issue a query to a DNS server. It effectively allows to use local hostnames such as localhost.

I don't know if it's better to require aiodns 1.1 or if the code should instead check if gethostbyname() is available and fallback to query() if not. We can also add a third Resolver class instead of modifying AsyncResolver, but it don't think that's really useful.

In the first case, I will add the requirement notice in the documentation/readme/requirements, otherwise I can rewrite the PR.

@codecov-io
Copy link

codecov-io commented Aug 29, 2016

Current coverage is 98.28% (diff: 100%)

Merging #1136 into master will increase coverage by <.01%

@@             master      #1136   diff @@
==========================================
  Files            28         28          
  Lines          6465       6474     +9   
  Methods           0          0          
  Messages          0          0          
  Branches       1083       1085     +2   
==========================================
+ Hits           6354       6363     +9   
  Misses           61         61          
  Partials         50         50          

Powered by Codecov. Last update 77fa3b5...f31b009

@asvetlov
Copy link
Member

Cool!
The patch should support both versions.
I suggest to check for _resolver.gethostbyname existence in the constructor an call appropriate method.

@Martiusweb Martiusweb force-pushed the gethostbyname branch 3 times, most recently from 6bdfa37 to 13097c6 Compare August 30, 2016 09:30
@Martiusweb
Copy link
Contributor Author

Hi,

I updated my patch, rebased master and eventually defeated the CI and coverage checks. Tell me if there is something missing.

@asvetlov asvetlov merged commit 0d42774 into aio-libs:master Aug 30, 2016
@asvetlov
Copy link
Member

Thanks!

@Martiusweb Martiusweb deleted the gethostbyname branch August 30, 2016 14:16
@lock
Copy link

lock bot commented Oct 29, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants