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

b-pass · 2025-05-23T21:04:22Z

Description

Likely fix for #5687.

The module objects created by MODULE_INIT macros are stored in static pybind11::object, which will try to DECREF them when the program exits, which is likely to crash because the interpreter has probably shut down by then.

The solution is to just store the pointer, so that there is nothing to destruct during atexit.

📚 Documentation preview 📚: https://pybind11--5688.org.readthedocs.build/

include/pybind11/detail/common.h

Can't properly be made immortal

… for every interpreter.

include/pybind11/pybind11.h

…his be removed` The TODO was introduced with: pybind@7800e7f

rwgk · 2025-05-24T19:28:07Z

@b-pass I added commit 8736e1a

I got started with that because I wanted to replace the auto here, to improve readability:

-        static auto def = []() {                                                                  \
+        static PyModuleDef *def = []() {                                                          \

I'm still reviewing.

rwgk · 2025-05-24T19:55:28Z

I pushed another cleanup commit. I overlooked that part before.

The CI to watch is: https://github.com/pybind/pybind11/actions/runs/15230269745

EDIT: The CI run for commit 8736e1a succeeded (after one round of deflaking).

include/pybind11/detail/common.h

include/pybind11/pybind11.h

rwgk · 2025-05-24T20:11:56Z

@b-pass, see my last three question. Could you please help me getting my head around: What exactly happens in that code when there are sub-interpreters? What variables should be static, and what variables cannot be static?

rwgk · 2025-05-24T20:17:05Z

Unrelated to this PR: @henryiii there was this flake: https://github.com/pybind/pybind11/actions/runs/15230269745/job/42837274697

Not sure if you want to keep track?

(I just triggered a rerun.) EDIT: The rerun succeeded: https://github.com/pybind/pybind11/actions/runs/15230269745/job/42837982511

b-pass · 2025-05-24T20:24:16Z

@b-pass, see my last three question. Could you please help me getting my head around: What exactly happens in that code when there are sub-interpreters? What variables should be static, and what variables cannot be static?

Sub-interpreters rerun PyInit_ the first time the module is imported in the sub-interpreter. That could (and we have a test that does) happen in multiple threads at the same time. Everything here should be static. The only non-static step is running PyModuleDef_Init which converts the def into a PyObject.

The resulting PyObject should, IMO, be treated like any other Pyobject which means it should not be used across interpreters. However, it would be safe to do so based on the current implementation of PyModuleDef_Init. IMO it's defensive against future CPython changes to just treat it like any PyObject and so just call init every time and return it.

rwgk · 2025-05-24T20:27:18Z

@b-pass, see my last three question. Could you please help me getting my head around: What exactly happens in that code when there are sub-interpreters? What variables should be static, and what variables cannot be static?

Sub-interpreters rerun PyInit_ the first time the module is imported in the sub-interpreter. That could (and we have a test that does) happen in multiple threads at the same time. Everything here should be static. The only non-static step is running PyModuleDef_Init which converts the def into a PyObject.

The resulting PyObject should, IMO, be treated like any other Pyobject which means it should not be used across interpreters. However, it would be safe to do so based on the current implementation of PyModuleDef_Init. IMO it's defensive against future CPython changes to just treat it like any PyObject and so just call init every time and return it.

Do you think it would make sense to distill this into a comment? (I'm not sure what's the best place to put it.)

- Get rid of initialize_multiphase_module_def and just initialize it here in the function. - Make a simple slots init - Mkae these function statics instead of globals - Get rid of the lambda and just use the PyModuleDef member initializer.

henryiii · 2025-05-25T05:04:42Z

I'm tracking flakes in #5674. Those tests were intended to test the GIL, have so far resisted temptation to just skip them in free-threading. They need a lock somewhere. If someone has an idea, let me know. :)

I haven't been able to recreate these errors locally; I've tried the pytest free threading plugin, increasing the number of threads, and changing timeouts.

rwgk

Very nice, thanks a lot for the cleanup! Looks good to me, just one small oversight, and a question.

include/pybind11/pybind11.h

henryiii

Sorry, this didn't send earlier.

henryiii · 2025-05-25T05:14:12Z

include/pybind11/pybind11.h

@@ -1438,19 +1477,16 @@ 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)?


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.

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

Done: commit 93d093c

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

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

rwgk

Looks Great To Me. Thanks @b-pass and @henryiii!

(I'll be outside for the rest of the day. Please feel free to merge if all tests pass.)

b-pass added 4 commits May 23, 2025 16:36

These objects outlive the interpreter, they must be immortal.

3906de6

Add some alternatives for immortalize on older versions

ac8bc82

Format

476fd4b

Parenthesize macro parameter to make clang-tidy happy

441db31

henryiii previously approved these changes May 24, 2025

View reviewed changes

henryiii changed the title ~~Immortalize modules on init~~ fix: immortalize modules on init May 24, 2025

XuehaiPan reviewed May 24, 2025

View reviewed changes

include/pybind11/detail/common.h Outdated Show resolved Hide resolved

Return the PyModuleDef after it is initialized, call PyModuleDef_Init…

c4346e3

… for every interpreter.

b-pass changed the title ~~fix: immortalize modules on init~~ fix: don't destruct module objects in atexit May 24, 2025

XuehaiPan reviewed May 24, 2025

View reviewed changes

include/pybind11/pybind11.h Show resolved Hide resolved

Resolve long-standing `using module_def = PyModuleDef; // TODO: Can t…

