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.

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

Add release_gil_before_calling_cpp_dtor annotation for class_ #5522

Merged
merged 2 commits into from
Feb 16, 2025

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Feb 15, 2025

Description

Closes #1446.

This PR is a backport of:

It introduces the py::release_gil_before_calling_cpp_dtor option (py::class_ argument). For background, see the description of google/pybind11clif#30088 and #1446.

  • The core change (in pybind11.h class_::dealloc()) is to add ReleaseGIL-try-catch-ReaquireGIL-rethrow logic around the existing code calling the C++ destructor of wrapped objects.

  • All the rest is more-or-less boilerplate code to add the py:: release_gil_before_calling_cpp_dtor annotation, then use it to enable the new feature if that annotation is provided by the user.


Note for completeness:

These are identical to the current versions on the pybind11clif main branch @ commit 4841661:

  • test_class_release_gil_before_calling_cpp_dtor.cpp
  • test_class_release_gil_before_calling_cpp_dtor.py

@rainwoodman (original reviewer) for visibility.


Suggested changelog entry:

A ``py::release_gil_before_calling_cpp_dtor`` option (for ``py::class_``) was added to resolve the long-standing issue #1446.

…if#30092 (minor fixes).

Note for completeness:

These are identical to the current versions on the pybind11clif main branch (@ commit 4841661df5daf26ecdedaace54e64d0782e63f64):

* test_class_release_gil_before_calling_cpp_dtor.cpp
* test_class_release_gil_before_calling_cpp_dtor.py
@rwgk
Copy link
Collaborator Author

rwgk commented Feb 15, 2025

@gentlegiantJGC could you please review (and formally approve if it looks good to you)?

(This PR is pretty much exactly as reviewed previously by @rainwoodman.)

@rwgk rwgk marked this pull request as ready for review February 15, 2025 18:59
@rwgk rwgk requested a review from henryiii as a code owner February 15, 2025 18:59
@rwgk
Copy link
Collaborator Author

rwgk commented Feb 15, 2025

Looking at this with a fresh eye, I got suspicious about a potential data race in the test, and ChatGPT agrees that there is a problem:

It looks like a very easy fix. I'll add a commit in a minute.

…dtor.cpp

The original intent was to let the singleton leak, but making that tread-safe is slightly more involved than this solution. It's totally fine in this case if the RegistryType destructor runs on process teardown.

See also: pybind#5522 (comment)
Copy link
Contributor

@gentlegiantJGC gentlegiantJGC left a comment

Choose a reason for hiding this comment

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

The implementation looks fine to me.
I got a bit lost in the test code. You might want someone with more knowledge of the internals of pybind11 and python to look over this.

@rwgk
Copy link
Collaborator Author

rwgk commented Feb 16, 2025

The implementation looks fine to me. I got a bit lost in the test code. You might want someone with more knowledge of the internals of pybind11 and python to look over this.

Thanks @gentlegiantJGC!

Since the test was reviewed already (almost as-is), I'll go ahead with merging.

@rwgk rwgk merged commit 34a118f into pybind:master Feb 16, 2025
76 checks passed
@rwgk rwgk deleted the dtorgil_master branch February 16, 2025 19:01
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Feb 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs changelog Possibly needs a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Blocking destructors and the GIL
2 participants