gh-129289: fix subclass of asyncio.Task not propagating __del__() cau… #129804
Conversation
…() causes segfault
tom-pytel
requested review from
1st1,
asvetlov,
kumaraditya303 and
willingc
as code owners
| if (PyErr_WarnEx(PyExc_Warning, "subclasses of asyncio.Task must propagate __del__()", 1) < 0) { | ||
| PyErr_Clear(); | ||
| } | ||
| _PyObject_ResurrectStart(self); |
There was a problem hiding this comment.
I don't think you want to do it like this. You probably want PyObject_CallFinalizerFromDealloc.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ah wait, I didn't see that it's already called before, but manually trying to resurrect like this seems error-prone.
There was a problem hiding this comment.
I'd lean towards just emitting the warning, and then refactoring as needed to prevent a crash if the finalizer wasn't called.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I have a better fix in works, I'll post in sometime.
3 participants
Its been a bit and I didn't see anyone pick this up, so at least fix the segfault in this case.