8736e1a

…his be removed` The TODO was introduced with: pybind@7800e7f

[skip ci] Clean up mentions of module_def in comments.

3cd05c6

rwgk reviewed May 24, 2025

View reviewed changes

include/pybind11/detail/common.h Outdated Show resolved Hide resolved

rwgk reviewed May 24, 2025

View reviewed changes

include/pybind11/detail/common.h Outdated Show resolved Hide resolved

rwgk reviewed May 24, 2025

View reviewed changes

include/pybind11/detail/common.h Outdated Show resolved Hide resolved

rwgk reviewed May 24, 2025

View reviewed changes

include/pybind11/pybind11.h Outdated Show resolved Hide resolved

b-pass added 2 commits May 24, 2025 17:48

Clean up PYBIND11_MODULE_PYINIT even more

e17afca

- Get rid of initialize_multiphase_module_def and just initialize it here in the function. - Make a simple slots init - Mkae these function statics instead of globals - Get rid of the lambda and just use the PyModuleDef member initializer.

Add a comment

dffb2d5

rwgk mentioned this pull request May 25, 2025

Occasional errors with free-threading #5674

Open

rwgk reviewed May 25, 2025

View reviewed changes

include/pybind11/pybind11.h Show resolved Hide resolved

include/pybind11/pybind11.h Outdated Show resolved Hide resolved

b-pass added 2 commits May 25, 2025 08:24

Clean up error handling in init_slots, add a comment

131d7d5

[skip ci] spelling

69a6498

henryiii reviewed May 25, 2025

View reviewed changes

rwgk added 3 commits May 25, 2025 10:20

Merge branch 'master' into immortalize-modules-on-init

753bc5e

Add back using module_def = PyModuleDef;

93d093c

Replace typedef with type alias in comment.

580480f

rwgk approved these changes May 25, 2025

View reviewed changes

henryiii merged commit 03b4a9e into pybind:master May 25, 2025
74 checks passed

github-actions bot added the needs changelog Possibly needs a changelog entry label May 25, 2025

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

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

Conversation

b-pass commented May 23, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

Uh oh!

Uh oh!

rwgk commented May 24, 2025

Uh oh!

rwgk commented May 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rwgk commented May 24, 2025

Uh oh!

rwgk commented May 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

b-pass commented May 24, 2025

Uh oh!

rwgk commented May 24, 2025

Uh oh!

henryiii commented May 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rwgk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

henryiii left a comment

Choose a reason for hiding this comment

Uh oh!

henryiii May 25, 2025

Choose a reason for hiding this comment

Uh oh!

rwgk May 25, 2025

Choose a reason for hiding this comment

Uh oh!

rwgk May 25, 2025

Choose a reason for hiding this comment

Uh oh!

rwgk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

b-pass commented May 23, 2025 •

edited by github-actions bot

Loading

rwgk commented May 24, 2025 •

edited

Loading

rwgk commented May 24, 2025 •

edited

Loading

henryiii commented May 25, 2025 •

edited

Loading