pymethods: add support for __del__

[messense]

Closes #2479

[messense]

Not sure what's the best way to test it.

It looks like the tp_dealloc in that file also calls tp_finalize in some circumstances; we might need to check carefully that we reproduce the Python behaviour correctly.

@davidhewitt Could you elaborate on how do we check that?

[davidhewitt]

Good questions.

For testing, you could also potentially just let the reference count of the object go to zero at which point I think all versions of CPython which we support will call tp_finalize pretty much immediately.

In pytests module it might be worth writing a Python and Rust version of the same class and confirming that their __del__ method works the same.

Note also that the Python docs mention that __del__ can increase the reference count of the object and "resurrect" it. It might be worth adding a test that this works correctly? Not sure.

[davidhewitt]

For tp_dealloc - I note that here in the CPython tp_dealloc implementation for Python classes it calls tp_finalize. We might need to modify the tp_dealloc we have in our PyCell implementation to do this.

...or maybe we should be removing our custom tp_dealloc and moving all logic to tp_finalize ??

[davidhewitt]

Thanks! Please also add a CHANGELOG and add to protocols.md in the guide.

Note that also on Python 3.7 we need to set Py_TPFLAGS_HAVE_FINALIZE, it's ignored on newer versions.

[davidhewitt]

Good questions.

For testing, you could also potentially just let the reference count of the object go to zero at which point I think all versions of CPython which we support will call tp_finalize pretty much immediately.

In pytests module it might be worth writing a Python and Rust version of the same class and confirming that their __del__ method works the same.

Note also that the Python docs mention that __del__ can increase the reference count of the object and "resurrect" it. It might be worth adding a test that this works correctly? Not sure.

[davidhewitt]

As per CPython docs, exceptions in __del__ are ignored and print a warning. It would be cool to check that if we return PyResult we get the same behaviour. We could add a test to the pytests crate which checks that sys.unraiseablehook is called, I think.

https://github.com/python/cpython/blob/71868a0066a90519ccc6aeb75673ae882aaa03f0/Objects/typeobject.c#L8047

[davidhewitt]

For tp_dealloc - I note that here in the CPython tp_dealloc implementation for Python classes it calls tp_finalize. We might need to modify the tp_dealloc we have in our PyCell implementation to do this.

...or maybe we should be removing our custom tp_dealloc and moving all logic to tp_finalize ??

[messense]

This test case does not pass locally, adding a panic!() to __del__ seems to indicate that __del__ is not called as it did not trigger a panic.

[davidhewitt]

As per #2484 (comment) this indicates to me that we may want to consider adding a block similar to the following to PyCell::tp_dealloc

https://github.com/python/cpython/blob/56310136170ef6653cb050f58dd2d32538997f59/Modules/_asynciomodule.c#L1534-L1542

But PyObject_CallFinalizerFromDealloc is only in the limited API, so it might be that we can only support __del__ when not running abi3.

[messense]

That's unfortunate.

...or maybe we should be removing our custom tp_dealloc and moving all logic to tp_finalize ??

Could we do this instead so it does not require calling PyObject_CallFinalizerFromDealloc in extension module?

[davidhewitt]

Yes, that seems reasonable to me. I have no idea whether it will definitely work. I plan to play around with this once the 0.17 release is complete. (Feel free to experiment sooner if you want!)

[davidhewitt]

I found today that if we don't implement tp_dealloc then the default tp_dealloc implementation (at least for CPython) will call tp_finalize. This gives us a potential route to support this and also fix #3064 at the same time.

However the documentation in this area is confusing, as it implies that tp_dealloc doesn't have a default - python/cpython#105018 / https://docs.python.org/3/c-api/typeobj.html#c.PyTypeObject.tp_dealloc

I think it may be possible that we use that default implementation to support this case, but we'll need to test carefully whether this also works for all Python versions, PyPy etc.

[davidhewitt]

I spoke to vstinner (thought no need to ping so no @) briefly about this, I understood that PyObject_CallFinalizerFromDealloc could be added to the limited API in 3.13. So maybe we can figure out a way to proceed with this where __del__ is supported without abi3 for now and then we can add abi3 support on 3.13.