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.

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

gh-91887: Store strong references to pending tasks #121264

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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 Include/internal/pycore_global_objects_fini_generated.h

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions Include/internal/pycore_global_strings.h
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,7 @@ struct _Py_global_strings {
STRUCT_FOR_ID(_abc_impl)
STRUCT_FOR_ID(_abstract_)
STRUCT_FOR_ID(_active)
STRUCT_FOR_ID(_add_pending_task)
STRUCT_FOR_ID(_anonymous_)
STRUCT_FOR_ID(_argtypes_)
STRUCT_FOR_ID(_as_parameter_)
Expand All @@ -234,11 +235,13 @@ struct _Py_global_strings {
STRUCT_FOR_ID(_bootstrap)
STRUCT_FOR_ID(_check_retval_)
STRUCT_FOR_ID(_dealloc_warn)
STRUCT_FOR_ID(_del_pending_task)
STRUCT_FOR_ID(_feature_version)
STRUCT_FOR_ID(_field_types)
STRUCT_FOR_ID(_fields_)
STRUCT_FOR_ID(_finalizing)
STRUCT_FOR_ID(_find_and_load)
STRUCT_FOR_ID(_finish_execution)
STRUCT_FOR_ID(_fix_up_module)
STRUCT_FOR_ID(_flags_)
STRUCT_FOR_ID(_get_sourcefile)
Expand Down
3 changes: 3 additions & 0 deletions Include/internal/pycore_runtime_init_generated.h

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 12 additions & 0 deletions Include/internal/pycore_unicodeobject_generated.h

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 14 additions & 0 deletions Lib/asyncio/base_events.py
Original file line number Diff line number Diff line change
Expand Up @@ -449,6 +449,10 @@ def __init__(self):
# Set to True when `loop.shutdown_default_executor` is called.
self._executor_shutdown_called = False

# Holds references to all pending tasks to avoid garbage
# collection (see #91887)
self._pending_tasks = set()

def __repr__(self):
return (
f'<{self.__class__.__name__} running={self.is_running()} '
Expand Down Expand Up @@ -2042,6 +2046,16 @@ def _set_coroutine_origin_tracking(self, enabled):

self._coroutine_origin_tracking_enabled = enabled

def _add_pending_task(self, task):
"""Add task to the _pending_tasks set.

This avoids garbage collection as long as the loop is alive.
"""
self._pending_tasks.add(task)

def _del_pending_task(self, task):
self._pending_tasks.discard(task)

def get_debug(self):
return self._debug

Expand Down
10 changes: 5 additions & 5 deletions Lib/asyncio/futures.py
Original file line number Diff line number Diff line change
Expand Up @@ -151,11 +151,11 @@ def cancel(self, msg=None):
return False
self._state = _CANCELLED
self._cancel_message = msg
self.__schedule_callbacks()
self._finish_execution()
return True

def __schedule_callbacks(self):
"""Internal: Ask the event loop to call all callbacks.
def _finish_execution(self):
Copy link
Member

Choose a reason for hiding this comment

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

For clarification, what's the reason for this name change? This is still, effectively, scheduling all callbacks -- "finish execution" is a bit more ambiguous to me.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, in Future, "_schedule_callbacks" is perfectly fine to describe what the function does. When overriding this function in Task, I think _finish_execution is more meaningful to indicate that it will be called when the task completes.

"""Ask the event loop to call all callbacks.

The callbacks are scheduled to be called as soon as possible. Also
clears the callback list.
Expand Down Expand Up @@ -256,7 +256,7 @@ def set_result(self, result):
raise exceptions.InvalidStateError(f'{self._state}: {self!r}')
self._result = result
self._state = _FINISHED
self.__schedule_callbacks()
self._finish_execution()

def set_exception(self, exception):
"""Mark the future done and set an exception.
Expand All @@ -278,7 +278,7 @@ def set_exception(self, exception):
self._exception = exception
self._exception_tb = exception.__traceback__
self._state = _FINISHED
self.__schedule_callbacks()
self._finish_execution()
self.__log_traceback = True

def __await__(self):
Expand Down
8 changes: 8 additions & 0 deletions Lib/asyncio/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,10 @@ def __init__(self, coro, *, loop=None, name=None, context=None,
self._loop.call_soon(self.__step, context=self._context)
_register_task(self)

def _finish_execution(self):
super()._finish_execution()
_unregister_task(self)

def __del__(self):
if self._state == futures._PENDING and self._log_destroy_pending:
context = {
Expand Down Expand Up @@ -1032,6 +1036,8 @@ def factory(loop, coro, *, name=None, context=None):
def _register_task(task):
"""Register an asyncio Task scheduled to run on an event loop."""
_scheduled_tasks.add(task)
loop = futures._get_loop(task)
loop._add_pending_task(task)


def _register_eager_task(task):
Expand Down Expand Up @@ -1067,6 +1073,8 @@ def _swap_current_task(loop, task):
def _unregister_task(task):
"""Unregister a completed, scheduled Task."""
_scheduled_tasks.discard(task)
loop = futures._get_loop(task)
loop._del_pending_task(task)


def _unregister_eager_task(task):
Expand Down
11 changes: 11 additions & 0 deletions Lib/test/test_asyncio/test_tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -2283,6 +2283,17 @@ async def kill_me(loop):
source_traceback = task._source_traceback
task = None

# no more reference to kill_me() task in user code. The task
# should be kept alive as long as it is pending and we hold a
# reference to the event loop (#91887)
support.gc_collect()

self.assertEqual(len(self.all_tasks(loop=self.loop)), 1)
mock_handler.assert_not_called()

# remove strong reference held by the event loop
self.loop._pending_tasks.clear()

# no more reference to kill_me() task: the task is destroyed by the GC
support.gc_collect()

Expand Down
1 change: 1 addition & 0 deletions Misc/ACKS
Original file line number Diff line number Diff line change
Expand Up @@ -724,6 +724,7 @@ Derek Harland
Jason Harper
David Harrigan
Brian Harring
Alexander Hartl
Jonathan Hartley
Travis B. Hartwell
Henrik Harutyunyan
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Retain strong references for pending :class:`asyncio.Task` objects to avoid rare crashes due
to background tasks. Patch by Alexander Hartl.
Loading
Loading