| msg264384 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) *  |
Date: 2016-04-27 17:52 |
| PyModule_AddObject() has weird and counterintuitive behavior. It steals a reference only on success. The caller is responsible to decref it on error. This behavior was not documented and inconsistent with the behavior of other functions stealing a reference (PyList_SetItem() and PyTuple_SetItem()). Seems most developers don't use this function correctly, since only in few places in the stdlib a reference is decrefed explicitly after PyModule_AddObject() failure. This weird behavior was first reported in , and changing it was proposed. Related bugs in PyModule_AddIntConstant() and PyModule_AddStringConstant() were fixed, but the behavior of PyModule_AddObject() was not changed and not documented. This issue is opened for gradual changing the behavior of PyModule_AddObject(). Proposed 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). This needs minimal changes to source code if PyModule_AddObject() was used incorrectly (i.e. as documented), and keep the code that explicitly decref a reference after PyModule_AddObject() working correctly. Use of PyModule_AddObject() without defining PY_MODULE_ADDOBJECT_CLEAN is declared deprecated (or we can defer this to 3.7). In the distant future (after dropping the support of 2.7) the old behavior will be dropped. See also a discussion on Python-Dev: http://comments.gmane.org/gmane.comp.python.devel/157545 . |
|
|
| msg264386 - (view) |
Author: Stefan Krah (skrah) * |
Date: 2016-04-27 18:02 |
| I think the current behavior is good for error handling by goto, which is common for module initialization. Please don't change this. |
|
|
| msg264393 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2016-04-27 19:07 |
| Idiomatic code is if (PyModule_AddObject(module, "name", create_new_object()) < 0) goto error; If you already have a reference and need to use it later: obj = create_new_object(); ... /* use obj */ Py_INCREF(); if (PyModule_AddObject(module, "name", create_new_object()) < 0) goto error; ... /* use obj */ Py_DECREF(obj); error: Py_XDECREF(obj); Many current code use above idioms, but it doesn't work as expected. It is almost impossible to write correct code with current behavior. And _decimal.c is not an exception, it has leaks. |
|
|
| msg264394 - (view) |
Author: Stefan Krah (skrah) * |
Date: 2016-04-27 19:23 |
| Your definition of correctness is very odd. The "leaks" you are talking about are single references in case of a module initialization failure, in which case you are likely to have much bigger problems. Please take _decimal out of this patch, it's a waste of time. |
|
|
| msg264404 - (view) |
Author: Stefan Krah (skrah) * |
Date: 2016-04-27 22:48 |
| It seems that the patch also introduces a segfault if PyLong_FromSsize_t() returns NULL. |
|
|
| msg264423 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2016-04-28 08:04 |
| > It seems that the patch also introduces a segfault if PyLong_FromSsize_t() returns NULL. Good catch Stefan! Py_XDECREF ahould be used instead of Py_DECREF in _PyModule_AddObject(). |
|
|
| msg264432 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2016-04-28 15:19 |
| Removed changes in _decimal.c and deprecation. Used Py_XDECREF in _PyModule_AddObject(). |
|
|
| msg264535 - (view) |
Author: Stefan Krah (skrah) * |
Date: 2016-04-29 23:07 |
| Serhiy, I'm sorry that I overreacted here. You're doing great work for Python -- we just happen to disagree on this one particular issue. I think there were some proponents on python-dev, perhaps they'll show up and discuss the details. |
|
|
| msg399752 - (view) |
Author: Petr Viktorin (petr.viktorin) * |
Date: 2021-08-17 14:14 |
| Since https://github.com/python/cpython/pull/23122, there is PyModule_AddObjectRef doesn't steal a reference. PyModule_AddObject is discouraged in the docs, but not formally deprecated. (As Stefan notes, the leaks are single references in case of a module initialization failure -- the cost of using the API incorrectly is very small; requiring everyone to change their code because it was possible to misuse it is overkill.) See https://docs.python.org/3.10/c-api/module.html#c.PyModule_AddObjectRef I think this is enough to close the issue. |
|
|