GeezerVison

        PyObject *ref = PyWeakref_NewRef(mod, NULL);
        Py_DECREF(mod);
        if (ref == NULL) {
            return NULL;
        }
        mod = PyWeakref_GetObject(ref);
        Py_DECREF(ref);

Borrowed references are bad:

"Functions must not return borrowed references" says https://devguide.python.org/developer-workflow/c-api/index.html
Unclear lifetimes of borrowed references capi-workgroup/problems#5

I proposed to:

Add a new PyImport_AddModuleRef(const char *name) function, similar to PyImport_AddModule() but return a strong reference
Deprecate PyImport_AddModule() and PyImport_AddModuleObject()

Linked PRs

Reacted by sunmy2019

Activity

vstinner
added
type-bugAn unexpected behavior, bug, or errorAn unexpected behavior, bug, or error

topic-C-API
on Jun 19, 2023
vstinner
added 12 commits that reference this issue on Jun 19, 2023
pythongh-105922: Add PyImport_AddModuleRef() function
546ace8
pythongh-105922: Add PyImport_AddModuleRef() function
fa7d2d6
pythongh-105922: Add PyImport_AddModuleRef() function
631c02c
pythongh-105922: Refactor PyRun_InteractiveOneObjectEx()
7f2b8e2
pythongh-105922: Refactor PyRun_InteractiveOneObjectEx()
4ff1af3
pythongh-105922: Refactor PyRun_InteractiveOneObjectEx()
842036e
pythongh-105922: Refactor PyRun_InteractiveOneObjectEx()
f65aeaa
pythongh-105922: Refactor PyRun_InteractiveOneObjectEx()
66fb11f
gh-105922: Refactor PyRun_InteractiveOneObjectEx() (#105925)
a5c2ad0
pythongh-105922: Add PyImport_AddModuleRef() function
bf2a756
pythongh-105922: Add PyImport_AddModuleRef() function
f3a223d
gh-105922: Add PyImport_AddModuleRef() function (#105923)
03f1a13
erlend-aasland commented on Jun 20, 2023
erlend-aasland
on Jun 20, 2023
Contributor
More actions
The PRs are merged; looks like this can be closed.
vstinner commented on Jun 21, 2023
vstinner
on Jun 21, 2023
MemberAuthor
More actions

The PRs are merged; looks like this can be closed.

Well, this part is not done yet:

Deprecate PyImport_AddModule() and PyImport_AddModuleObject()

Reacted by Erlend E. Aasland
vstinner commented on Jun 21, 2023
vstinner
on Jun 21, 2023
MemberAuthor
More actions
I added PyImport_AddModuleRef() to pythoncapi-compat: commit python/pythoncapi-compat@8ba8db3
Reacted by Erlend E. Aasland
encukou commented on Jun 21, 2023
encukou
on Jun 21, 2023
Member
More actions
Is this function actually useful for anything except the __main__ module?
It returns an already loaded module, or a new empty module, but doesn't give any indication of which one of those it did. IMO, that means you can't really use it for anything except a module that's supposed to stay empty.

Would it be better to deprecate the old functions without direct replacement, and add PyImport_AddModuleRef as a recipe in the docs that users could adapt to their needs?

Or, should we change the function to indicate whether the returned module is new or not?

vstinner commented on Jun 22, 2023

vstinner

on Jun 22, 2023

MemberAuthor

More actions

@encukou:

Is this function actually useful for anything except the main module?

I'm not sure. Here is a code search on PyPI top 5,000 projcts: 31 projects use it.

cffi-1.15.1.tar.gz: cffi-1.15.1/cffi/_cffi_errors.h: m = PyImport_AddModule("_cffi_error_capture");
matplotlib-3.7.1.tar.gz: matplotlib-3.7.1/src/_macosx.m: if (!(builtins = PyImport_AddModule("builtins"))  // borrowed.
Cython-0.29.34.tar.gz: Cython-0.29.34/Cython/Compiler/ModuleNode.py: '%s = PyImport_AddModule(__Pyx_BUILTIN_MODULE_NAME); %s' % (
Cython-0.29.34.tar.gz: Cython-0.29.34/Cython/Compiler/ModuleNode.py: '%s = PyImport_AddModule((char *) "cython_runtime"); %s' % (
Cython-0.29.34.tar.gz: Cython-0.29.34/Cython/Compiler/ModuleNode.py: '%s = PyImport_AddModule("%s"); %s' % (
Cython-0.29.34.tar.gz: Cython-0.29.34/Cython/Includes/cpython/module.pxd: PyObject* PyImport_AddModule(const char *name) except NULL
Cython-0.29.34.tar.gz: Cython-0.29.34/Cython/Utility/CommonStructures.c: fake_module = PyImport_AddModule((char*) "_cython_" CYTHON_ABI);
Cython-0.29.34.tar.gz: Cython-0.29.34/Cython/Utility/CommonStructures.c: fake_module = PyImport_AddModule((char*) "_cython_" CYTHON_ABI);
Cython-0.29.34.tar.gz: Cython-0.29.34/Cython/Utility/ModuleSetupCode.c: abi_module = PyImport_AddModule(__Pyx_FastGIL_ABI_module);
cloudpickle-2.2.1.tar.gz: cloudpickle-2.2.1/tests/cloudpickle_testpkg/_cloudpickle_testpkg/mod.py: # it will use the Python/C API ``PyImport_AddModule(submodule_qualified_name)``
cloudpickle-2.2.1.tar.gz: cloudpickle-2.2.1/tests/cloudpickle_testpkg/build/lib/_cloudpickle_testpkg/mod.py: # it will use the Python/C API ``PyImport_AddModule(submodule_qualified_name)``
orjson-3.8.10.tar.gz: orjson-3.8.10/local_dependencies/pyo3-ffi/src/import.rs: pub fn PyImport_AddModuleObject(name: *mut PyObject) -> *mut PyObject;
orjson-3.8.10.tar.gz: orjson-3.8.10/local_dependencies/pyo3-ffi/src/import.rs: #[cfg_attr(PyPy, link_name = "PyPyImport_AddModule")]
orjson-3.8.10.tar.gz: orjson-3.8.10/local_dependencies/pyo3-ffi/src/import.rs: pub fn PyImport_AddModule(name: *const c_char) -> *mut PyObject;
orjson-3.8.10.tar.gz: orjson-3.8.10/include/pyo3/pyo3-ffi/src/import.rs: pub fn PyImport_AddModuleObject(name: *mut PyObject) -> *mut PyObject;
orjson-3.8.10.tar.gz: orjson-3.8.10/include/pyo3/pyo3-ffi/src/import.rs: #[cfg_attr(PyPy, link_name = "PyPyImport_AddModule")]
orjson-3.8.10.tar.gz: orjson-3.8.10/include/pyo3/pyo3-ffi/src/import.rs: pub fn PyImport_AddModule(name: *const c_char) -> *mut PyObject;
sentencepiece-0.1.98.tar.gz: sentencepiece-0.1.98/src/sentencepiece/sentencepiece_wrap.cxx: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
opencv-python-4.7.0.72.tar.gz: opencv-python-4.7.0.72/opencv/modules/python/src2/cv2.cpp: submodule = PyImport_AddModule(full_submodule_name.c_str());
opencv-python-4.7.0.72.tar.gz: opencv-python-4.7.0.72/opencv/modules/python/src2/cv2.cpp: /// Return `PyImport_AddModule` NULL with an exception set on failure.
JPype1-1.4.1.tar.gz: JPype1-1.4.1/native/common/include/jp_primitive_accessor.h: PyObject *typing = PyImport_AddModule("jpype.protocol");
JPype1-1.4.1.tar.gz: JPype1-1.4.1/native/common/jp_booleantype.cpp: PyObject *typing = PyImport_AddModule("jpype.protocol");
JPype1-1.4.1.tar.gz: JPype1-1.4.1/native/common/jp_booleantype.cpp: PyObject *typing = PyImport_AddModule("jpype.protocol");
JPype1-1.4.1.tar.gz: JPype1-1.4.1/native/common/jp_classhints.cpp: PyObject *typing = PyImport_AddModule("jpype.protocol");
JPype1-1.4.1.tar.gz: JPype1-1.4.1/native/common/jp_classhints.cpp: PyObject *typing = PyImport_AddModule("jpype.protocol");
JPype1-1.4.1.tar.gz: JPype1-1.4.1/native/common/jp_classhints.cpp: PyObject *typing = PyImport_AddModule("jpype.protocol");
JPype1-1.4.1.tar.gz: JPype1-1.4.1/native/common/jp_classhints.cpp: PyObject *typing = PyImport_AddModule("jpype.protocol");
JPype1-1.4.1.tar.gz: JPype1-1.4.1/native/common/jp_classhints.cpp: PyObject *typing = PyImport_AddModule("jpype.protocol");
JPype1-1.4.1.tar.gz: JPype1-1.4.1/native/common/jp_context.cpp: JPPyObject import = JPPyObject::use(PyImport_AddModule("importlib.util"));
JPype1-1.4.1.tar.gz: JPype1-1.4.1/native/common/jp_functional.cpp: PyObject *typing = PyImport_AddModule("jpype.protocol");
pybind11-2.10.4.tar.gz: pybind11-2.10.4/pybind11/include/pybind11/pybind11.h: handle submodule = PyImport_AddModule(full_name.c_str());
opencv-python-headless-4.7.0.72.tar.gz: opencv-python-headless-4.7.0.72/opencv/modules/python/src2/cv2.cpp: submodule = PyImport_AddModule(full_submodule_name.c_str());
opencv-python-headless-4.7.0.72.tar.gz: opencv-python-headless-4.7.0.72/opencv/modules/python/src2/cv2.cpp: /// Return `PyImport_AddModule` NULL with an exception set on failure.
onnx-1.13.1.tar.gz: onnx-1.13.1/third_party/pybind11/include/pybind11/pybind11.h: auto result = reinterpret_borrow<module_>(PyImport_AddModule(full_name.c_str()));
uwsgi-2.0.21.tar.gz: uwsgi-2.0.21/plugins/python/python_plugin.c: if (!PyImport_AddModule("dummy_threading"))
uwsgi-2.0.21.tar.gz: uwsgi-2.0.21/plugins/python/python_plugin.c: py_file_module = PyImport_AddModule(name);
uwsgi-2.0.21.tar.gz: uwsgi-2.0.21/plugins/python/python_plugin.c: new_uwsgi_module = PyImport_AddModule("uwsgi");
uwsgi-2.0.21.tar.gz: uwsgi-2.0.21/plugins/python/symimporter.c: PyObject *mod = PyImport_AddModule(fullname);
uwsgi-2.0.21.tar.gz: uwsgi-2.0.21/plugins/python/symimporter.c: PyObject *mod = PyImport_AddModule(fullname);
uwsgi-2.0.21.tar.gz: uwsgi-2.0.21/plugins/python/symimporter.c: PyObject *mod = PyImport_AddModule(fullname);
uwsgi-2.0.21.tar.gz: uwsgi-2.0.21/plugins/python/symimporter.c: PyObject *mod = PyImport_AddModule(fullname);
python-crfsuite-0.9.9.tar.gz: python-crfsuite-0.9.9/crfsuite/swig/python/export_wrap.cpp: PyObject *module = PyImport_AddModule((char*)"swig_runtime_data" SWIG_RUNTIME_VERSION);
pyinstaller-5.10.0.tar.gz: pyinstaller-5.10.0/bootloader/src/pyi_launch.c: __main__ = PI_PyImport_AddModule("__main__");
pyinstaller-5.10.0.tar.gz: pyinstaller-5.10.0/bootloader/src/pyi_python.c: DECLPROC(PyImport_AddModule);
pyinstaller-5.10.0.tar.gz: pyinstaller-5.10.0/bootloader/src/pyi_python.c: GETPROC(dll, PyImport_AddModule);
pyinstaller-5.10.0.tar.gz: pyinstaller-5.10.0/bootloader/src/pyi_python.h: EXTDECLPROC(PyObject *, PyImport_AddModule, (char *));
opencv-contrib-python-4.7.0.72.tar.gz: opencv-contrib-python-4.7.0.72/opencv/modules/python/src2/cv2.cpp: submodule = PyImport_AddModule(full_submodule_name.c_str());
opencv-contrib-python-4.7.0.72.tar.gz: opencv-contrib-python-4.7.0.72/opencv/modules/python/src2/cv2.cpp: /// Return `PyImport_AddModule` NULL with an exception set on failure.
PyMuPDF-1.21.1.tar.gz: PyMuPDF-1.21.1/fitz/helper-other.i: PyObject *this_module = PyImport_AddModule("fitz");  // get our module
cvxpy-1.3.1.tar.gz: cvxpy-1.3.1/cvxpy/cvxcore/python/cvxcore_wrap.cxx: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
mecab-python3-1.0.6.tar.gz: mecab-python3-1.0.6/src/MeCab/MeCab_wrap.cpp: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
M2Crypto-0.38.0.tar.gz: M2Crypto-0.38.0/src/SWIG/_m2crypto_wrap.c: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
pygraphviz-1.10.zip: pygraphviz-1.10/pygraphviz/graphviz_wrap.c: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
opencv-contrib-python-headless-4.7.0.72.tar.gz: opencv-contrib-python-headless-4.7.0.72/opencv/modules/python/src2/cv2.cpp: submodule = PyImport_AddModule(full_submodule_name.c_str());
opencv-contrib-python-headless-4.7.0.72.tar.gz: opencv-contrib-python-headless-4.7.0.72/opencv/modules/python/src2/cv2.cpp: /// Return `PyImport_AddModule` NULL with an exception set on failure.
GDAL-3.6.3.tar.gz: GDAL-3.6.3/extensions/gdal_array_wrap.cpp: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
GDAL-3.6.3.tar.gz: GDAL-3.6.3/extensions/gdal_wrap.cpp: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
GDAL-3.6.3.tar.gz: GDAL-3.6.3/extensions/gdalconst_wrap.c: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
GDAL-3.6.3.tar.gz: GDAL-3.6.3/extensions/gnm_wrap.cpp: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
GDAL-3.6.3.tar.gz: GDAL-3.6.3/extensions/ogr_wrap.cpp: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
GDAL-3.6.3.tar.gz: GDAL-3.6.3/extensions/osr_wrap.cpp: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
pythonnet-3.0.1.tar.gz: pythonnet-3.0.1/src/runtime/PythonEngine.cs: var module = Runtime.PyImport_AddModule(name);
pythonnet-3.0.1.tar.gz: pythonnet-3.0.1/src/runtime/Runtime.Delegates.cs: PyImport_AddModule = (delegate* unmanaged[Cdecl]<StrPtr, BorrowedReference>)GetFunctionByName(nameof(PyImport_AddModule), GetUnmanagedDll(_PythonDll));
pythonnet-3.0.1.tar.gz: pythonnet-3.0.1/src/runtime/Runtime.Delegates.cs: internal static delegate* unmanaged[Cdecl]<StrPtr, BorrowedReference> PyImport_AddModule { get; }
pythonnet-3.0.1.tar.gz: pythonnet-3.0.1/src/runtime/Runtime.cs: internal static BorrowedReference PyImport_AddModule(string name)
pythonnet-3.0.1.tar.gz: pythonnet-3.0.1/src/runtime/Runtime.cs: return Delegates.PyImport_AddModule(namePtr);
pyarmor-8.1.3.zip: pyarmor-8.1.3/src/helper/buildext.py: m = PyImport_AddModule("__main__");
dlib-19.24.1.tar.gz: dlib-19.24.1/dlib/external/pybind11/include/pybind11/pybind11.h: handle submodule = PyImport_AddModule(full_name.c_str());
osmium-3.6.0.tar.gz: osmium-3.6.0/contrib/pybind11/include/pybind11/pybind11.h: handle submodule = PyImport_AddModule(full_name.c_str());
osmium-3.6.0.tar.gz: osmium-3.6.0/contrib/pybind11/tests/test_modules.py: # Meant to trigger PyImport_AddModule() failure:
onnxoptimizer-0.3.10.tar.gz: onnxoptimizer-0.3.10/third_party/onnx/third_party/pybind11/include/pybind11/pybind11.h: auto result = reinterpret_borrow<module_>(PyImport_AddModule(full_name.c_str()));
casadi-3.6.0.tar.gz: casadi-3.6.0/swig/casadi.i: PyObject* module = PyImport_AddModule("casadi");
casadi-3.6.0.tar.gz: casadi-3.6.0/swig/python/target/source/casadiPYTHON_wrap.cxx: PyObject *module = PyImport_AddModule((char*)"swig_runtime_data" SWIG_RUNTIME_VERSION);
casadi-3.6.0.tar.gz: casadi-3.6.0/swig/python/target/source/casadiPYTHON_wrap.cxx: PyObject* module = PyImport_AddModule(SWIG_module_name);
casadi-3.6.0.tar.gz: casadi-3.6.0/swig/python/target/source/casadiPYTHON_wrap.cxx: PyObject* module = PyImport_AddModule("casadi");
casadi-3.6.0.tar.gz: casadi-3.6.0/swig/python/target3/source/casadiPYTHON_wrap.cxx: PyObject *module = PyImport_AddModule((char*)"swig_runtime_data" SWIG_RUNTIME_VERSION);
casadi-3.6.0.tar.gz: casadi-3.6.0/swig/python/target3/source/casadiPYTHON_wrap.cxx: PyObject* module = PyImport_AddModule(SWIG_module_name);
casadi-3.6.0.tar.gz: casadi-3.6.0/swig/python/target3/source/casadiPYTHON_wrap.cxx: PyObject* module = PyImport_AddModule("casadi");
onnxsim-0.4.19.tar.gz: onnxsim-0.4.19/third_party/onnx-optimizer/third_party/onnx/third_party/pybind11/include/pybind11/pybind11.h: auto result = reinterpret_borrow<module_>(PyImport_AddModule(full_name.c_str()));
onnxsim-0.4.19.tar.gz: onnxsim-0.4.19/third_party/pybind11/include/pybind11/pybind11.h: auto result = reinterpret_borrow<module_>(PyImport_AddModule(full_name.c_str()));
pygdal-3.5.1.11.tar.gz: pygdal-3.5.1.11/extensions/gdal_array_wrap.cpp: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
pygdal-3.5.1.11.tar.gz: pygdal-3.5.1.11/extensions/gdal_wrap.cpp: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
pygdal-3.5.1.11.tar.gz: pygdal-3.5.1.11/extensions/gdalconst_wrap.c: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
pygdal-3.5.1.11.tar.gz: pygdal-3.5.1.11/extensions/ogr_wrap.cpp: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
pygdal-3.5.1.11.tar.gz: pygdal-3.5.1.11/extensions/osr_wrap.cpp: PyObject *module = PyImport_AddModule("swig_runtime_data" SWIG_RUNTIME_VERSION);
mercurial-6.4.1.tar.gz: mercurial-6.4.1/contrib/fuzz/pyutil.cc: mainmod = PyImport_AddModule("__main__");

vstinner
added 4 commits that reference this issue on Jun 22, 2023
pythongh-105922: Use PyImport_AddModuleRef() function
57b2b7b
pythongh-105922: PyImport_AddModule() uses Py_DECREF()
283783c
pythongh-105922: PyImport_AddModule() uses Py_DECREF()
cef28f1
gh-105922: Use PyImport_AddModuleRef() function (#105999)
193a2b2
vstinner
changed the title ~~[-]C API: PyImport_AddModule() returns a borrowed reference[/-]~~ [+]C API: Add PyImport_AddModuleRef() function[/+] on Jun 23, 2023
vstinner commented on Jul 10, 2023
vstinner
on Jul 10, 2023
MemberAuthor
More actions
The initial issue, adding PyImport_AddModuleRef() function, was done, so I close the issue.

Deprecate PyImport_AddModule() and PyImport_AddModuleObject()

IMO PyWeakref_GetObject() API is unsafe since it returns a borrowed reference from a weak reference, so I chose to deprecate it as soon as I added PyWeakref_GetRef().

For PyImport_AddModuleObject(), it's less obvious since the module must be stored in sys.modules which holds a strong reference. Moreover, I checked and it looks impossible to use a type other than dict for sys.modules in practice, since PyImport_AddModuleObject() access directly to PyInterpreterState.imports.modules which cannot be overriden.

See PR #105998 and PR #106001 for technical details.

Moreover, the Steering Council is still discussing the topic: python/steering-council#201

For now, I prefer to not deprecate the old API. We can still deprecate it later once the situation will be more clear.
Reacted by Petr Viktorin
vstinner
closed this as completed on Jul 10, 2023
encukou commented on Jul 20, 2023
encukou
on Jul 20, 2023
Member
More actions
Let me bring the discussion from python/steering-council#201 (comment) here:

We will want to yet another function to fix that wart, and if no one gets to it in time, the *Ref iteration will be hard to get rid of.

Is there any user request for such feature? Can't you get the behavior you want with existing functions, like checking if the name is in sys.modules?

No, there is no such request. Neither is there a user request for a PyImport_AddModuleRef.

One minor change would be to provide the information if the module was imported or created (add an optional parameter for that). You can open an issue if you want that.

I opened an issue here: #106915
Reacted by Victor Stinner

Metadata

Assignees

No one assigned

Labels

topic-C-APItype-bugAn unexpected behavior, bug, or errorAn unexpected behavior, bug, or error

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

C API: Add PyImport_AddModuleRef() function #105922

Description

Linked PRs

Activity

erlend-aasland commented on Jun 20, 2023

vstinner commented on Jun 21, 2023

vstinner commented on Jun 21, 2023

encukou commented on Jun 21, 2023

vstinner commented on Jun 22, 2023

vstinner commented on Jul 10, 2023

encukou commented on Jul 20, 2023

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions