New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change weird behavior of PyModule_AddObject() #71058
Comments
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 bpo-1782, 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 . |
I think the current behavior is good for error handling by goto, Please don't change this. |
Idiomatic code is
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. |
Your definition of correctness is very odd. The "leaks" you are talking Please take _decimal out of this patch, it's a waste of time. |
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(). |
Removed changes in _decimal.c and deprecation. Used Py_XDECREF in _PyModule_AddObject(). |
Serhiy, I'm sorry that I overreacted here. You're doing great work for I think there were some proponents on python-dev, perhaps they'll show |
Since #23122, there is PyModule_AddObjectRef doesn't steal a reference. See https://docs.python.org/3.10/c-api/module.html#c.PyModule_AddObjectRef I think this is enough to close the issue. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: