Incorrect import of TimeoutError while creating happy eyeballs connection

[tirkarthi]

BPO	39129
Nosy	@asvetlov, @1st1, @tirkarthi
PRs	bpo-39129: Fix import path for asyncio.TimeoutError #17691 [3.8] bpo-39129: Fix import path for asyncio.TimeoutError (GH-17691) #17693

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2019-12-24.07:04:12.998>
labels = ['type-bug', '3.8', '3.9', 'expert-asyncio']
title = 'Incorrect import of TimeoutError while creating happy eyeballs connection'
updated_at = <Date 2020-01-08.19:00:16.275>
user = 'https://github.com/tirkarthi'

bugs.python.org fields:

activity = <Date 2020-01-08.19:00:16.275>
actor = 'xtreak'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['asyncio']
creation = <Date 2019-12-24.07:04:12.998>
creator = 'xtreak'
dependencies = []
files = []
hgrepos = []
issue_num = 39129
keywords = ['patch']
message_count = 5.0
messages = ['358841', '358845', '358847', '358848', '359625']
nosy_count = 3.0
nosy_names = ['asvetlov', 'yselivanov', 'xtreak']
pr_nums = ['17691', '17693']
priority = 'normal'
resolution = None
stage = 'test needed'
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue39129'
versions = ['Python 3.8', 'Python 3.9']

[tirkarthi]

I guess the TimeoutError exception needs to be imported from asyncio.exceptions and not from asyncio.futures that causes AttributeError while instantiating a connection with happy eyeballs.

./python.exe -m asyncio
asyncio REPL 3.9.0a2+ (heads/master:068768faf6, Dec 23 2019, 18:35:26)
[Clang 10.0.1 (clang-1001.0.46.4)] on darwin
Use "await" directly instead of "asyncio.run()".
Type "help", "copyright", "credits" or "license" for more information.
>>> import asyncio
>>> conn = await asyncio.open_connection("localhost", port=8000, happy_eyeballs_delay=1)
Traceback (most recent call last):
  File "/Users/kasingar/stuff/python/cpython/Lib/concurrent/futures/_base.py", line 439, in result
    return self.__get_result()
  File "/Users/kasingar/stuff/python/cpython/Lib/concurrent/futures/_base.py", line 388, in __get_result
    raise self._exception
  File "<console>", line 1, in <module>
  File "/Users/kasingar/stuff/python/cpython/Lib/asyncio/streams.py", line 52, in open_connection
    transport, _ = await loop.create_connection(
  File "/Users/kasingar/stuff/python/cpython/Lib/asyncio/base_events.py", line 1041, in create_connection
    sock, _, _ = await staggered.staggered_race(
  File "/Users/kasingar/stuff/python/cpython/Lib/asyncio/staggered.py", line 144, in staggered_race
    raise d.exception()
  File "/Users/kasingar/stuff/python/cpython/Lib/asyncio/staggered.py", line 86, in run_one_coro
    with contextlib.suppress(futures.TimeoutError):
AttributeError: module 'asyncio.futures' has no attribute 'TimeoutError'

[asvetlov]

You are right.

Hmm, I thought that Happy Eyeballs is better tested.

[tirkarthi]

Is there a way this can be tested? I tried simulating OSError for IPv4 address and returning socket for ipv6 one like it resolves correctly. I guess that's the underlying idea of happy eyeballs but a test can be added for basic workflow since it lacks tests.

@patch_socket
def test_create_connection_happy_eyeballs_exception(self, sock_mock):
    async def getaddrinfo(*args, **kw):
        return [(2, 1, 6, '', ('127.0.0.1', 8000)),
                (2, 1, 6, '', ('::1', 8000, 0, 0)),]

    def getaddrinfo_task(*args, **kwds):
        return self.loop.create_task(getaddrinfo(*args, **kwds))

    self.loop.getaddrinfo = getaddrinfo_task
    with mock.patch.object(self.loop, 'sock_connect',
                           side_effect=[OSError, sock_mock]):
        coro = self.loop.create_connection(MyProto, host='localhost', port=8000,
                                           happy_eyeballs_delay=1)
        transport, _ = self.loop.run_until_complete(coro)
        transport.close()

[asvetlov]

The main problem of mock-based tests in asyncio is that the tests become unreliably very easy after mocked asyncio internal changes.

I'm +0 on adding the proposed test. The better idea is serving on explicit AF_INET '127.0.0.1:<port>' address when the connection is established to AF_INET6 '[::1]:<port>'.

[tirkarthi]

Sorry, I never got around writing a proper test for this so I am moving it to test needed if someone wants to volunteer for it. trio has some test cases for their happy eyeball implementation if it helps : https://github.com/python-trio/trio/blob/c5497c5ac4f6c457e3390c69cb8b5b62eca41979/trio/tests/test_highlevel_open_tcp_stream.py#L326

[kumaraditya303]

Closing in favor of #98388