Skip to content

fix: don't destruct module objects in atexit #5688

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.

Sign up for GitHub

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

Merged
merged 14 commits into from
May 25, 2025
Merged

fix: don't destruct module objects in atexit #5688

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
3906de6
These objects outlive the interpreter, they must be immortal.
b-pass May 23, 2025
ac8bc82
Add some alternatives for immortalize on older versions
b-pass May 23, 2025
476fd4b
Format
b-pass May 23, 2025
441db31
Parenthesize macro parameter to make clang-tidy happy
b-pass May 23, 2025
c4346e3
Return the PyModuleDef after it is initialized, call PyModuleDef_Init…
b-pass May 24, 2025
8736e1a
Resolve long-standing `using module_def = PyModuleDef; // TODO: Can t…
rwgk May 24, 2025
3cd05c6
[skip ci] Clean up mentions of `module_def` in comments.
rwgk May 24, 2025
e17afca
Clean up PYBIND11_MODULE_PYINIT even more
b-pass May 24, 2025
dffb2d5
Add a comment
b-pass May 24, 2025
131d7d5
Clean up error handling in init_slots, add a comment
b-pass May 25, 2025
69a6498
[skip ci] spelling
b-pass May 25, 2025
753bc5e
Merge branch 'master' into immortalize-modules-on-init
rwgk May 25, 2025
93d093c
Add back `using module_def = PyModuleDef;`
rwgk May 25, 2025
580480f
Replace `typedef` with `type alias` in comment.
rwgk May 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 24 additions & 20 deletions include/pybind11/detail/common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,35 +385,39 @@
} \
PyObject *pybind11_init()

// this push is for the next several macros
PYBIND11_WARNING_PUSH
PYBIND11_WARNING_DISABLE_CLANG("-Wgnu-zero-variadic-macro-arguments")

/**
Create a PyInit_ function for this module.

Note that this is run once for each (sub-)interpreter the module is imported into, including
possibly concurrently. The PyModuleDef is allowed to be static, but the PyObject* resulting from
PyModuleDef_Init should be treated like any other PyObject (so not shared across interpreters).
*/
#define PYBIND11_MODULE_PYINIT(name, pre_init, ...) \
static ::pybind11::module_::module_def PYBIND11_CONCAT(pybind11_module_def_, name); \
static ::pybind11::module_::slots_array PYBIND11_CONCAT(pybind11_module_slots_, name); \
static int PYBIND11_CONCAT(pybind11_exec_, name)(PyObject *); \
static void PYBIND11_CONCAT(pybind11_init_, name)(::pybind11::module_ &); \
PYBIND11_PLUGIN_IMPL(name) { \
PYBIND11_CHECK_PYTHON_VERSION \
pre_init; \
PYBIND11_ENSURE_INTERNALS_READY \
static auto result = []() { \
auto &slots = PYBIND11_CONCAT(pybind11_module_slots_, name); \
slots[0] = {Py_mod_exec, \
reinterpret_cast<void *>(&PYBIND11_CONCAT(pybind11_exec_, name))}; \
slots[1] = {0, nullptr}; \
return ::pybind11::module_::initialize_multiphase_module_def( \
PYBIND11_TOSTRING(name), \
nullptr, \
&PYBIND11_CONCAT(pybind11_module_def_, name), \
slots, \
##__VA_ARGS__); \
}(); \
return result.ptr(); \
static ::pybind11::detail::slots_array slots = ::pybind11::detail::init_slots( \
&PYBIND11_CONCAT(pybind11_exec_, name), ##__VA_ARGS__); \
static PyModuleDef def{/* m_base */ PyModuleDef_HEAD_INIT, \
/* m_name */ PYBIND11_TOSTRING(name), \
/* m_doc */ nullptr, \
/* m_size */ 0, \
/* m_methods */ nullptr, \
/* m_slots */ slots.data(), \
/* m_traverse */ nullptr, \
/* m_clear */ nullptr, \
/* m_free */ nullptr}; \
return PyModuleDef_Init(&def); \
}

PYBIND11_WARNING_POP

#define PYBIND11_MODULE_EXEC(name, variable) \
static void PYBIND11_CONCAT(pybind11_init_, name)(::pybind11::module_ &); \
int PYBIND11_CONCAT(pybind11_exec_, name)(PyObject * pm) { \
try { \
auto m = pybind11::reinterpret_borrow<::pybind11::module_>(pm); \
Expand Down Expand Up @@ -464,12 +468,12 @@ PYBIND11_WARNING_POP
}

\endrst */
PYBIND11_WARNING_PUSH
PYBIND11_WARNING_DISABLE_CLANG("-Wgnu-zero-variadic-macro-arguments")
#define PYBIND11_MODULE(name, variable, ...) \
PYBIND11_MODULE_PYINIT( \
name, (pybind11::detail::get_num_interpreters_seen() += 1), ##__VA_ARGS__) \
PYBIND11_MODULE_EXEC(name, variable)

// pop gnu-zero-variadic-macro-arguments
PYBIND11_WARNING_POP

PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)
Expand Down
108 changes: 36 additions & 72 deletions include/pybind11/pybind11.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1325,6 +1325,38 @@ inline void *multi_interp_slot(F &&, O &&...o) {
}
#endif

/// Must be a POD type, and must hold enough entries for all of the possible slots PLUS ONE for
/// the sentinel (0) end slot.
using slots_array = std::array<PyModuleDef_Slot, 4>;

/// Initialize an array of slots based on the supplied exec slot and options.
template <typename... Options>
static slots_array init_slots(int (*exec_fn)(PyObject *), Options &&...options) noexcept {
/* NOTE: slots_array MUST be large enough to hold all possible options. If you add an option
here, you MUST also increase the size of slots_array in the type alias above! */
slots_array slots;
size_t next_slot = 0;

if (exec_fn != nullptr) {
slots[next_slot++] = {Py_mod_exec, reinterpret_cast<void *>(exec_fn)};
}

#ifdef Py_mod_multiple_interpreters
slots[next_slot++] = {Py_mod_multiple_interpreters, multi_interp_slot(options...)};
#endif

if (gil_not_used_option(options...)) {
#if defined(Py_mod_gil) && defined(Py_GIL_DISABLED)
slots[next_slot++] = {Py_mod_gil, Py_MOD_GIL_NOT_USED};
#endif
}

// slots must have a zero end sentinel
slots[next_slot++] = {0, nullptr};

return slots;
}

PYBIND11_NAMESPACE_END(detail)

/// Wrapper for Python extension modules
Expand Down Expand Up @@ -1438,19 +1470,19 @@ class module_ : public object {
PyModule_AddObject(ptr(), name, obj.inc_ref().ptr() /* steals a reference */);
}

using module_def = PyModuleDef; // TODO: Can this be removed (it was needed only for Python 2)?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this isn't heavily used, it is used, so maybe we should keep it around just a little longer? Especially since we are in the RC phase?

Longer term, we have a collection of these defines used for Py2 compatibility, and it would be really nice to list them as deprecated and ideally make using them produce a deprecation warning.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this! I'll put it back, with a deprecation comment.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: commit 93d093c

I tried using PYBIND11_DEPRECATED but unfortunately that doesn't work:

https://chatgpt.com/share/683351d0-14a8-8008-9d31-0fd287dfc198

// DEPRECATED (since PR #5688): Use PyModuleDef directly instead.
using module_def = PyModuleDef;

/** \rst
Create a new top-level module that can be used as the main module of a C extension.

``def`` should point to a statically allocated module_def.
``def`` should point to a statically allocated PyModuleDef.
\endrst */
static module_ create_extension_module(const char *name,
const char *doc,
module_def *def,
PyModuleDef *def,
mod_gil_not_used gil_not_used
= mod_gil_not_used(false)) {
// module_def is PyModuleDef
// Placement new (not an allocation).
new (def) PyModuleDef{/* m_base */ PyModuleDef_HEAD_INIT,
/* m_name */ name,
Expand Down Expand Up @@ -1478,74 +1510,6 @@ class module_ : public object {
// For Python 2, reinterpret_borrow was correct.
return reinterpret_borrow<module_>(m);
}

/// Must be a POD type, and must hold enough entries for all of the possible slots PLUS ONE for
/// the sentinel (0) end slot.
using slots_array = std::array<PyModuleDef_Slot, 4>;

/** \rst
Initialize a module def for use with multi-phase module initialization.

``def`` should point to a statically allocated module_def.
``slots`` must already contain a Py_mod_exec or Py_mod_create slot and will be filled with
additional slots from the supplied options (and the empty sentinel slot).
\endrst */
template <typename... Options>
static object initialize_multiphase_module_def(const char *name,
const char *doc,
module_def *def,
slots_array &slots,
Options &&...options) {
size_t next_slot = 0;
size_t term_slot = slots.size() - 1;

// find the end of the supplied slots
while (next_slot < term_slot && slots[next_slot].slot != 0) {
++next_slot;
}

#ifdef Py_mod_multiple_interpreters
if (next_slot >= term_slot) {
pybind11_fail("initialize_multiphase_module_def: not enough space in slots");
}
slots[next_slot++] = {Py_mod_multiple_interpreters, detail::multi_interp_slot(options...)};
#endif

if (detail::gil_not_used_option(options...)) {
#if defined(Py_mod_gil) && defined(Py_GIL_DISABLED)
if (next_slot >= term_slot) {
pybind11_fail("initialize_multiphase_module_def: not enough space in slots");
}
slots[next_slot++] = {Py_mod_gil, Py_MOD_GIL_NOT_USED};
#endif
}

// slots must have a zero end sentinel
if (next_slot > term_slot) {
pybind11_fail("initialize_multiphase_module_def: not enough space in slots");
}
slots[next_slot++] = {0, nullptr};

// module_def is PyModuleDef
// Placement new (not an allocation).
new (def) PyModuleDef{/* m_base */ PyModuleDef_HEAD_INIT,
/* m_name */ name,
/* m_doc */ options::show_user_defined_docstrings() ? doc : nullptr,
/* m_size */ 0,
/* m_methods */ nullptr,
/* m_slots */ &slots[0],
/* m_traverse */ nullptr,
/* m_clear */ nullptr,
/* m_free */ nullptr};
auto *m = PyModuleDef_Init(def);
if (m == nullptr) {
if (PyErr_Occurred()) {
throw error_already_set();
}
pybind11_fail("Internal error in module_::initialize_multiphase_module_def()");
}
return reinterpret_borrow<object>(m);
}
};

PYBIND11_NAMESPACE_BEGIN(detail)
Expand Down
3 changes: 1 addition & 2 deletions tests/test_modules.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,7 @@ TEST_SUBMODULE(modules, m) {
class DupeException {};

// Go ahead and leak, until we have a non-leaking py::module_ constructor
auto dm
= py::module_::create_extension_module("dummy", nullptr, new py::module_::module_def);
auto dm = py::module_::create_extension_module("dummy", nullptr, new PyModuleDef);
auto failures = py::list();

py::class_<Dupe1>(dm, "Dupe1");
Expand Down
Loading