Skip to content

Fix TSan warning in sub-interpreter test #5729

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 1 commit into from
Jun 18, 2025
Merged

Fix TSan warning in sub-interpreter test #5729

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 5 additions & 2 deletions include/pybind11/detail/internals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,8 +513,11 @@ class internals_pp_manager {
/// Drop all the references we're currently holding.
void unref() {
#ifdef PYBIND11_HAS_SUBINTERPRETER_SUPPORT
last_istate_.reset();
internals_tls_p_.reset();
if (get_num_interpreters_seen() > 1) {
last_istate_.reset();
internals_tls_p_.reset();
return;
}
#endif
internals_singleton_pp_ = nullptr;
Copy link
Collaborator

Choose a reason for hiding this comment

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

After looking around for a few minutes I'm thinking: This will only ever be reached from unsafe_reset_internals_for_single_interpreter() in tests/test_embed/test_subinterpreter.cpp. Is that correct (and intentional)?

Would it make sense to leave a small comment to explain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's also reached when there's a single interpreter, from finalize_interpreter.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@b-pass Could you please take a look here?

It's a short conversation. It ends with this question:

Did you perhaps mean to write:

last_istate_.reset();
internals_tls_p_.reset();
if (get_num_interpreters_seen() == 1) {
    internals_singleton_pp_ = nullptr;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

get_num_interpreters_seen is not supposed to go down (but it does in the tests, to try not to have that state bleed between tests). Since it never goes down normally, once it increases basically we stop using the singleton_pp and start instead using the two thread locals.

So resetting the singleton_pp pointer shouldn't be necessary once the count has increased (but it was being changed before, for the tests, but created this data race in the tests).

The structure of the code (#if, code, return, #endif) mirrors the function above it, and is that way to avoid having an #else with duplicate code. But it could be changed to maybe read a little more linearly if you want....

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry I'm still unclear TBH.

Considering only this specific case:

  • PYBIND11_HAS_SUBINTERPRETER_SUPPORT is true
  • get_num_interpreters_seen() == 1

I believe before this PR this will run:

last_istate_.reset();
internals_tls_p_.reset();
internals_singleton_pp_ = nullptr;

With this PR, only this:

internals_singleton_pp_ = nullptr;

I.e. the two .reset() are skipped with this PR. Is that a correct understanding?

Could you please confirm that skipping the two .reset() is intentional?

Copy link
Contributor Author

@b-pass b-pass Jun 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Skipping the reset was intentional, as those are not used/touched until the count is greater than 1. But you're also correct, the real purpose of the PR was to avoid changing internals_singleton_pp_ when the count is above 1, and, so skipping the resets was not necessary.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it, thanks!

}
Expand Down
14 changes: 12 additions & 2 deletions tests/test_embed/test_subinterpreter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,21 @@ bool has_state_dict_internals_obj();
uintptr_t get_details_as_uintptr();

void unsafe_reset_internals_for_single_interpreter() {
// unsafe normally, but for subsequent tests, put this back.. we know there are no threads
// running and only 1 interpreter
// NOTE: This code is NOT SAFE unless the caller guarantees no other threads are alive
// NOTE: This code is tied to the precise implementation of the internals holder

// first, unref the thread local internals
py::detail::get_internals_pp_manager().unref();
py::detail::get_local_internals_pp_manager().unref();

// we know there are no other interpreters, so we can lower this. SUPER DANGEROUS
py::detail::get_num_interpreters_seen() = 1;

// now we unref the static global singleton internals
py::detail::get_internals_pp_manager().unref();
py::detail::get_local_internals_pp_manager().unref();

// finally, we reload the static global singleton
py::detail::get_internals();
py::detail::get_local_internals();
}
Expand Down
Loading