[Python-Dev] Inconsistency of PyModule_AddObject()
There are three functions (or at least three documented functions) in C API that "steals" references: PyList_SetItem(), PyTuple_SetItem() and PyModule_AddObject(). The first two "steals" references even on failure, and this is well known behaviour. But PyModule_AddObject() "steals" a reference only on success. There is nothing in the documentation that points on this. Most usages of PyModule_AddObject() in the stdlib don't decref the reference to the value on PyModule_AddObject() failure. The only exceptions are in _json, _io, and _tkinter modules. In many cases, including examples in the documentation, the successfulness of PyModule_AddObject() is not checked either, but this is different issue. We can just fix the documentation but adding a note that PyModule_AddObject() doesn't steal a reference on failure. And add explicit decrefs after PyModule_AddObject() in hundreds of places in the code. But I think it would be better to "fix" PyModule_AddObject() by making it decrefing a reference on failure as expected by most developers. But this is dangerous change, because if the author of third-party code read not only the documentation, but CPython code, and added explicit decref on PyModule_AddObject() failure, we will get a double decrefing. I think that we can resolve this issue by following steps: 1. Add a new function PyModule_AddObject2(), that steals a reference even on failure. 2. Introduce a special macro like PY_SSIZE_T_CLEAN (any suggestions about a name?). If it is defined, define PyModule_AddObject as PyModule_AddObject2. Define this macro before including Python.h in all CPython modules except _json, _io, and _tkinter. 3. Make old PyModule_AddObject to emit a warning about possible leak and a suggestion to define above macro. ___ Python-Dev mailing list [email protected] https://mail.python.org/mailman/listinfo/python-dev Unsubscribe: https://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
Re: [Python-Dev] Inconsistency of PyModule_AddObject()
On Wed, Apr 27, 2016 at 10:14 AM, Serhiy Storchaka wrote: > I think that we can resolve this issue by following steps: > > 1. Add a new function PyModule_AddObject2(), that steals a reference even on > failure. +1 It would be good to document PyModule_AddObject's current behavior in 3.5+ (already attached a patch). > 2. Introduce a special macro like PY_SSIZE_T_CLEAN (any suggestions about a > name?). If it is defined, define PyModule_AddObject as PyModule_AddObject2. > Define this macro before including Python.h in all CPython modules except > _json, _io, and _tkinter. +1 > 3. Make old PyModule_AddObject to emit a warning about possible leak and a > suggestion to define above macro. +0 ___ Python-Dev mailing list [email protected] https://mail.python.org/mailman/listinfo/python-dev Unsubscribe: https://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
Re: [Python-Dev] Inconsistency of PyModule_AddObject()
On 04/27/2016 09:14 AM, Serhiy Storchaka wrote: There are three functions (or at least three documented functions) in C API that "steals" references: PyList_SetItem(), PyTuple_SetItem() and PyModule_AddObject(). The first two "steals" references even on failure, and this is well known behaviour. But PyModule_AddObject() "steals" a reference only on success. There is nothing in the documentation that points on this. This inconsistency has caused bugs (or, more fairly, potential leaks) before, see http://bugs.python.org/issue1782 Unfortunately, the suggested Python 3 change to PyModule_AddObject was not accepted. 1. Add a new function PyModule_AddObject2(), that steals a reference even on failure. This sounds like a good idea, except the name could be prettier :), e.g. PyModule_InsertObject. PyModule_AddObject could be deprecated. Hrvoje ___ Python-Dev mailing list [email protected] https://mail.python.org/mailman/listinfo/python-dev Unsubscribe: https://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
Re: [Python-Dev] Inconsistency of PyModule_AddObject()
On 27 April 2016 at 17:14, Serhiy Storchaka wrote: > I think that we can resolve this issue by following steps: > > 1. Add a new function PyModule_AddObject2(), that steals a reference even on > failure. I'd suggest a variant on this that more closely matches the PyList_SetItem and PyTuple_SetItem cases: PyModule_SetAttrString The first two match the signature of PySequence_SetItem, but steal the reference instead of making a new one, and the same relationship would exist between PyObject_SetAttrString and the new PyModule_SetAttrString. Cheers, Nick. -- Nick Coghlan | [email protected] | Brisbane, Australia ___ Python-Dev mailing list [email protected] https://mail.python.org/mailman/listinfo/python-dev Unsubscribe: https://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
Re: [Python-Dev] Inconsistency of PyModule_AddObject()
On 27 April 2016 at 23:08, Nick Coghlan wrote: > On 27 April 2016 at 17:14, Serhiy Storchaka wrote: >> I think that we can resolve this issue by following steps: >> >> 1. Add a new function PyModule_AddObject2(), that steals a reference even on >> failure. > > I'd suggest a variant on this that more closely matches the > PyList_SetItem and PyTuple_SetItem cases: PyModule_SetAttrString And for the record: that suggestion was prompted by Hrvoje's email suggesting using a more descriptive name, I just went and looked up the name of the corresponding PyObject_* API. Cheers, Nick. -- Nick Coghlan | [email protected] | Brisbane, Australia ___ Python-Dev mailing list [email protected] https://mail.python.org/mailman/listinfo/python-dev Unsubscribe: https://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
Re: [Python-Dev] Inconsistency of PyModule_AddObject()
On 27.04.16 10:14, Serhiy Storchaka wrote: There are three functions (or at least three documented functions) in C API that "steals" references: PyList_SetItem(), PyTuple_SetItem() and PyModule_AddObject(). The first two "steals" references even on failure, and this is well known behaviour. But PyModule_AddObject() "steals" a reference only on success. There is nothing in the documentation that points on this. Most usages of PyModule_AddObject() in the stdlib don't decref the reference to the value on PyModule_AddObject() failure. The only exceptions are in _json, _io, and _tkinter modules. In many cases, including examples in the documentation, the successfulness of PyModule_AddObject() is not checked either, but this is different issue. We can just fix the documentation but adding a note that PyModule_AddObject() doesn't steal a reference on failure. And add explicit decrefs after PyModule_AddObject() in hundreds of places in the code. But I think it would be better to "fix" PyModule_AddObject() by making it decrefing a reference on failure as expected by most developers. But this is dangerous change, because if the author of third-party code read not only the documentation, but CPython code, and added explicit decref on PyModule_AddObject() failure, we will get a double decrefing. I think that we can resolve this issue by following steps: 1. Add a new function PyModule_AddObject2(), that steals a reference even on failure. 2. Introduce a special macro like PY_SSIZE_T_CLEAN (any suggestions about a name?). If it is defined, define PyModule_AddObject as PyModule_AddObject2. Define this macro before including Python.h in all CPython modules except _json, _io, and _tkinter. 3. Make old PyModule_AddObject to emit a warning about possible leak and a suggestion to define above macro. Opened an issue: http://bugs.python.org/issue26871 . Provided patch introduces new macros PY_MODULE_ADDOBJECT_CLEAN that controls the behavior of PyModule_AddObject() as PY_SSIZE_T_CLEAN controls the behavior of PyArg_Parse* functions. If the macro is defined before including "Python.h", PyModule_AddObject() steals a reference unconditionally. Otherwise it steals a reference only on success, and the caller is responsible for decref'ing it on error (current behavior). ___ Python-Dev mailing list [email protected] https://mail.python.org/mailman/listinfo/python-dev Unsubscribe: https://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
Re: [Python-Dev] Inconsistency of PyModule_AddObject()
On 27.04.16 15:31, Hrvoje Niksic wrote: On 04/27/2016 09:14 AM, Serhiy Storchaka wrote: There are three functions (or at least three documented functions) in C API that "steals" references: PyList_SetItem(), PyTuple_SetItem() and PyModule_AddObject(). The first two "steals" references even on failure, and this is well known behaviour. But PyModule_AddObject() "steals" a reference only on success. There is nothing in the documentation that points on this. This inconsistency has caused bugs (or, more fairly, potential leaks) before, see http://bugs.python.org/issue1782 Glad to hear I'm not the first faced with this problem. Unfortunately, the suggested Python 3 change to PyModule_AddObject was not accepted. Bad. May be it happened because of the risk to break third-party working code. I propose a gradual path to change PyModule_AddObject. 1. Add a new function PyModule_AddObject2(), that steals a reference even on failure. This sounds like a good idea, except the name could be prettier :), e.g. PyModule_InsertObject. PyModule_AddObject could be deprecated. I have decided to not introduce new public function. But just control the behavior of old function with the macro. This needs minimal changes to user code. ___ Python-Dev mailing list [email protected] https://mail.python.org/mailman/listinfo/python-dev Unsubscribe: https://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
Re: [Python-Dev] Inconsistency of PyModule_AddObject()
On 27.04.16 16:08, Nick Coghlan wrote: On 27 April 2016 at 17:14, Serhiy Storchaka wrote: I think that we can resolve this issue by following steps: 1. Add a new function PyModule_AddObject2(), that steals a reference even on failure. I'd suggest a variant on this that more closely matches the PyList_SetItem and PyTuple_SetItem cases: PyModule_SetAttrString The first two match the signature of PySequence_SetItem, but steal the reference instead of making a new one, and the same relationship would exist between PyObject_SetAttrString and the new PyModule_SetAttrString. I think it is better to have relation with PyModule_AddIntConstant() etc than with PyObject_SetAttrString. My patch doesn't introduce new public function, but changes the behavior of the old function. This needs minimal changes to user code that mostly use PyModule_AddObject() incorrectly (not blaming authors). ___ Python-Dev mailing list [email protected] https://mail.python.org/mailman/listinfo/python-dev Unsubscribe: https://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
Re: [Python-Dev] Inconsistency of PyModule_AddObject()
Hrvoje Niksic avl.com> writes: > This inconsistency has caused bugs (or, more fairly, potential leaks) > before, see http://bugs.python.org/issue1782 > > Unfortunately, the suggested Python 3 change to PyModule_AddObject was > not accepted. First, these "leaks" only potentially show up when you already have much bigger problems (i.e. on Linux the machine would already freeze due to overallocation). Second, these "leaks" don't even show up as "definitely lost" in Valgrind (yes, I checked). On the bright side, Python must be in a very healthy state if we can afford to spend time on issues such as this one. Stefan Krah ___ Python-Dev mailing list [email protected] https://mail.python.org/mailman/listinfo/python-dev Unsubscribe: https://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
Re: [Python-Dev] Inconsistency of PyModule_AddObject()
On Wed, Apr 27, 2016 at 11:06 AM, Serhiy Storchaka wrote: > I think it is better to have relation with PyModule_AddIntConstant() etc > than with PyObject_SetAttrString. > > My patch doesn't introduce new public function, but changes the behavior of > the old function. This needs minimal changes to user code that mostly use > PyModule_AddObject() incorrectly (not blaming authors). How will this impact code that uses PyModule_AddObject() correctly? ___ Python-Dev mailing list [email protected] https://mail.python.org/mailman/listinfo/python-dev Unsubscribe: https://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
