Skip to content

gh-129289: fix subclass of asyncio.Task not propagating __del__() cau… #129804

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
wants to merge 4 commits into from
Closed

gh-129289: fix subclass of asyncio.Task not propagating __del__() cau… #129804

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d8f18d8
gh-129289: fix subclass of asyncio.Task not propagating __del__() cau…
tom-pytel Feb 7, 2025
347dcf7
📜🤖 Added by blurb_it.
blurb-it[bot] Feb 7, 2025
0922a32
fix news
tom-pytel Feb 7, 2025
12e3302
export _PyObject_ResurrectEndSlow
tom-pytel Feb 7, 2025
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
4 changes: 2 additions & 2 deletions Include/internal/pycore_object.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ PyAPI_DATA(Py_ssize_t) _Py_RefTotal;

extern void _Py_AddRefTotal(PyThreadState *, Py_ssize_t);
extern PyAPI_FUNC(void) _Py_IncRefTotal(PyThreadState *);
extern void _Py_DecRefTotal(PyThreadState *);
extern PyAPI_FUNC(void) _Py_DecRefTotal(PyThreadState *);

# define _Py_DEC_REFTOTAL(interp) \
interp->object_state.reftotal--
Expand Down Expand Up @@ -710,7 +710,7 @@ _PyObject_SetMaybeWeakref(PyObject *op)
}
}

extern int _PyObject_ResurrectEndSlow(PyObject *op);
extern PyAPI_FUNC(int) _PyObject_ResurrectEndSlow(PyObject *op);
#endif

// Temporarily resurrects an object during deallocation. The refcount is set
Expand Down
13 changes: 13 additions & 0 deletions Lib/test/test_asyncio/test_tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2710,6 +2710,19 @@ def __str__(self):

self.assertEqual(sys.getrefcount(obj), initial_refcount)

def test_subclass_fail_to_propagate_del(self):
# gh-129289: Fix subclass of asyncio.Task not propagating __del__() causes segfault.
class BadTask(self.Task):
def __del__(self):
pass

async def func():
return

task = BadTask(func(), loop=self.loop)

result = self.loop.run_until_complete(task)


def add_subclass_tests(cls):
BaseTask = cls.Task
Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Core_and_Builtins/2025-02-07-12-58-23.gh-issue-129289.tHkEC-.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix subclass of :class:`asyncio.Task` not propagating finalizer causes segfault.
10 changes: 10 additions & 0 deletions Modules/_asynciomodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3075,6 +3075,16 @@ TaskObj_dealloc(PyObject *self)
// resurrected.
return;
}
if (task->task_node.next != NULL) {
if (PyErr_WarnEx(PyExc_Warning, "subclasses of asyncio.Task must propagate __del__()", 1) < 0) {
PyErr_Clear();
}
_PyObject_ResurrectStart(self);
Copy link
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 you want to do it like this. You probably want PyObject_CallFinalizerFromDealloc.

Copy link
Contributor Author

@tom-pytel tom-pytel Feb 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Look above, this only happens if PyObject_CallFinalizerFromDealloc executes but the Task.__del__() was not called, so this needs to do this stuff which normally happens in PyObject_CallFinalizerFromDealloc but explicitly. Agree is not cleanest, but did not see better option.

Copy link
Member

Choose a reason for hiding this comment

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

Ah wait, I didn't see that it's already called before, but manually trying to resurrect like this seems error-prone.

Copy link
Member

Choose a reason for hiding this comment

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

I'd lean towards just emitting the warning, and then refactoring as needed to prevent a crash if the finalizer wasn't called.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As said, agreed is a bit hacky, though is used in a few other places (codeobject, dictobject, funcobject). Currently the alternative is segfault in this case so your call.

Copy link
Member

@ZeroIntensity ZeroIntensity Feb 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, and I'd also expect strings to do this as well--but that's not the point. Those are all core interpreter types, asyncio.Task isn't. I'll let @kumaraditya303 make the call here, but IMO, types should always behave in a sane way when both finalized and non-finalized.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the record, a simpler way to prevent the segfault without needing to resurrect and call the finalizer would be to just remove the task from the linked list. This has the drawback of not calling callbacks but is a less messy way to keep going. Would you prefer this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I have a better fix in works, I'll post in sometime.

TaskObj_finalize(task);
if (_PyObject_ResurrectEnd(self)) {
return;
}
}

PyTypeObject *tp = Py_TYPE(task);
PyObject_GC_UnTrack(self);
Expand Down
Loading