Allow getaddrinfo as a fallback in connect_socket when inet_pton can't parse addr or is unavailable #357
Conversation
| return self.run_in_executor(None, socket.getaddrinfo, | ||
| host, port, family, type, proto, flags) | ||
|
|
||
| def _ensure_resolved(self, host, port, *, |
There was a problem hiding this comment.
Let's make this a top-level function in base_events.py.
Fix Unix domain socket connections, too.
|
Tests pass, but there's a ResourceWarning on Windows Nevertheless, this is ready for review. Here's the general idea:
Since no major platform locks around getaddrinfo any more (issue 26406 and issue 25924), the original motivation for avoiding getaddrinfo is less strong now. |
|
Thanks, Jesse, a few notes:
|
If inet_pton is unavailable, go straight to getaddrinfo without other logic to transform host info into a getaddrinfo result. Also remove the LRU cache around _ipaddr_info.
| # "host" is wrong IP version for "family". | ||
| return None | ||
| if af == socket.AF_INET6: | ||
| socket.inet_pton(af, host.partition('%')[0]) |
There was a problem hiding this comment.
I think we should also just return None if there is a % character in the host. The zone index should somehow influence IPv6 scope_id, something we can't do here.
|
Thanks, Jesse. Left a couple more comments. Also, the Windows build is failing. |
Apparently getaddrinfo can return protocol=0.
|
@1st1 I think I've fixed everything you commented on? I'm proposing this patch as final. |
|
Thanks Yury! I appreciate learning and contributing with you. On Wednesday, June 8, 2016, Yury Selivanov notifications@github.com wrote:
|
This is an approach to http://bugs.python.org/issue27136. It allows getaddrinfo for addresses we can't parse, like the Bluetooth address in that bug report. My theory is that, since getaddrinfo no longer has a mutex on popular platforms, it's ok to call it to confirm that an address has been resolved already, in cases where we can't confirm it in asyncio.
The
BaseSelectorEventLoopTestspass with Python 3.4 and 3.5 on Mac, Linux, and Windows; other tests would need to be updated before this can be merged.Based on @1st1's suggestions: credit is due to him, but blame mistakes on me.