Skip to content
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

Jump to bottom

[BUG]: Python atexit broken with pybind11 version >= 2.9.2 #4459

Open
3 tasks done
chrreisinger opened this issue Jan 17, 2023 · 6 comments · Fixed by #4486 · May be fixed by #4505
Open
3 tasks done

[BUG]: Python atexit broken with pybind11 version >= 2.9.2 #4459

chrreisinger opened this issue Jan 17, 2023 · 6 comments · Fixed by #4486 · May be fixed by #4505
Labels
bug help wanted

Comments

@chrreisinger
Copy link

Required prerequisites

What version (or hash if on master) of pybind11 are you using?

2.10.2

Problem description

We want to upgrade from pybind11 release 2.9.0 to 2.10.2, but we encountered an issue in the Python atexit (https://docs.python.org/3/library/atexit.html) functions, that our custom types are not registered anymore. With delta debugging we found that this PR caused the issue: #3744

In my opinion the

    detail::get_local_internals().registered_types_cpp.clear();    
    detail::get_local_internals().registered_exception_translators.clear();

calls should be after the Py_Finalize function call.

When I change the orders of those function calls, the problem is gone.

Reproducible example code

No response

Is this a regression? Put the last known working version here if it is.

2.9.1
@chrreisinger chrreisinger added the triage New bug, unverified label Jan 17, 2023
@Skylion007
Copy link
Collaborator

FYI @rwgk

@rwgk
Copy link
Collaborator

rwgk commented Jan 19, 2023

@chrreisinger could you help by

  • creating a PR that moves the .clear() calls after the Py_Finalize() call,
  • and ideally adds a unit test for your situation (in tests/test_embed/test_interpreter.cpp)?

The change makes sense to me. My understanding is: the Py_Finalize() invalidates all PyObject* pointers in the local_internals, but looking at the struct local_internals code, it seems to me the .clear() calls don't ever use those pointers. There are a bunch of non-trivial destructors that will run, but none of them will call into the Python C API.

@chrreisinger
Copy link
Author

I will give it a try on the weekend.

@rwgk
Copy link
Collaborator

rwgk commented Jan 19, 2023

I will give it a try on the weekend.

Awesome.

If you create a PR with just the lines moved, that'll be a great start. It will be good to confirm that valgrind does not find any issues. We can work on the test together from there.

@Skylion007
Copy link
Collaborator

@chrreisinger Any updates on this?

Skylion007 added a commit to Skylion007/pybind11 that referenced this issue Feb 2, 2023
@Skylion007
Keep registered types until after Py_Finalize(). Fix pybind#4459
a29a591
@Skylion007 Skylion007 mentioned this issue Feb 2, 2023
Merged
@Skylion007 Skylion007 added bug and removed triage New bug, unverified labels Feb 4, 2023
Skylion007 added a commit that referenced this issue Feb 6, 2023
@Skylion007
bugfix: Keep registered types until after Py_Finalize(). Fix #4459 (#…
b2c1978 
…4486)

* Keep registered types until after Py_Finalize(). Fix #4459

* Address reviewer comments
@rwgk rwgk mentioned this issue Feb 8, 2023
Closed
rwgk added a commit to rwgk/pybind11 that referenced this issue Feb 8, 2023
@rwgk
Revert "bugfix: Keep registered types until after Py_Finalize(). Fix p…
4efdec8 
…ybind#4459 (pybind#4486)"

This reverts commit b2c1978.

See pybind#4500 for background.
@Skylion007 Skylion007 reopened this Feb 8, 2023
@Skylion007
Copy link
Collaborator

Oh boy, this broke some more things.

@rwgk rwgk closed this as completed in f8713ec Feb 8, 2023
@Skylion007 Skylion007 reopened this Feb 8, 2023
Skylion007 added a commit to Skylion007/pybind11 that referenced this issue Feb 8, 2023
@Skylion007
Fix pybind#4459 (cpp atexit callbacks) without segfault
7d63557
@Skylion007 Skylion007 linked a pull request Feb 8, 2023 that will close this issue
Open
@rwgk rwgk mentioned this issue Feb 11, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug help wanted
Projects
None yet
Milestone
No milestone
3 participants
@chrreisinger @rwgk @Skylion007