Skip to content

Avoid local_internals destruction #4192

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 2 commits into from
Sep 21, 2022
Merged

Avoid local_internals destruction #4192

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dfa2c0c
Avoid local_internals destruction
bogdan-lab Sep 20, 2022
c296ce9
Add link to relevant google style guide discussion
bogdan-lab Sep 21, 2022
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
9 changes: 7 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 @@ -514,8 +514,13 @@ struct local_internals {

/// Works like `get_internals`, but for things which are locally registered.
inline local_internals &get_local_internals() {
static local_internals locals;
return locals;
// Current static can be created in the interpreter finalization routine. If the later will be
// destroyed in another static variable destructor, creation of this static there will cause
// static deinitialization fiasco. In order to avoid it we avoid destruction of the
// local_internals static. One can read more about the problem and current solution here:
// https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables
static auto *locals = new local_internals();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, wait doesn't this leak now? as in the local_internals memory is never freed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Which is fine I suppose since there doesn't appear to be a good alternative, but we need to document it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Defining a function-local static variable that leaks like this is a common pattern (recommended by Google C++ style guide, for example: https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables). Since the destructor would only be called when the program exits, the "leak" is irrelevant as long as it is just memory. You can define it in a slightly more complicated way (https://github.com/google/tensorstore/blob/master/tensorstore/internal/no_destructor.h) to avoid the extra heap allocation, but one extra heap allocation is presumably not very important.

There are actually two different cases to consider here:

  • For get_local_internals used within an extension module, Python never actually unloads extension modules (because in general it is not really feasible to do that safely), so the destructor is never invoked anyway.
  • The only case where the destructor previously was getting called was in the case where get_local_internals is used outside of an extension module, e.g. when using an embedded interpreter, or when statically linking other libraries directly to the Python interpreter itself (as we do internally at Google).

Looking more closely at finalize_interpreter, though, the use of get_local_internals seems suspect:

pybind11/include/pybind11/embed.h

Line 229 in 424ac4f

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

By default there will be a separate copy of local_internals for each extension module, and for the embedding program itself. Therefore, this will only free types and exception handlers that were registered module-local by whichever module calls finalize_interpreter. Any types registered by other modules will remain registered. To work properly, this logic would need to change. Though given all of the bugs and limitations with multiple initialization and finalization of the python interpreter, I think it might be better to just not support that at all.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jbms How would suggest changing this logic then? Removing that line?

Copy link
Contributor

Choose a reason for hiding this comment

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

Either to remove those two lines from finalize_interpreter, and document that initialize_interpreter should not be called more than once, or have the global internal state keep a reference to all of the local internals states, so that finalize_interpreter can free all of the local internals states.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first option sounds like something that can break someones old code. Don't we care about it?
About the second option I do not quite understand how will it fix the bug. We still will be able to create static in the destructor routine. Or maybe I do not understand the second option...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't we care about it?

Thinking pragmatically, I think the original fix is great, maybe we could add a link to the comment, e.g. what Jeremy provided (https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables). The original fix solves an immediate problem without disturbing anything else. All it does is shift the memory for the local internals from the data section of the binary to the heap (inconsequential side-effect) and not run the destructors (desired fix).

Thinking idealistically, long-term I'd really want to make it difficult to use pybind11 to initialize/finalize the interpreter multiple times. It's a trap / illusion. But I think it's best to not venture there in this PR, or now.

Copy link
Contributor

Choose a reason for hiding this comment

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

The option to have finalize_interpreter free all of the local states would require some changes to how local_internals works, which would hopefully be done in such a way as to avoid this bug.

I agree that the current PR is the simplest fix, and we can address this other issue of finalize_interpreter not freeing all of the local internals as a separate issue.

return *locals;
}

/// Constructs a std::string with the given arguments, stores it in `internals`, and returns its
Expand Down