Skip to content

Commit b44637f

Browse files
kumaraditya303miss-islington
authored andcommitted
gh-156321: Don't eagerly log exceptions from a cancelled asyncio.shield() (GH-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. (cherry picked from commit 2095eab) Co-authored-by: Kumar Aditya <kumaraditya@python.org>
1 parent 1e1ebf6 commit b44637f

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
@@ -927,25 +927,6 @@ def _done_callback(fut, cur_task=cur_task):
927927
return outer
928928

929929

930-
def _log_on_exception(fut):
931-
if fut.cancelled():
932-
return
933-
934-
exc = fut.exception()
935-
if exc is None:
936-
return
937-
938-
context = {
939-
'message':
940-
f'{exc.__class__.__name__} exception in shielded future',
941-
'exception': exc,
942-
'future': fut,
943-
}
944-
if fut._source_traceback:
945-
context['source_traceback'] = fut._source_traceback
946-
fut._loop.call_exception_handler(context)
947-
948-
949930
def shield(arg):
950931
"""Wait for a future, shielding it from cancellation.
951932
@@ -1010,9 +991,6 @@ def _inner_done_callback(inner):
1010991
def _outer_done_callback(outer):
1011992
if not inner.done():
1012993
inner.remove_done_callback(_inner_done_callback)
1013-
# Keep only one callback to log on cancel
1014-
inner.remove_done_callback(_log_on_exception)
1015-
inner.add_done_callback(_log_on_exception)
1016994
if cur_task is not None:
1017995
inner.remove_done_callback(_clear_awaited_by_callback)
1018996
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
@@ -2148,7 +2148,7 @@ def test_shield_cancel_outer(self):
21482148
self.assertTrue(outer.cancelled())
21492149
self.assertEqual(0, 0 if outer._callbacks is None else len(outer._callbacks))
21502150
self.assertFalse(inner._asyncio_awaited_by)
2151-
self.assertTrue({f for f, _ctx in inner._callbacks or []} <= {asyncio.tasks._log_on_exception})
2151+
self.assertFalse(inner._callbacks)
21522152

21532153
def test_shield_cancel_outer_result(self):
21542154
mock_handler = mock.Mock()
@@ -2163,6 +2163,8 @@ def test_shield_cancel_outer_result(self):
21632163
mock_handler.assert_not_called()
21642164

21652165
def test_shield_cancel_outer_exception(self):
2166+
# gh-156321: an exception in the inner future must not be reported
2167+
# eagerly, as it may still be retrieved later.
21662168
mock_handler = mock.Mock()
21672169
self.loop.set_exception_handler(mock_handler)
21682170
inner = self.new_future(self.loop)
@@ -2172,7 +2174,29 @@ def test_shield_cancel_outer_exception(self):
21722174
test_utils.run_briefly(self.loop)
21732175
inner.set_exception(Exception('foo'))
21742176
test_utils.run_briefly(self.loop)
2177+
mock_handler.assert_not_called()
2178+
self.assertIsInstance(inner.exception(), Exception)
2179+
2180+
def test_shield_cancel_outer_exception_never_retrieved(self):
2181+
# gh-156321: an exception nobody retrieves is reported by the inner
2182+
# future itself when it is garbage collected, like any other future.
2183+
mock_handler = mock.Mock()
2184+
self.loop.set_exception_handler(mock_handler)
2185+
inner = self.new_future(self.loop)
2186+
outer = asyncio.shield(inner)
2187+
test_utils.run_briefly(self.loop)
2188+
outer.cancel()
2189+
test_utils.run_briefly(self.loop)
2190+
inner.set_exception(Exception('foo'))
2191+
test_utils.run_briefly(self.loop)
2192+
mock_handler.assert_not_called()
2193+
inner = None
2194+
outer = None
2195+
support.gc_collect()
21752196
mock_handler.assert_called_once()
2197+
context = mock_handler.call_args[0][1]
2198+
self.assertEndsWith(context['message'], 'exception was never retrieved')
2199+
self.assertIsInstance(context['exception'], Exception)
21762200

21772201
def test_shield_cancel_outer_in_task(self):
21782202
inner = self.new_future(self.loop)
@@ -2187,9 +2211,9 @@ async def coro():
21872211
task = self.new_task(self.loop, coro())
21882212
self.loop.run_until_complete(task)
21892213
self.assertFalse(inner._asyncio_awaited_by)
2190-
self.assertTrue({f for f, _ctx in inner._callbacks or []} <= {asyncio.tasks._log_on_exception})
2214+
self.assertFalse(inner._callbacks)
21912215

2192-
def test_shield_duplicate_log_once(self):
2216+
def test_shield_cancel_outer_twice_exception(self):
21932217
mock_handler = mock.Mock()
21942218
self.loop.set_exception_handler(mock_handler)
21952219
inner = self.new_future(self.loop)
@@ -2201,9 +2225,11 @@ def test_shield_duplicate_log_once(self):
22012225
test_utils.run_briefly(self.loop)
22022226
outer.cancel()
22032227
test_utils.run_briefly(self.loop)
2228+
self.assertFalse(inner._callbacks)
22042229
inner.set_exception(Exception('foo'))
22052230
test_utils.run_briefly(self.loop)
2206-
mock_handler.assert_called_once()
2231+
mock_handler.assert_not_called()
2232+
self.assertIsInstance(inner.exception(), Exception)
22072233

22082234
def test_shield_shortcut(self):
22092235
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)