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

Aiodns support #747

Closed
wants to merge 10 commits into from
Closed

Aiodns support #747

wants to merge 10 commits into from

Conversation

gwillem
Copy link
Contributor

@gwillem gwillem commented Jan 24, 2016

Fixes #728. Todo:

  • Mock DNS tests
  • Add docs

Willem de Groot added 4 commits January 24, 2016 13:14
@gwillem gwillem changed the title WIP Aiodns support (fixes #728) WIP Aiodns support Jan 24, 2016
aiodns = None


class ExecutorResolver(AbstractResolver):
Copy link
Member

Choose a reason for hiding this comment

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

Strictly speaking thread pool is just internal details of asyncio implementation, and may be change in future. I'd call it 'DefaultResolver' and document it as default asyncio based resolver

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

@@ -0,0 +1,62 @@
import asyncio
Copy link
Member

Choose a reason for hiding this comment

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

Please rewrite tests to use py.test style (like test_client_connection.py for example).

@lphuberdeau
Copy link
Contributor

Any way I can help this feature move forward?

@gwillem
Copy link
Contributor Author

gwillem commented Feb 11, 2016

@lphuberdeau I was planning to continue on this PR in a week or 2-3 but I am absolutely swamped right now. You're most welcome to contribute if you have an earlier opportunity. Let me know.

@lphuberdeau
Copy link
Contributor

@gwillem I've branched off your work. I'll notify you when the pull request is ready to be updated.

@lphuberdeau
Copy link
Contributor

https://github.com/lphuberdeau/aiohttp/tree/aiodns-support

  • I've converted the tests to py.test
  • Mocked the DNS requests
  • Added documentation

I'm a bit unclear about what needs to be tested in the TCPConnector without mocking absolutely everything.

About the missing .close(), it could be added, but given the object is instantiated outside of the connector, I'm not certain it is the connector's responsibility.

@gwillem
Copy link
Contributor Author

gwillem commented Feb 12, 2016

Apparently pycares does not build on Win arch. Is this a showstopper, given that Aiodns is optional?

@lphuberdeau great work btw!

@gwillem gwillem changed the title WIP Aiodns support Aiodns support Feb 12, 2016
@lphuberdeau
Copy link
Contributor

The tests will run fine without it as the skips are in place. Everything in pycares seem to indicate it should build on windows. To resolve the CI issue, I see two options:

  • Separate the requirements file for travis and appveyor
  • Move the aiodns installation to the travis file only

@gwillem
Copy link
Contributor Author

gwillem commented Mar 7, 2016

FYI I have since migrated back to Tornado's AsyncHTTPClient for the following reasons:

IMHO aiohttp is cleaner, especially with native support for asyncio. And I would love to use aiohttp in the future. But right now I have deadlines :) so I need to be pragmatic.

@gwillem gwillem closed this Mar 7, 2016
@gwillem gwillem reopened this Mar 7, 2016
@lphuberdeau
Copy link
Contributor

@gwillem If you no longer want to track this issue, I can probably submit it back under a different PR and handle the back and forth.

@gwillem
Copy link
Contributor Author

gwillem commented Mar 7, 2016

@lphuberdeau ok, please do! And again, thanks for joining in.

@lphuberdeau lphuberdeau mentioned this pull request Mar 7, 2016
1 task
@lphuberdeau
Copy link
Contributor

Feel free to close this one.

@gwillem gwillem closed this Mar 8, 2016
@gwillem gwillem deleted the aiodns-support branch September 15, 2016 14:28
@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.

Add optional support for aiodns to TCPConnector for async DNS resolving
4 participants