C API: Add PyModule_Add() function

[serhiy-storchaka]

BPO	42327
Nosy	@vstinner, @ambv, @serhiy-storchaka, @brandtbucher
PRs	bpo-42327: Add PyModule_Add(). #23240 bpo-42327: Add PyModule_Add() (smaller). #23443 [3.9] bpo-44050: Extension modules can share state when they don't support sub-interpreters. (GH-27794) #28741

^{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:

assignee = None
closed_at = None
created_at = <Date 2020-11-11.20:35:14.453>
labels = ['expert-C-API', 'type-feature', '3.10']
title = 'Add PyModule_Add()'
updated_at = <Date 2021-10-05.17:30:16.409>
user = 'https://github.com/serhiy-storchaka'

bugs.python.org fields:

activity = <Date 2021-10-05.17:30:16.409>
actor = 'lukasz.langa'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['C API']
creation = <Date 2020-11-11.20:35:14.453>
creator = 'serhiy.storchaka'
dependencies = []
files = []
hgrepos = []
issue_num = 42327
keywords = ['patch']
message_count = 9.0
messages = ['380794', '380795', '380818', '380819', '380820', '380821', '380829', '380833', '384812']
nosy_count = 4.0
nosy_names = ['vstinner', 'lukasz.langa', 'serhiy.storchaka', 'brandtbucher']
pr_nums = ['23240', '23443', '28741']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue42327'
versions = ['Python 3.10']

Linked PRs

• gh-86493: Fix possible leaks in modules initialization: _curses_panel, _decimal, posix, xxsubtype #106767
• gh-86493: Fix possible leaks in some modules initialization #106768
• [3.12] gh-86493: Fix possible leaks in modules initialization: _curses_panel, _decimal, posix, xxsubtype (GH-106767) #106849
• [3.11] gh-86493: Fix possible leaks in modules initialization: _curses_panel, _decimal, posix, xxsubtype (GH-106767) (GH-106849) #106851
• [3.12] gh-86493: Fix possible leaks in some modules initialization (GH-106768) #106855
• gh-86493: Modernize modules initialization code #106858
• gh-86493: Use PyModule_Add() instead of PyModule_AddObjectRef() #106860
• [3.11] gh-86493: Fix possible leaks in some modules initialization (GH-106768) (GH-106855) #106863

[serhiy-storchaka]

There is a design flaw in PyModule_AddObject(). It steals reference of its value only if the result is success. To avoid leaks it should be used in the following form:

 PyObject *tmp = <new reference>;
 if (PyModule_AddObject(name, name, tmp) < 0){
 Py_XDECREF(tmp);
 goto error;
 }

It is inconvenient and many code forgot to use a temporary variable and call Py_XDECREF().

It was not intention, but it is too late to change this behavior now, because some code calls Py_XDECREF() if PyModule_AddObject() fails. Fixing PyModule_AddObject() now will break hard such code.

There was an idea to make the change gradual, controlled by a special macro (see bpo-26871). But it still has significant risk.

I propose to add new function PyModule_Add() which always steals reference to its argument. It is more convenient and allows to get rid of temporary variable:

 if (PyModule_Add(name, name, <new reference>) < 0){
 goto error;
 }

I choose name PyModule_Add because it is short, and allow to write the call in one line with moderately long expression <new reference> (like PyFloat_FromDouble(...) or PyLong_FromUnsignedLong(...)).

[brandtbucher]

See also:

https://bugs.python.org/issue38823

#17298

[vstinner]

Oh, I just rejected PR 17298. Copy of my message:
---
I added PyModule_AddObjectRef() which uses strong references, rather than only stealing a reference on success.

I also enhanced the documentation to show concrete examples:
https://docs.python.org/dev/c-api/module.html#c.PyModule_AddObjectRef

I modified a few extension to use PyModule_AddObjectRef(). Sometimes, PyModule_AddObject() is more appropriate. Sometimes, PyModule_AddObjectRef() is more appropriate. Both functions are relevant, and I don't see a clear winner.

I agree than fixing existing code is painful, but I hope that new code using mostly PyModule_AddObjectRef() would be simpler to review. I'm not sure that it's simpler to write new code using PyModule_AddObjectRef(), since you might need more Py_DECREF() calls.

My intent is to have more "regular" code about reference counting. See also: https://bugs.python.org/issue42294

Since you wrote that this API is a band aid on a broken API, I consider that you are fine with rejecting it and move on to the new PyModule_AddObjectRef().

Anyway, thanks for you attempt to make the C API less broken :-)
---

