Skip to content

gh-96037: Always insert TimeoutError when exit an expired asyncio.timeout() block #113819

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

Merged
serhiy-storchaka merged 3 commits into from
Jan 10, 2024
Merged

gh-96037: Always insert TimeoutError when exit an expired asyncio.timeout() block #113819

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
458fda0
gh-96037: Always insert TimeoutError when exit an expired asyncio.tim…
serhiy-storchaka Jan 8, 2024
4fc39e1
Address review comments.
serhiy-storchaka Jan 8, 2024
6813b05
Merge branch 'main' into asyncio-timeout-insert-timeout-error-in-context
serhiy-storchaka Jan 9, 2024
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
Prev Previous commit
Next Next commit
Address review comments.
  • Loading branch information
@serhiy-storchaka
serhiy-storchaka committed Jan 8, 2024
commit 4fc39e122ad50b0602b2d9b4768fedb3317f5a12
2 changes: 1 addition & 1 deletion Lib/asyncio/timeouts.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ async def __aexit__(
# handling this.
if exc_type is exceptions.CancelledError:

@gvanrossum gvanrossum Jan 8, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think CancelledError is final. Maybe we should switch to isinstance()?

@serhiy-storchaka serhiy-storchaka Jan 8, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Some code uses isinstance(), other code (in taskgroups.py and futures.py) uses is.

Are we going to backport this PR? If not, then perhaps it is better to change is to isinstance() in other PR before merging this PR. If yes, then we can include this change in this PR.

@gvanrossum gvanrossum Jan 8, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it's a perf hack, but it makes things harder to reason about (what will happen if I raise a subclass of CancelledError? Who knows?).

I don't think we should backport this, such changes always trip some folks over.

raise TimeoutError from exc_val
Comment thread
iritkatriel marked this conversation as resolved.
elif 10 and exc_val is not None:
elif exc_val is not None:
self._insert_timeout_error(exc_val)
if isinstance(exc_val, ExceptionGroup):
for exc in exc_val.exceptions:
Expand Down
100 changes: 61 additions & 39 deletions Lib/test/test_asyncio/test_timeouts.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,6 @@
from test.test_asyncio.utils import await_without_task


class CustomError(Exception):
pass

def tearDownModule():
asyncio.set_event_loop_policy(None)

Expand Down Expand Up @@ -127,10 +124,13 @@ async def test_timeout_exception_context(self):
finally:
await asyncio.sleep(1)
e = cm.exception
Comment thread
gvanrossum marked this conversation as resolved.
self.assertIsInstance(e.__context__, asyncio.CancelledError)
self.assertIs(e.__cause__, e.__context__)
self.assertIsInstance(e.__context__.__context__, ZeroDivisionError)
self.assertIsNone(e.__context__.__cause__)
# Expect TimeoutError caused by CancelledError raised during handling
# of ZeroDivisionError.
e2 = e.__cause__
self.assertIsInstance(e2, asyncio.CancelledError)
self.assertIs(e.__context__, e2)
self.assertIsNone(e2.__cause__)
self.assertIsInstance(e2.__context__, ZeroDivisionError)

async def test_foreign_exception_on_timeout(self):
async def crash():
Expand All @@ -142,10 +142,14 @@ async def crash():
async with asyncio.timeout(0.01):
await crash()
e = cm.exception
Comment thread
gvanrossum marked this conversation as resolved.
self.assertIsInstance(e.__context__, TimeoutError)
self.assertIsInstance(e.__context__.__context__, asyncio.CancelledError)
self.assertIs(e.__context__.__cause__, e.__context__.__context__)
# Expect ZeroDivisionError raised during handling of TimeoutError
# caused by CancelledError.
self.assertIsNone(e.__cause__)
e2 = e.__context__
self.assertIsInstance(e2, TimeoutError)
e3 = e2.__cause__
self.assertIsInstance(e3, asyncio.CancelledError)
self.assertIs(e2.__context__, e3)

async def test_foreign_exception_on_timeout_2(self):
with self.assertRaises(ZeroDivisionError) as cm:
Expand All @@ -161,15 +165,19 @@ async def test_foreign_exception_on_timeout_2(self):
finally:
1/0
e = cm.exception
Comment thread
gvanrossum marked this conversation as resolved.
self.assertIsInstance(e.__context__, KeyError)
e2 = e.__context__.__context__
self.assertIsInstance(e2, TimeoutError)
self.assertIsInstance(e2.__context__, asyncio.CancelledError)
self.assertIsInstance(e2.__context__.__context__, ValueError)
self.assertIsNone(e2.__context__.__cause__)
self.assertIsNone(e.__context__.__cause__)
self.assertIs(e2.__cause__, e2.__context__)
# Expect ZeroDivisionError raised during handling of KeyError
# raised during handling of TimeoutError caused by CancelledError.
self.assertIsNone(e.__cause__)
e2 = e.__context__
self.assertIsInstance(e2, KeyError)
self.assertIsNone(e2.__cause__)
e3 = e2.__context__
self.assertIsInstance(e3, TimeoutError)
e4 = e3.__cause__
self.assertIsInstance(e4, asyncio.CancelledError)
self.assertIsNone(e4.__cause__)
self.assertIsInstance(e4.__context__, ValueError)
self.assertIs(e3.__context__, e4)

async def test_foreign_cancel_doesnt_timeout_if_not_expired(self):
with self.assertRaises(asyncio.CancelledError):
Expand Down Expand Up @@ -273,15 +281,21 @@ async def test_nested_timeout_in_finally(self):
async with asyncio.timeout(0.01):
await asyncio.sleep(10)
e1 = cm1.exception
Comment thread
gvanrossum marked this conversation as resolved.
self.assertIsInstance(e1.__context__, asyncio.CancelledError)
self.assertIsNone(e1.__context__.__context__)
self.assertIsNone(e1.__context__.__cause__)
self.assertIs(e1.__cause__, e1.__context__)
# Expect TimeoutError caused by CancelledError.
e12 = e1.__cause__
self.assertIsInstance(e12, asyncio.CancelledError)
self.assertIsNone(e12.__cause__)
self.assertIsNone(e12.__context__)
self.assertIs(e1.__context__, e12)
e2 = cm2.exception
self.assertIsInstance(e2.__context__, asyncio.CancelledError)
self.assertIs(e2.__context__.__context__, e1.__context__)
self.assertIsNone(e2.__context__.__cause__)
self.assertIs(e2.__cause__, e2.__context__)
# Expect TimeoutError caused by CancelledError raised during
# handling of other CancelledError (which is the same as in
# the above chain).
e22 = e2.__cause__
self.assertIsInstance(e22, asyncio.CancelledError)
self.assertIsNone(e22.__cause__)
self.assertIs(e22.__context__, e12)
self.assertIs(e2.__context__, e22)

async def test_timeout_after_cancellation(self):
try:
Expand Down Expand Up @@ -367,22 +381,30 @@ async def task():
finally:
await asyncio.sleep(1)
eg = cm.exception
Comment thread
gvanrossum marked this conversation as resolved.
self.assertIsInstance(eg.__context__, TimeoutError)
self.assertIsInstance(eg.__context__.__context__, asyncio.CancelledError)
self.assertIsInstance(eg.__context__.__context__.__context__, ValueError)
self.assertIsNone(eg.__context__.__context__.__cause__)
self.assertIs(eg.__context__.__cause__, eg.__context__.__context__)
# Expect ExceptionGroup raised during handling of TimeoutError caused
# by CancelledError raised during handling of ValueError.
self.assertIsNone(eg.__cause__)
e_1 = eg.__context__
self.assertIsInstance(e_1, TimeoutError)
e_2 = e_1.__cause__
self.assertIsInstance(e_2, asyncio.CancelledError)
self.assertIsNone(e_2.__cause__)
self.assertIsInstance(e_2.__context__, ValueError)
self.assertIs(e_1.__context__, e_2)

self.assertEqual(len(eg.exceptions), 1, eg)
e = eg.exceptions[0]
self.assertIsInstance(e, ZeroDivisionError)
self.assertIsInstance(e.__context__, TimeoutError)
self.assertIsInstance(e.__context__.__context__, asyncio.CancelledError)
self.assertIsNone(e.__context__.__context__.__context__)
self.assertIsNone(e.__context__.__context__.__cause__)
self.assertIs(e.__context__.__cause__, e.__context__.__context__)
self.assertIsNone(e.__cause__)
e1 = eg.exceptions[0]
# Expect ZeroDivisionError raised during handling of TimeoutError
# caused by CancelledError (it is a different CancelledError).
self.assertIsInstance(e1, ZeroDivisionError)
self.assertIsNone(e1.__cause__)
e2 = e1.__context__
self.assertIsInstance(e2, TimeoutError)
e3 = e2.__cause__
self.assertIsInstance(e3, asyncio.CancelledError)
self.assertIsNone(e3.__context__)
self.assertIsNone(e3.__cause__)
self.assertIs(e2.__context__, e3)


if __name__ == '__main__':
Expand Down