-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
work around the changes in 3.11, eg asyncio.TimeoutError is an OSError, and IsolatedAsyncioTestCase calls set_event_loop differently #6877
Conversation
might break it for 3.7, but probably possible to fix with a sys.version_info condition |
This comment was marked as outdated.
This comment was marked as outdated.
@webknjaz I think this fixes the issue with TestCase, but now there's a TimeoutError problem instead |
in 3.11 asyncio.TimeoutError became an OSError
I think I've been a bit too liberal with my |
@@ -532,6 +532,8 @@ async def _request( | |||
except ClientError: | |||
raise | |||
except OSError as exc: | |||
if exc.errno is None and isinstance(exc, asyncio.TimeoutError): |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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):
...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
Note to self: After this is merged, we should remove |
I think this looks fine as is, for a temporary fix to get 3.11 working. We can come back and update this with another PR once we can do the |
Backport to 3.8: 💔 cherry-picking failed — conflicts found❌ Failed to cleanly apply 5bf9b4a on top of patchback/backports/3.8/5bf9b4aae7046704b418c16452c2d1ced934abfb/pr-6877 Backporting merged PR #6877 into master
🤖 @patchback |
Backport to 3.9: 💔 cherry-picking failed — conflicts found❌ Failed to cleanly apply 5bf9b4a on top of patchback/backports/3.9/5bf9b4aae7046704b418c16452c2d1ced934abfb/pr-6877 Backporting merged PR #6877 into master
🤖 @patchback |
(cherry picked from commit 5bf9b4a)
(cherry picked from commit 5bf9b4a)
(cherry picked from commit 5bf9b4a)
except RuntimeError: | ||
self.loop = asyncio.get_event_loop_policy().get_event_loop() |
There was a problem hiding this comment.
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.
What do these changes do?
Fixes #6757
Are there changes in behavior for the user?
yeah, if someone overrides asyncSetUp/asyncTearDown in their tests it could cause confusion
Related issue number
Checklist
CONTRIBUTORS.txt
CHANGES
folder<issue_id>.<type>
for example (588.bugfix)issue_id
change it to the pr id after creating the pr.feature
: Signifying a new feature..bugfix
: Signifying a bug fix..doc
: Signifying a documentation improvement..removal
: Signifying a deprecation or removal of public API..misc
: A ticket has been closed, but it is not of interest to users.