I added PyModule_AddObjectRef() in bpo-163574.

[vstinner]

I'm using more and more often such macro:

#define MOD_ADD(name, expr) \
 do{\
 PyObject *obj = (expr); \
 if (obj == NULL){\
 return -1; \
 } \
 if (PyModule_AddObjectRef(mod, name, obj) < 0){\
 Py_DECREF(obj); \
 return -1; \
 } \
 Py_DECREF(obj); \
 } while (0)

Would PyModule_Add() replace such macro?

[vstinner]

If PyModule_Add() is added, I would suggest to rename PyModule_AddObjectRef() to PyModule_AddRef() for consistency.

IMHO PyModule_AddObjectRef() remains useful even if PyModule_Add() is added.

[vstinner]

In practice, PyModule_AddRef(mod, obj) behaves as PyModule_Add(mod, Py_NewRef(obj)) if I understood correctly.

[serhiy-storchaka]

PyModule_Add() allows to make such macro much simpler:

#define MOD_ADD(name, expr) \
 do{\
 if (PyModule_Add(mod, name, expr) < 0){\
 return -1; \
 } \
 } while (0)

PyModule_AddObjectRef() is just Py_XINCREF() followed by PyModule_Add(). But since most values added to the module are new references, Py_XINCREF() is usually not needed. The PyModule_Add* API is a convenient API. It is not necessary, you can use PyModule_GetDict() + PyDict_SetItemString(), but with this API it is easier. And PyModule_Add() is a correct PyModule_AddObject() (which was broken a long time ago and cannot be fixed now) and is more convenient than PyModule_AddObjectRef().

PyModule_AddIntConstant() and PyModule_AddStringConstant() can be easily expressed in terms of PyModule_Add():

 PyModule_Add(m, name, PyLong_FromLong(value))
 PyModule_Add(m, name, PyUnicode_FromString(value))

And it is easy to combine it with other functions: PyLong_FromLongLong(), PyLong_FromUnsignedLong(), PyLong_FromVoidPtr(), PyFloat_FromDouble(), PyCapsule_New(), PyType_FromSpec(), etc.

[vstinner]

PyModule_AddObjectRef() is just Py_XINCREF() followed by PyModule_Add(). But since most values added to the module are new references, Py_XINCREF() is usually not needed.

There is no general rule. I saw two main cases.

(A) Create an object only to add it into the module. PyModule_Add() and PyModule_AddObject() are good for that case.

Example in the array module:

 PyObject *typecodes = PyUnicode_DecodeASCII(buffer, p - buffer, NULL);
 if (PyModule_AddObject(m, "typecodes", typecodes) < 0){
 Py_XDECREF(typecodes);
 return -1;
 }

This code can be rewritten with PyModule_Add():

 PyObject *typecodes = PyUnicode_DecodeASCII(buffer, p - buffer, NULL);
 if (PyModule_Add(m, "typecodes", typecodes) < 0){
 return -1;
 }

Py_XDECREF(typecodes) is no longer needed using PyModule_Add().

(B) Add an existing object into the module, but the objet is already stored elsewhere. PyModule_AddObjectRef() is good for that case. It became common to have this case when an object is also stored in the module state.

Example in _ast:

 state->AST_type = PyType_FromSpec(&AST_type_spec);
 if (!state->AST_type){
 return 0;
 }
 (...)
 if (PyModule_AddObjectRef(m, "AST", state->AST_type) < 0){
 return -1;
 }

state->AST_type and module attribute both hold a strong reference to the type.

[vstinner]

Note for myself: I added PyModule_AddObjectRef() to https://github.com/pythoncapi/pythoncapi_compat If it's renamed, pythoncapi_compat must also be updated.