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.

Sign up for GitHub

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

work around the changes in 3.11, eg asyncio.TimeoutError is an OSError, and IsolatedAsyncioTestCase calls set_event_loop differently #6877

Merged
merged 7 commits into from
Aug 22, 2022
Merged

work around the changes in 3.11, eg asyncio.TimeoutError is an OSError, and IsolatedAsyncioTestCase calls set_event_loop differently #6877

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f9c86b4
try fixing AioHTTPTestCase for 3.11
graingert Aug 9, 2022
355dc8e
Update aiohttp/test_utils.py
graingert Aug 9, 2022
37fa089
add the setUp/tearDown code back to 3.7
graingert Aug 9, 2022
e5789af
disambiguate asyncio.TimeoutError from OSError
graingert Aug 9, 2022
74340cb
wrap real OSError asyncio.TimeoutErrors in a ClientOSError
graingert Aug 9, 2022
a776ee3
add changelog
graingert Aug 9, 2022
37b892c
Apply formatting to the change note
webknjaz Aug 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGES/6757.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Work around the changes in 3.11, e.g. :py:class:`~asyncio.TimeoutError` is an :py:class:`OSError`,
and :py:class:`~unittest.IsolatedAsyncioTestCase` calls :py:function:`~asyncio.set_event_loop`
differently -- by :user:`graingert`.
2 changes: 2 additions & 0 deletions aiohttp/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -532,6 +532,8 @@ async def _request(
except ClientError:
raise
except OSError as exc:
if exc.errno is None and isinstance(exc, asyncio.TimeoutError):
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain the is None check? Is it just a performance optimization? Why'd you drop the separate block idea?

Copy link
Contributor Author

@graingert graingert Aug 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

So this is for the case when you get a real OSError with an errno that's also a TimeoutError

I thought about checking for type(TimeoutError.__cause__) is CancelledError but your timeout context manager doesn't set the cause and neither does _chain_future

Then to my dismay if you do:

a, b = socket.socketpair()
with a, b:
    a.settimeout(0.1)
    a.recv(1)

You still get a TimeoutError with a None .errno so there's no way to actually tell if you have a real operating system TimeoutError or a synthetic one from asyncio

This is why trio uses trio.TooSlowError so it's a bit more clear

Copy link
Member

Choose a reason for hiding this comment

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

Maybe worth checking if asyncio.timeout() has the same problem. We could start migrating from async-timeout to that in Python 3.11+.
https://docs.python.org/3.11/library/asyncio-task.html#timeouts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would be good if we can all agree on some

class TooSlowError(asyncio.TimeoutError):
   ...

Copy link
Member

Choose a reason for hiding this comment

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

Ah, it still uses the same logic from async-timeout:
https://github.com/python/cpython/blob/main/Lib/asyncio/timeouts.py#L98

You'd have to take that up with the cpython issue tracker, or @asvetlov.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

python/cpython#95830

Copy link
Member

@Dreamsorcerer Dreamsorcerer Aug 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

So, if that gets merged, what does the code here become (assuming we've switched away from async-timeout)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixing _chain_future needs to happen first, but we can check the __cause__ to see if it's an asyncio Timeout rather than an OS timeout

Copy link
Member

Choose a reason for hiding this comment

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

Just coming back round to this, I see the change is now in Python 3.12+. Can we update these checks with a sys.version_info check, so we don't forget about it completely?

raise
raise ClientOSError(*exc.args) from exc

self._cookie_jar.update_cookies(resp.cookies, resp.url)
Expand Down
15 changes: 9 additions & 6 deletions aiohttp/client_reqrep.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -537,12 +537,15 @@ async def write_bytes(

await writer.write_eof()
except OSError as exc:
new_exc = ClientOSError(
exc.errno, "Can not write request body for %s" % self.url
)
new_exc.__context__ = exc
new_exc.__cause__ = exc
protocol.set_exception(new_exc)
if exc.errno is None and isinstance(exc, asyncio.TimeoutError):
webknjaz marked this conversation as resolved.
Show resolved Hide resolved
protocol.set_exception(exc)
else:
new_exc = ClientOSError(
exc.errno, "Can not write request body for %s" % self.url
)
new_exc.__context__ = exc
new_exc.__cause__ = exc
protocol.set_exception(new_exc)
except asyncio.CancelledError as exc:
if not conn.closed:
protocol.set_exception(exc)
Expand Down
10 changes: 10 additions & 0 deletions aiohttp/connector.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -970,6 +970,8 @@ async def _wrap_create_connection(
except ssl_errors as exc:
raise ClientConnectorSSLError(req.connection_key, exc) from exc
except OSError as exc:
if exc.errno is None and isinstance(exc, asyncio.TimeoutError):
raise
raise client_error(req.connection_key, exc) from exc

def _warn_about_tls_in_tls(
Expand Down Expand Up @@ -1049,6 +1051,8 @@ async def _start_tls_connection(
except ssl_errors as exc:
raise ClientConnectorSSLError(req.connection_key, exc) from exc
except OSError as exc:
if exc.errno is None and isinstance(exc, asyncio.TimeoutError):
raise
raise client_error(req.connection_key, exc) from exc
except TypeError as type_err:
# Example cause looks like this:
Expand Down Expand Up @@ -1100,6 +1104,8 @@ def drop_exception(fut: "asyncio.Future[List[Dict[str, Any]]]") -> None:
host_resolved.add_done_callback(drop_exception)
raise
except OSError as exc:
if exc.errno is None and isinstance(exc, asyncio.TimeoutError):
raise
# in case of proxy it is not ClientProxyConnectionError
# it is problem of resolving proxy ip itself
raise ClientConnectorError(req.connection_key, exc) from exc
Expand Down Expand Up @@ -1293,6 +1299,8 @@ async def _create_connection(
self._factory, self._path
)
except OSError as exc:
if exc.errno is None and isinstance(exc, asyncio.TimeoutError):
raise
raise UnixClientConnectorError(self.path, req.connection_key, exc) from exc

return cast(ResponseHandler, proto)
Expand Down Expand Up @@ -1358,6 +1366,8 @@ async def _create_connection(
# other option is to manually set transport like
# `proto.transport = trans`
except OSError as exc:
if exc.errno is None and isinstance(exc, asyncio.TimeoutError):
raise
raise ClientConnectorError(req.connection_key, exc) from exc

return cast(ResponseHandler, proto)
16 changes: 10 additions & 6 deletions aiohttp/test_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,12 +430,12 @@ def get_app(self) -> Application:
raise RuntimeError("Did you forget to define get_application()?")

def setUp(self) -> None:
try:
self.loop = asyncio.get_running_loop()
except RuntimeError:
self.loop = asyncio.get_event_loop_policy().get_event_loop()
Comment on lines -435 to -436
Copy link
Member

Choose a reason for hiding this comment

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

This bit needed to be recovered in aiohttp v3.8 to preserve the compatibility with Python 3.6: ef6a373.

if not PY_38:
asyncio.get_event_loop().run_until_complete(self.asyncSetUp())

self.loop.run_until_complete(self.setUpAsync())
async def asyncSetUp(self) -> None:
self.loop = asyncio.get_running_loop()
return await self.setUpAsync()

async def setUpAsync(self) -> None:
self.app = await self.get_application()
Expand All @@ -445,7 +445,11 @@ async def setUpAsync(self) -> None:
await self.client.start_server()

def tearDown(self) -> None:
self.loop.run_until_complete(self.tearDownAsync())
if not PY_38:
self.loop.run_until_complete(self.asyncTearDown())

async def asyncTearDown(self) -> None:
return await self.tearDownAsync()

async def tearDownAsync(self) -> None:
await self.client.close()
Expand Down