Deprecate Py_TRASHCAN_SAFE_BEGIN/END in 3.10?
Re https://bugs.python.org/issue40608. I think it will be an act of kindness to deprecate Py_TRASHCAN_SAFE_BEGIN/END in 3.10 and tell people to use Py_TRASHCAN_BEGIN/END instead. TL;DR: There was a change in 3.8 that introduced the latter while leaving the former for backwards compatibility, but also inadvertently breaking them. This is not an easy bug to deal with in the wild, we found it because we have a unit test in our codebase referencing https:// bugs.python.org/issue16602. A deprecation note pointing to the new macros would have made it easier. Is there any reason not to deprecate the old macros? Irit
Hi everybody, I'd like to revive this thread as I feel like we have to do something here but some consensus is needed first. To recap, the current state of things is as follows: - in March 2000 (d724b23420f) Christian Tismer contributed the "trashcan" patch that added Py_TRASHCAN_SAFE_BEGIN and Py_TRASHCAN_SAFE_END macros which allow destroying nested objects non-recursively. - in May 2019 (GH-11841 of BPO-35983) Antoine Pitrou merged a change by Jeroen Demeyer which made Py_TRASHCAN_SAFE_BEGIN/END (unintentionally?) backwards incompatible; this was released in Python 3.8.0. - by the way, GH-11841 introduced a new pair of macros (because they have different signatures) called simply Py_TRASHCAN_BEGIN and Py_TRASHCAN_END. - by that time there was already a follow-up PR open (GH-12607) to improve backwards compatibility of the macros, as well as introduce tests for them; this was never merged. - in Feb 2020 (0fa4f43db08) Victor Stinner removed the trashcan mechanism from the limited C API (note: not ABI, those are macros) since it accesses fields of structs not exposed in the limited C API; this was released in Python 3.9.0. - in May 2020 Irit noticed that the backwards incompatibility (BPO-40608) causes segfaults for C API code that worked fine with Python 3.7. Using the new macros requires code changes but doesn't crash. Now, there are a couple of things we can do here: Option 1: Finish GH-12607 to fix the old macros, keeping in mind this will restore compatibility lost with Python 3.8 - 3.10 only for users of 3.11+ Option 2: Review and merge GH-20104 that reverts the macro changes that make old client code segfault -- unclear what else this needs and again, that would only fix it for users of 3.11+ Option 3: Abandon GH-12607 and GH-20104, instead declaring the old macros deprecated for 3.11 and remove them in 3.13 I personally agree with Irit, voting +1 for Option 3 since the old macros were soft-deprecated already by introducing new macros in 3.8, and more importantly made incompatible with pre-3.8 usage. Let's talk on how to proceed. - Ł
On 26 Apr 2021, at 23:55, Irit Katriel via Python-Dev <python-dev@python.org> wrote:
Re https://bugs.python.org/issue40608 <https://bugs.python.org/issue40608>.
I think it will be an act of kindness to deprecate Py_TRASHCAN_SAFE_BEGIN/END in 3.10 and tell people to use Py_TRASHCAN_BEGIN/END instead.
TL;DR: There was a change in 3.8 that introduced the latter while leaving the former for backwards compatibility, but also inadvertently breaking them. This is not an easy bug to deal with in the wild, we found it because we have a unit test in our codebase referencing https://bugs.python.org/issue16602 <http://bugs.python.org/issue16602>. A deprecation note pointing to the new macros would have made it easier.
Is there any reason not to deprecate the old macros?
Irit _______________________________________________ Python-Dev mailing list -- python-dev@python.org To unsubscribe send an email to python-dev-leave@python.org https://mail.python.org/mailman3/lists/python-dev.python.org/ Message archived at https://mail.python.org/archives/list/python-dev@python.org/message/LWP6MOLP... Code of Conduct: http://python.org/psf/codeofconduct/
On Tue, 17 Aug 2021 12:00:11 +0200 Łukasz Langa <lukasz@langa.pl> wrote:
Now, there are a couple of things we can do here: Option 1: Finish GH-12607 to fix the old macros, keeping in mind this will restore compatibility lost with Python 3.8 - 3.10 only for users of 3.11+ Option 2: Review and merge GH-20104 that reverts the macro changes that make old client code segfault -- unclear what else this needs and again, that would only fix it for users of 3.11+ Option 3: Abandon GH-12607 and GH-20104, instead declaring the old macros deprecated for 3.11 and remove them in 3.13
I personally agree with Irit, voting +1 for Option 3 since the old macros were soft-deprecated already by introducing new macros in 3.8, and more importantly made incompatible with pre-3.8 usage.
Agreed as well. Regards Antoine.
I don't know if it is pertinent to this at all, but I raised https://bugs.python.org/issue44449 in which the faulthandler module can lead to a segfault inside Py_TRASHCAN_SAFE_BEGIN. Would that be avoided if frameobject.c was changed to use Py_TRASHCAN_BEGIN / END? Duncan. On Tue, 2021-08-17 at 12:00 +0200, Łukasz Langa wrote:
Hi everybody, I'd like to revive this thread as I feel like we have to do something here but some consensus is needed first.
To recap, the current state of things is as follows: - in March 2000 (d724b23420f) Christian Tismer contributed the "trashcan" patch that added Py_TRASHCAN_SAFE_BEGIN and Py_TRASHCAN_SAFE_END macros which allow destroying nested objects non-recursively. - in May 2019 (GH-11841 of BPO-35983) Antoine Pitrou merged a change by Jeroen Demeyer which made Py_TRASHCAN_SAFE_BEGIN/END (unintentionally?) backwards incompatible; this was released in Python 3.8.0. - by the way, GH-11841 introduced a new pair of macros (because they have different signatures) called simply Py_TRASHCAN_BEGIN and Py_TRASHCAN_END. - by that time there was already a follow-up PR open (GH-12607) to improve backwards compatibility of the macros, as well as introduce tests for them; this was never merged. - in Feb 2020 (0fa4f43db08) Victor Stinner removed the trashcan mechanism from the limited C API (note: not ABI, those are macros) since it accesses fields of structs not exposed in the limited C API; this was released in Python 3.9.0. - in May 2020 Irit noticed that the backwards incompatibility (BPO- 40608) causes segfaults for C API code that worked fine with Python 3.7. Using the new macros requires code changes but doesn't crash.
Now, there are a couple of things we can do here: Option 1: Finish GH-12607 to fix the old macros, keeping in mind this will restore compatibility lost with Python 3.8 - 3.10 only for users of 3.11+ Option 2: Review and merge GH-20104 that reverts the macro changes that make old client code segfault -- unclear what else this needs and again, that would only fix it for users of 3.11+ Option 3: Abandon GH-12607 and GH-20104, instead declaring the old macros deprecated for 3.11 and remove them in 3.13
I personally agree with Irit, voting +1 for Option 3 since the old macros were soft-deprecated already by introducing new macros in 3.8, and more importantly made incompatible with pre-3.8 usage.
Let's talk on how to proceed.
- Ł
-- Duncan Grisby <duncan@grisby.org>
On Tue, Aug 17, 2021 at 12:05 PM Duncan Grisby <duncan@grisby.org> wrote:
I don't know if it is pertinent to this at all, but I raised https://bugs.python.org/issue44449 in which the faulthandler module can lead to a segfault inside Py_TRASHCAN_SAFE_BEGIN. Would that be avoided if frameobject.c was changed to use Py_TRASHCAN_BEGIN / END?
We just changed frameobject.c to use the new macros. Can you check? https://github.com/python/cpython/pull/27683
On Tue, 2021-08-17 at 12:14 +0100, Irit Katriel wrote:
On Tue, Aug 17, 2021 at 12:05 PM Duncan Grisby <duncan@grisby.org> wrote:
I don't know if it is pertinent to this at all, but I raised https://bugs.python.org/issue44449 in which the faulthandler module can lead to a segfault inside Py_TRASHCAN_SAFE_BEGIN. Would that be avoided if frameobject.c was changed to use Py_TRASHCAN_BEGIN / END?
We just changed frameobject.c to use the new macros. Can you check? https://github.com/python/cpython/pull/27683
Thanks. Victor Stinner has now commented on the defect I raised to say that it is actually a bug in the traceback module, in that it should not be using Py_DECREF at all inside a signal handler. The fact that it leads to use of the old trashcan macros is not the cause of the crash that I saw. I therefore don't think it's necessary for me to check with the updated frameobject.c. Do you agree? I can try to reproduce the situation, but I can't just quickly test it because the infrastructure where the crash was seen is now being used for other things. Duncan. -- Duncan Grisby <duncan@grisby.org>
On Thu, Aug 19, 2021 at 11:28 AM Duncan Grisby
Thanks. Victor Stinner has now commented on the defect I raised to say that it is actually a bug in the traceback module, in that it should not be using Py_DECREF at all inside a signal handler. The fact that it leads to use of the old trashcan macros is not the cause of the crash that I saw.
I therefore don't think it's necessary for me to check with the updated frameobject.c. Do you agree? I can try to reproduce the situation, but I can't just quickly test it because the infrastructure where the crash was seen is now being used for other things.
Well, in order to verify that we fixed the problem you reported we need to be able to test the fix and see that the problem is gone. Right now we have two theories of what the cause could be, because there are two known bugs in that area. But you may have stumbled upon an unrelated third bug. The best scenario is that we close a bpo issue because we can reproduce the bug and see that it has been fixed. When that's not possible, there will come a point where we will close it and say "the code has changed since you observed the problem and we fixed things that could have caused it. Please open a new issue if you still see this bug".
On Thu, 2021-08-19 at 11:41 +0100, Irit Katriel wrote:
Well, in order to verify that we fixed the problem you reported we need to be able to test the fix and see that the problem is gone. Right now we have two theories of what the cause could be, because there are two known bugs in that area. But you may have stumbled upon an unrelated third bug.
The best scenario is that we close a bpo issue because we can reproduce the bug and see that it has been fixed. When that's not possible, there will come a point where we will close it and say "the code has changed since you observed the problem and we fixed things that could have caused it. Please open a new issue if you still see this bug".
That's fair. I'll do my best to reproduce it and see which (if either) change resolves it. Duncan. -- Duncan Grisby <duncan@grisby.org>
On 17. 08. 21 12:00, Łukasz Langa wrote:
Hi everybody, I'd like to revive this thread as I feel like we have to do something here but some consensus is needed first.
To recap, the current state of things is as follows: - *in March 2000* (d724b23420f) Christian Tismer contributed the "trashcan" patch that added Py_TRASHCAN_SAFE_BEGIN and Py_TRASHCAN_SAFE_END macros which allow destroying nested objects non-recursively. - *in May 2019* (GH-11841 of BPO-35983) Antoine Pitrou merged a change by Jeroen Demeyer which made Py_TRASHCAN_SAFE_BEGIN/END (unintentionally?) backwards incompatible; this was released in Python 3.8.0. - by the way, GH-11841 introduced a new pair of macros (because they have different signatures) called simply Py_TRASHCAN_BEGIN and Py_TRASHCAN_END. - by that time there was already a follow-up PR open (GH-12607) to improve backwards compatibility of the macros, as well as introduce tests for them; this was never merged. - *in Feb 2020* (0fa4f43db08) Victor Stinner removed the trashcan mechanism from the limited C API (note: not ABI, those are macros) since it accesses fields of structs not exposed in the limited C API; this was released in Python 3.9.0. - *in May 2020* Irit noticed that the backwards incompatibility (BPO-40608) causes segfaults for C API code that worked fine with Python 3.7. Using the new macros requires code changes but doesn't crash.
Now, there are a couple of things we can do here: *Option 1*: Finish GH-12607 to fix the old macros, keeping in mind this will restore compatibility lost with Python 3.8 - 3.10 only for users of 3.11+ *Option 2*: Review and merge GH-20104 that reverts the macro changes that make old client code segfault -- unclear what else this needs and again, that would only fix it for users of 3.11+ *Option 3*: Abandon GH-12607 and GH-20104, instead declaring the old macros deprecated for 3.11 and remove them in 3.13
I personally agree with Irit, voting +1 for Option 3 since the old macros were soft-deprecated already by introducing new macros in 3.8, and more importantly made incompatible with pre-3.8 usage.
+1. The deprecation should follow PEP 387, which means emitting a warning. I think a compiler warning is appropriate; that could be done by having the macro call a deprecated function. Depending on how broken the macros are, a compiler warning could be backported to older versions.
If the macro is deprecated, please provide an detailed explanation how to port existing C code to the new C API, in What's New In Python 3.X (version where the macro is deprecated, Python 3.11 if I understood correctly). Also, is there a way to write a single code base working on Python 3.6-Python 3.11? It seems like mypy uses macros to support Python < 3.8 and Python >= 3.8: #if PY_MAJOR_VERSION >= 3 && PY_MINOR_VERSION >= 8 #define CPy_TRASHCAN_BEGIN(op, dealloc) Py_TRASHCAN_BEGIN(op, dealloc) #define CPy_TRASHCAN_END(op) Py_TRASHCAN_END #else #define CPy_TRASHCAN_BEGIN(op, dealloc) Py_TRASHCAN_SAFE_BEGIN(op) #define CPy_TRASHCAN_END(op) Py_TRASHCAN_SAFE_END(op) A search in PyPI top 1000 project gives me 8 projects (I was too lazy to check the full top 5000). It would be great to help these projects (propose pull requests) to migrate to the new C API. (*) pyrsistent-0.18.0.tar.gz
./pvectorcmodule.c: Py_TRASHCAN_SAFE_BEGIN(self); ./pvectorcmodule.c: Py_TRASHCAN_SAFE_BEGIN(self);
(*) PyGObject-3.40.1.tar.gz
./gi/pygi-resulttuple.c: Py_TRASHCAN_SAFE_BEGIN (self)
(*) pycurl-7.44.1.tar.gz
./src/easy.c: Py_TRASHCAN_SAFE_BEGIN(self); ./src/multi.c: Py_TRASHCAN_SAFE_BEGIN(self); ./src/share.c: Py_TRASHCAN_SAFE_BEGIN(self);
(*) mypy-0.910.tar.gz
./mypyc/lib-rt/CPy.h:
#if PY_MAJOR_VERSION >= 3 && PY_MINOR_VERSION >= 8 #define CPy_TRASHCAN_BEGIN(op, dealloc) Py_TRASHCAN_BEGIN(op, dealloc) #define CPy_TRASHCAN_END(op) Py_TRASHCAN_END #else #define CPy_TRASHCAN_BEGIN(op, dealloc) Py_TRASHCAN_SAFE_BEGIN(op) #define CPy_TRASHCAN_END(op) Py_TRASHCAN_SAFE_END(op) #endif --- (*) pypi-top-1000_2021-08-17/multidict-5.1.0.tar.gz
./multidict/_multidict.c: Py_TRASHCAN_SAFE_BEGIN(self);
(*) pypi-top-1000_2021-08-17/immutables-0.16.tar.gz
./immutables/_map.c: Py_TRASHCAN_SAFE_BEGIN(self) ./immutables/_map.c: Py_TRASHCAN_SAFE_BEGIN(self) ./immutables/_map.c: Py_TRASHCAN_SAFE_BEGIN(self)
(*) guppy3-3.1.1.tar.gz
./src/sets/nodeset.c: Py_TRASHCAN_SAFE_BEGIN(v) ./src/sets/immnodeset.c: Py_TRASHCAN_SAFE_BEGIN(it) ./src/sets/immnodeset.c: Py_TRASHCAN_SAFE_BEGIN(v) ./src/heapy/hv.c: Py_TRASHCAN_SAFE_BEGIN(v) ./src/heapy/classifier.c: Py_TRASHCAN_SAFE_BEGIN(op) ./src/heapy/nodegraph.c: Py_TRASHCAN_SAFE_BEGIN(v) ./src/heapy/hv_cli_rel.c: Py_TRASHCAN_SAFE_BEGIN(op)
(*) frozendict-2.0.6.tar.gz
<many files using the macro>
Victor On Tue, Aug 17, 2021 at 12:02 PM Łukasz Langa <lukasz@langa.pl> wrote:
Hi everybody, I'd like to revive this thread as I feel like we have to do something here but some consensus is needed first.
To recap, the current state of things is as follows: - in March 2000 (d724b23420f) Christian Tismer contributed the "trashcan" patch that added Py_TRASHCAN_SAFE_BEGIN and Py_TRASHCAN_SAFE_END macros which allow destroying nested objects non-recursively. - in May 2019 (GH-11841 of BPO-35983) Antoine Pitrou merged a change by Jeroen Demeyer which made Py_TRASHCAN_SAFE_BEGIN/END (unintentionally?) backwards incompatible; this was released in Python 3.8.0. - by the way, GH-11841 introduced a new pair of macros (because they have different signatures) called simply Py_TRASHCAN_BEGIN and Py_TRASHCAN_END. - by that time there was already a follow-up PR open (GH-12607) to improve backwards compatibility of the macros, as well as introduce tests for them; this was never merged. - in Feb 2020 (0fa4f43db08) Victor Stinner removed the trashcan mechanism from the limited C API (note: not ABI, those are macros) since it accesses fields of structs not exposed in the limited C API; this was released in Python 3.9.0. - in May 2020 Irit noticed that the backwards incompatibility (BPO-40608) causes segfaults for C API code that worked fine with Python 3.7. Using the new macros requires code changes but doesn't crash.
Now, there are a couple of things we can do here: Option 1: Finish GH-12607 to fix the old macros, keeping in mind this will restore compatibility lost with Python 3.8 - 3.10 only for users of 3.11+ Option 2: Review and merge GH-20104 that reverts the macro changes that make old client code segfault -- unclear what else this needs and again, that would only fix it for users of 3.11+ Option 3: Abandon GH-12607 and GH-20104, instead declaring the old macros deprecated for 3.11 and remove them in 3.13
I personally agree with Irit, voting +1 for Option 3 since the old macros were soft-deprecated already by introducing new macros in 3.8, and more importantly made incompatible with pre-3.8 usage.
Let's talk on how to proceed.
- Ł
On 26 Apr 2021, at 23:55, Irit Katriel via Python-Dev <python-dev@python.org> wrote:
Re https://bugs.python.org/issue40608.
I think it will be an act of kindness to deprecate Py_TRASHCAN_SAFE_BEGIN/END in 3.10 and tell people to use Py_TRASHCAN_BEGIN/END instead.
TL;DR: There was a change in 3.8 that introduced the latter while leaving the former for backwards compatibility, but also inadvertently breaking them. This is not an easy bug to deal with in the wild, we found it because we have a unit test in our codebase referencing https://bugs.python.org/issue16602. A deprecation note pointing to the new macros would have made it easier.
Is there any reason not to deprecate the old macros?
Irit _______________________________________________ Python-Dev mailing list -- python-dev@python.org To unsubscribe send an email to python-dev-leave@python.org https://mail.python.org/mailman3/lists/python-dev.python.org/ Message archived at https://mail.python.org/archives/list/python-dev@python.org/message/LWP6MOLP... Code of Conduct: http://python.org/psf/codeofconduct/
_______________________________________________ Python-Dev mailing list -- python-dev@python.org To unsubscribe send an email to python-dev-leave@python.org https://mail.python.org/mailman3/lists/python-dev.python.org/ Message archived at https://mail.python.org/archives/list/python-dev@python.org/message/TH4NILYO... Code of Conduct: http://python.org/psf/codeofconduct/
-- Night gathers, and now my watch begins. It shall not end until my death.
See also: * https://bugs.python.org/issue44881 "Consider integration of PyObject_GC_UnTrack() with the trashcan C API". * https://bugs.python.org/issue44897 "Integrate trashcan mechanism into _Py_Dealloc" The issue44897 would make all these macros useless ;-) Victor
Thanks, I've updated the PR. https://github.com/python/cpython/pull/27693 I added migration instructions to the news file (which Pablo can copy into what's new once it's committed). Also added a Py_DEPRECATED(3.11) typedef which is used in the macro. It remains the decide how to backport the compiler warning to older versions, and to which versions. On Tue, Aug 17, 2021 at 5:18 PM Victor Stinner <vstinner@python.org> wrote:
If the macro is deprecated, please provide an detailed explanation how to port existing C code to the new C API, in What's New In Python 3.X (version where the macro is deprecated, Python 3.11 if I understood correctly).
...
On 18 Aug 2021, at 11:47, Irit Katriel <iritkatriel@googlemail.com> wrote:
It remains the decide how to backport the compiler warning to older versions, and to which versions.
3.8 is when the macros broke. Since they cause segfaults, I'd say that's a pretty good reason to backport the warnings. While 3.8 in security-only fix mode, I think there's no issue with showing the "pending" deprecation in the to-be-released 3.8.12. And if that, then 3.9.7, and 3.10.0rc2 as well. Of course, for the latter we need Pablo's confirmation. The additional argument for it is that we backported GH-27683 (using new macros in frameobject.c) all the way back to 3.8. So building Python itself shouldn't start raising the deprecation warnings itself. But if it does, we'll just fix that as well. - Ł
On 26.04.21 23:55, Irit Katriel via Python-Dev wrote:
Re https://bugs.python.org/issue40608 <https://bugs.python.org/issue40608>.
I think it will be an act of kindness to deprecate Py_TRASHCAN_SAFE_BEGIN/END in 3.10 and tell people to use Py_TRASHCAN_BEGIN/END instead.
TL;DR: There was a change in 3.8 that introduced the latter while leaving the former for backwards compatibility, but also inadvertently breaking them. This is not an easy bug to deal with in the wild, we found it because we have a unit test in our codebase referencing https://bugs.python.org/issue16602 <http://bugs.python.org/issue16602>. A deprecation note pointing to the new macros would have made it easier.
Is there any reason not to deprecate the old macros?
Just a note (I'm happy that this old code is still in use :) ), can't we think of replacing it somehow by functions in the case of the Limited API? The API is so often used that it would make sense to _always_ don't crash deeply nested structures. Or do you think it makes no sense at all? Then let's turn it into a no-op. But the current mixed situation is not really pleasant. -- Christian Tismer-Sperling :^) tismer@stackless.com Software Consulting : http://www.stackless.com/ Strandstraße 37 : https://github.com/PySide 24217 Schönberg : GPG key -> 0xFB7BEE0E phone +49 173 24 18 776 fax +49 (30) 700143-0023
participants (7)
-
Antoine Pitrou -
Christian Tismer-Sperling -
Duncan Grisby -
Irit Katriel -
Petr Viktorin -
Victor Stinner -
Łukasz Langa