Skip to content

Commit 2095eab

Browse files
gh-156321: Don't eagerly log exceptions from a cancelled asyncio.shield() (#158590)
When the outer future returned by shield() was cancelled, a callback was added to the inner future that reported its exception through the loop exception handler as soon as it completed, even if the exception was retrieved afterwards by awaiting the inner future or calling `exception()`. Remove the callback; an exception nobody retrieves is still reported by the inner future itself when it is garbage collected, like any other future.
1 parent 5c7445b commit 2095eab

3 files changed

Lines changed: 35 additions & 26 deletions

File tree

‎Lib/asyncio/tasks.py‎

Lines changed: 0 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -938,25 +938,6 @@ def _done_callback(fut, cur_task=cur_task):
938938
return outer
939939

940940

941-
def _log_on_exception(fut):
942-
if fut.cancelled():
943-
return
944-
945-
exc = fut.exception()
946-
if exc is None:
947-
return
948-
949-
context = {
950-
'message':
951-
f'{exc.__class__.__name__} exception in shielded future',
952-
'exception': exc,
953-
'future': fut,
954-
}
955-
if fut._source_traceback:
956-
context['source_traceback'] = fut._source_traceback
957-
fut._loop.call_exception_handler(context)
958-
959-
960941
def shield(arg):
961942
"""Wait for a future, shielding it from cancellation.
962943
@@ -1021,9 +1002,6 @@ def _inner_done_callback(inner):
10211002
def _outer_done_callback(outer):
10221003
if not inner.done():
10231004
inner.remove_done_callback(_inner_done_callback)
1024-
# Keep only one callback to log on cancel
1025-
inner.remove_done_callback(_log_on_exception)
1026-
inner.add_done_callback(_log_on_exception)
10271005
if cur_task is not None:
10281006
inner.remove_done_callback(_clear_awaited_by_callback)
10291007
futures.future_discard_from_awaited_by(inner, cur_task)

‎Lib/test/test_asyncio/test_tasks.py‎

Lines changed: 30 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2153,7 +2153,7 @@ def test_shield_cancel_outer(self):
21532153
self.assertTrue(outer.cancelled())
21542154
self.assertEqual(0, 0 if outer._callbacks is None else len(outer._callbacks))
21552155
self.assertFalse(inner._asyncio_awaited_by)
2156-
self.assertTrue({f for f, _ctx in inner._callbacks or []} <= {asyncio.tasks._log_on_exception})
2156+
self.assertFalse(inner._callbacks)
21572157

21582158
def test_shield_cancel_outer_result(self):
21592159
mock_handler = mock.Mock()
@@ -2168,6 +2168,8 @@ def test_shield_cancel_outer_result(self):
21682168
mock_handler.assert_not_called()
21692169

21702170
def test_shield_cancel_outer_exception(self):
2171+
# gh-156321: an exception in the inner future must not be reported
2172+
# eagerly, as it may still be retrieved later.
21712173
mock_handler = mock.Mock()
21722174
self.loop.set_exception_handler(mock_handler)
21732175
inner = self.new_future(self.loop)
@@ -2177,7 +2179,29 @@ def test_shield_cancel_outer_exception(self):
21772179
test_utils.run_briefly(self.loop)
21782180
inner.set_exception(Exception('foo'))
21792181
test_utils.run_briefly(self.loop)
2182+
mock_handler.assert_not_called()
2183+
self.assertIsInstance(inner.exception(), Exception)
2184+
2185+
def test_shield_cancel_outer_exception_never_retrieved(self):
2186+
# gh-156321: an exception nobody retrieves is reported by the inner
2187+
# future itself when it is garbage collected, like any other future.
2188+
mock_handler = mock.Mock()
2189+
self.loop.set_exception_handler(mock_handler)
2190+
inner = self.new_future(self.loop)
2191+
outer = asyncio.shield(inner)
2192+
test_utils.run_briefly(self.loop)
2193+
outer.cancel()
2194+
test_utils.run_briefly(self.loop)
2195+
inner.set_exception(Exception('foo'))
2196+
test_utils.run_briefly(self.loop)
2197+
mock_handler.assert_not_called()
2198+
inner = None
2199+
outer = None
2200+
support.gc_collect()
21802201
mock_handler.assert_called_once()
2202+
context = mock_handler.call_args[0][1]
2203+
self.assertEndsWith(context['message'], 'exception was never retrieved')
2204+
self.assertIsInstance(context['exception'], Exception)
21812205

21822206
def test_shield_cancel_outer_in_task(self):
21832207
inner = self.new_future(self.loop)
@@ -2192,9 +2216,9 @@ async def coro():
21922216
task = self.new_task(self.loop, coro())
21932217
self.loop.run_until_complete(task)
21942218
self.assertFalse(inner._asyncio_awaited_by)
2195-
self.assertTrue({f for f, _ctx in inner._callbacks or []} <= {asyncio.tasks._log_on_exception})
2219+
self.assertFalse(inner._callbacks)
21962220

2197-
def test_shield_duplicate_log_once(self):
2221+
def test_shield_cancel_outer_twice_exception(self):
21982222
mock_handler = mock.Mock()
21992223
self.loop.set_exception_handler(mock_handler)
22002224
inner = self.new_future(self.loop)
@@ -2206,9 +2230,11 @@ def test_shield_duplicate_log_once(self):
22062230
test_utils.run_briefly(self.loop)
22072231
outer.cancel()
22082232
test_utils.run_briefly(self.loop)
2233+
self.assertFalse(inner._callbacks)
22092234
inner.set_exception(Exception('foo'))
22102235
test_utils.run_briefly(self.loop)
2211-
mock_handler.assert_called_once()
2236+
mock_handler.assert_not_called()
2237+
self.assertIsInstance(inner.exception(), Exception)
22122238

22132239
def test_shield_shortcut(self):
22142240
fut = self.new_future(self.loop)
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
Fix :func:`asyncio.shield` reporting an exception from the inner future
2+
through the loop exception handler as soon as it completes after the shield
3+
was cancelled, even when the exception is retrieved afterwards. The
4+
exception is now reported only if it is never retrieved, as with any other
5+
future.

0 commit comments

Comments
 (0)