Skip to content

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

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

Closed
alexhartl wants to merge 5 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c229526
Store strong references to pending tasks
alexhartl Jul 2, 2024
bbdbce1
NEWS, ACKs
alexhartl Jul 2, 2024
9a5ada0
Improved news item
alexhartl Jul 11, 2024
5d2b57e
Add test code to ensure #91887 is fixed
alexhartl Jul 12, 2024
eb89d4c
Merge branch 'main' into strong-refs-for-bg-tasks
freakboy3742 Sep 26, 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
Merge branch 'main' into strong-refs-for-bg-tasks
  • Loading branch information
@freakboy3742
freakboy3742 committed Sep 27, 2024
commit eb89d4cc3b3557dc04ea75b66597f2883df624f7
3 changes: 0 additions & 3 deletions Include/internal/pycore_global_objects_fini_generated.h
View file
Open in desktop

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

3 changes: 0 additions & 3 deletions Include/internal/pycore_global_strings.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,6 @@ struct _Py_global_strings {
STRUCT_FOR_ID(_abstract_)
STRUCT_FOR_ID(_active)
STRUCT_FOR_ID(_add_pending_task)
STRUCT_FOR_ID(_align_)
STRUCT_FOR_ID(_anonymous_)
STRUCT_FOR_ID(_argtypes_)
STRUCT_FOR_ID(_as_parameter_)
Expand Down Expand Up @@ -256,8 +255,6 @@ struct _Py_global_strings {
STRUCT_FOR_ID(_loop)
STRUCT_FOR_ID(_needs_com_addref_)
STRUCT_FOR_ID(_only_immortal)
STRUCT_FOR_ID(_pack_)
STRUCT_FOR_ID(_pending_tasks)
STRUCT_FOR_ID(_restype_)
STRUCT_FOR_ID(_showwarnmsg)
STRUCT_FOR_ID(_shutdown)
Expand Down
3 changes: 0 additions & 3 deletions Include/internal/pycore_runtime_init_generated.h
View file
Open in desktop

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

12 changes: 0 additions & 12 deletions Include/internal/pycore_unicodeobject_generated.h
View file
Open in desktop

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

6 changes: 3 additions & 3 deletions Lib/test/test_asyncio/test_tasks.py
View file
Open in desktop

@ZeroIntensity ZeroIntensity Jul 10, 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.

It might be a good idea to add a test to make sure that this actually fixes #91887, to make sure that someone doesn't accidentally break this in the future.

Likely would be something like:

def test_strong_task_references(self):
    called = False

    async def coro():
        nonlocal called
        called = True

    async def main():
        asyncio.create_task(coro())

    loop = asyncio.new_event_loop()
    try:
        loop.run_until_complete(main())
    finally:
        loop.close()
        self.assertTrue(called)

Original file line number Diff line number Diff line change
Expand Up @@ -2282,13 +2282,13 @@ async def kill_me(loop):
coro = None
source_traceback = task._source_traceback
task = None
# no more reference to kill_me() task in user code. the task

# 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(asyncio.all_tasks(loop=self.loop)), 1)
self.assertEqual(len(self.all_tasks(loop=self.loop)), 1)
mock_handler.assert_not_called()

# remove strong reference held by the event loop
Expand Down
185 changes: 114 additions & 71 deletions Modules/_asynciomodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,7 +459,7 @@ future_schedule_callbacks(asyncio_state *state, FutureObj *fut)
}

static inline int
future_call_finish_execution(FutureObj *fut)
future_finish_execution(FutureObj *fut)
{
PyObject *res = PyObject_CallMethodNoArgs((PyObject *)fut,
&_Py_ID(_finish_execution));
Expand Down Expand Up @@ -545,7 +545,7 @@ future_set_result(asyncio_state *state, FutureObj *fut, PyObject *res)
fut->fut_result = Py_NewRef(res);
fut->fut_state = STATE_FINISHED;

if (future_call_finish_execution(fut) < 0) {
if (future_finish_execution(fut) < 0) {
return NULL;
}
Py_RETURN_NONE;
Expand Down Expand Up @@ -609,7 +609,7 @@ future_set_exception(asyncio_state *state, FutureObj *fut, PyObject *exc)
fut->fut_exception_tb = PyException_GetTraceback(exc_val);
fut->fut_state = STATE_FINISHED;

if (future_call_finish_execution(fut) < 0) {
if (future_finish_execution(fut) < 0) {
return NULL;
}

Expand Down Expand Up @@ -767,7 +767,7 @@ future_cancel(asyncio_state *state, FutureObj *fut, PyObject *msg)
Py_XINCREF(msg);
Py_XSETREF(fut->fut_cancel_msg, msg);

if (future_call_finish_execution(fut) < 0) {
if (future_finish_execution(fut) < 0) {
return NULL;
}
Py_RETURN_TRUE;
Expand Down Expand Up @@ -1948,28 +1948,61 @@ static PyMethodDef TaskWakeupDef = {
/* ----- Task introspection helpers */

static void
register_task(asyncio_state *state, TaskObj *task)
register_task(asyncio_state *state, PyObject *pytask)
{
PyObject *res;

ASYNCIO_STATE_LOCK(state);
assert(Task_Check(state, task));
assert(task != &state->asyncio_tasks.tail);
if (task->next != NULL) {
// already registered
goto exit;
}
Py_DECREF(res);
if (Task_Check(state, pytask)) {
// If task is an asyncio.Task instance or subclass, use efficient
// linked-list implementation.
TaskObj *task = (TaskObj *) pytask;
assert(task != &state->asyncio_tasks.tail);
if (task->next != NULL) {
// already registered
goto exit;
}
assert(task->prev == NULL);
assert(state->asyncio_tasks.head != NULL);

task->next = state->asyncio_tasks.head;
state->asyncio_tasks.head->prev = task;
state->asyncio_tasks.head = task;

// Ensure there is a persistent reference to the pending task
res = PyObject_CallMethodOneArg(task->task_loop,
&_Py_ID(_add_pending_task), pytask);
if (res == NULL) {
goto exit;
}
Py_DECREF(res);
} else {
// As task does not inherit from asyncio.Task, fallback to less efficient
// weakset implementation.
res = PyObject_CallMethodOneArg(state->non_asyncio_tasks, &_Py_ID(add), pytask);
if (res == NULL) {
goto exit;
}
Py_DECREF(res);

PyObject *loop = get_future_loop(state, task);
if (loop == NULL) {
return -1;
}
res = PyObject_CallMethodOneArg(loop, &_Py_ID(_add_pending_task), task);
Py_DECREF(loop);
if (res == NULL) {
return -1;
// Ensure there is a persistent reference to the pending task
// Get a reference to the task loop
PyObject *task_loop = get_future_loop(state, pytask);
if (task_loop == NULL) {
goto exit;
}

res = PyObject_CallMethodOneArg(task_loop,
&_Py_ID(_add_pending_task), pytask);
Py_DECREF(task_loop);
if (res == NULL) {
goto exit;
}
Py_DECREF(res);
}
Py_DECREF(res);
return 0;

exit:
ASYNCIO_STATE_UNLOCK(state);
}

static int
Expand All @@ -1979,28 +2012,62 @@ register_eager_task(asyncio_state *state, PyObject *task)
}

static void
unregister_task(asyncio_state *state, TaskObj *task)
unregister_task(asyncio_state *state, PyObject *pytask)
{
PyObject *loop = get_future_loop(state, task);
if (loop == NULL) {
return -1;
}
PyObject *res = PyObject_CallMethodOneArg(loop, &_Py_ID(_del_pending_task),
task);
Py_DECREF(loop);
if (res == NULL) {
return -1;
}
task->next->prev = task->prev;
if (task->prev == NULL) {
assert(state->asyncio_tasks.head == task);
state->asyncio_tasks.head = task->next;
PyObject *res;

ASYNCIO_STATE_LOCK(state);
if (Task_Check(state, pytask)) {
TaskObj *task = (TaskObj *) pytask;

assert(task != &state->asyncio_tasks.tail);
if (task->next == NULL) {
// not registered
assert(task->prev == NULL);
assert(state->asyncio_tasks.head != task);
goto exit;
}
task->next->prev = task->prev;
if (task->prev == NULL) {
assert(state->asyncio_tasks.head == task);
state->asyncio_tasks.head = task->next;
} else {
task->prev->next = task->next;
}
task->next = NULL;
task->prev = NULL;
assert(state->asyncio_tasks.head != task);

// Delete the persistent reference to the pending task
res = PyObject_CallMethodOneArg(task->task_loop,
&_Py_ID(_del_pending_task), pytask);
if (res == NULL) {
goto exit;
}
Py_DECREF(res);
} else {
task->prev->next = task->next;
res = PyObject_CallMethodOneArg(state->non_asyncio_tasks,
&_Py_ID(discard), pytask);
if (res == NULL) {
goto exit;
}
Py_DECREF(res);

// Delete the persistent reference to the pending task
// Get a reference to the task loop
PyObject *task_loop = get_future_loop(state, pytask);
if (task_loop == NULL) {
goto exit;
}

res = PyObject_CallMethodOneArg(task_loop, &_Py_ID(_del_pending_task), pytask);
Py_DECREF(task_loop);
if (res == NULL) {
goto exit;
}
Py_DECREF(res);
}
task->next = NULL;
task->prev = NULL;
assert(state->asyncio_tasks.head != task);

exit:
ASYNCIO_STATE_UNLOCK(state);
}
Expand Down Expand Up @@ -2201,7 +2268,7 @@ _asyncio_Task___init___impl(TaskObj *self, PyObject *coro, PyObject *loop,
if (task_call_step_soon(state, self, NULL)) {
return -1;
}
register_task(state, self);
register_task(state, (PyObject *)self);
return 0;
}

Expand Down Expand Up @@ -2625,9 +2692,7 @@ _asyncio_Task__finish_execution_impl(TaskObj *self)
if (future_schedule_callbacks(state, (FutureObj *)self) < 0) {
return NULL;
}
if (unregister_task(state, (PyObject*) self) < 0) {
return NULL;
}
unregister_task(state, (PyObject*)self);
Py_RETURN_NONE;
}

Expand All @@ -2642,7 +2707,7 @@ TaskObj_finalize(TaskObj *task)
// should use the asyncio._unregister_task function.
// See https://docs.python.org/3/library/asyncio-extending.html#task-lifetime-support

unregister_task(state, task);
unregister_task(state, (PyObject *)task);

PyObject *context;
PyObject *message = NULL;
Expand Down Expand Up @@ -3256,7 +3321,7 @@ task_eager_start(asyncio_state *state, TaskObj *task)
}

if (task->task_state == STATE_PENDING) {
register_task(state, task);
register_task(state, (PyObject *)task);
} else {
// This seems to really help performance on pyperformance benchmarks
Py_CLEAR(task->task_coro);
Expand Down Expand Up @@ -3419,20 +3484,7 @@ _asyncio__register_task_impl(PyObject *module, PyObject *task)
/*[clinic end generated code: output=8672dadd69a7d4e2 input=21075aaea14dfbad]*/
{
asyncio_state *state = get_asyncio_state(module);
if (Task_Check(state, task)) {
// task is an asyncio.Task instance or subclass, use efficient
// linked-list implementation.
register_task(state, (TaskObj *)task);
Py_RETURN_NONE;
}
// As task does not inherit from asyncio.Task, fallback to less efficient
// weakset implementation.
PyObject *res = PyObject_CallMethodOneArg(state->non_asyncio_tasks,
&_Py_ID(add), task);
if (res == NULL) {
return NULL;
}
Py_DECREF(res);
register_task(state, task);
Py_RETURN_NONE;
}

Expand Down Expand Up @@ -3473,16 +3525,7 @@ _asyncio__unregister_task_impl(PyObject *module, PyObject *task)
/*[clinic end generated code: output=6e5585706d568a46 input=28fb98c3975f7bdc]*/
{
asyncio_state *state = get_asyncio_state(module);
if (Task_Check(state, task)) {
unregister_task(state, (TaskObj *)task);
Py_RETURN_NONE;
}
PyObject *res = PyObject_CallMethodOneArg(state->non_asyncio_tasks,
&_Py_ID(discard), task);
if (res == NULL) {
return NULL;
}
Py_DECREF(res);
unregister_task(state, task);
Py_RETURN_NONE;
}

Expand Down
Loading
You are viewing a condensed version of this merge commit. You can view the full changes here.