classification
Title: Change weird behavior of PyModule_AddObject()
Type: enhancement Stage: patch review
Components: Extension Modules, Interpreter Core Versions: Python 3.6
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: serhiy.storchaka, skrah
Priority: normal Keywords: patch

Created on 2016-04-27 17:52 by serhiy.storchaka, last changed 2016-04-29 23:07 by skrah.

Files
File name Uploaded Description Edit
pymodule_addobject.patch serhiy.storchaka, 2016-04-27 17:52 review
pymodule_addobject_2.patch serhiy.storchaka, 2016-04-28 15:19 review
Messages (8)
msg264384 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) 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 issue1782, 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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.
History
Date User Action Args
2017-04-16 16:44:28xiang.zhanglinkissue30081 superseder
2016-04-29 23:07:08skrahsetmessages: + msg264535
2016-04-28 15:19:45serhiy.storchakasetfiles: + pymodule_addobject_2.patch

messages: + msg264432
2016-04-28 08:04:55serhiy.storchakasetmessages: + msg264423
2016-04-27 22:48:25skrahsetnosy: + skrah
messages: + msg264404
2016-04-27 19:24:35skrahsetnosy: - skrah
2016-04-27 19:23:45skrahsetmessages: + msg264394
2016-04-27 19:07:31serhiy.storchakasetmessages: + msg264393
2016-04-27 18:09:03serhiy.storchakalinkissue26868 dependencies
2016-04-27 18:02:02skrahsetnosy: + skrah
messages: + msg264386
2016-04-27 17:52:51serhiy.storchakacreate