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

Get rid of aiodns gethostbyname() #1351

Closed
wants to merge 4 commits into from
Closed

Get rid of aiodns gethostbyname() #1351

wants to merge 4 commits into from

Conversation

weastur
Copy link
Contributor

@weastur weastur commented Oct 30, 2016

What do these changes do?

By default aiodns function gethostbyname() (which is actually just a wrapper around c-ares function) resolve host as IPv6 address if socket family set to AF_UNSPEC(default value for TCPConnector). On the other hand ThreadedResolver use asyncio loop function getaddrinfo() which, in accordance with POSIX return both(either IPv4 and IPv6) if socket family set to AF_UNSPEC. So we have unconsistent behavior of ThreadedResolver and AsyncResolver. But c-ares lib doesn't have getaddrinfo() function, so we can't use it in aiohttp resolver. So if I use IPv4 the following simple code does not work for me(if aiodns installed of cause):

 async with aiohttp.ClientSession() as session:
        async with session.get('http://google.com') as resp:
            body = await resp.read()
            print('Done(google.com). status: {}, body len: {}'.format(resp.status, len(body)))

To solve this problem in method resolve() of AsyncResolver I query both types if family set to AF_UNSPEC.

Are there changes in behavior for the user?

No

Related issue number

#559

Checklist

  • [ X] I think the code is well written
  • [ X] Unit tests for the changes exist
  • [ X] Documentation reflects the changes
  • [ X] Add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • [ X] Add a new entry to CHANGES.rst
    • Choose any open position to avoid merge conflicts with other PRs.
    • Add a link to the issue you are fixing (if any) using #issue_number format at the end of changelog message. Use Pull Request number if there are no issues for PR or PR covers the issue only partially.

Copy link
Member

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

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

No. Until the issue will be fixed by upstream we just should revert back default to use thread pool based solution.

@weastur
Copy link
Contributor Author

weastur commented Oct 30, 2016

Ok. But what if I want to use aiodns? I think the behavior should be the same as a thread pool. We can set thread pool as default and fix async resolver too. It does not look like they wanted to add getaddrinfo() into c-ares. c-ares/c-ares#70

@asvetlov
Copy link
Member

Looks like they are looking for volunteer.
If you want to work via aiodns -- connect it explicitly and use on your own.

@asvetlov asvetlov closed this Oct 31, 2016
@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.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants