Avoid local_internals destruction

[bogdan-lab]

Description

The first call of get_local_internals() creates static variable. It can be, that the first call to get_local_internals() happens in the finalize_interpreter(). If this function is called from another static object destructor, code violates the order of static variable destruction. Such situation can happen, for example, if one decides to store scoped_interpreter in a singleton class. The broken order of the destruction of statics can result in program freezing inside finalize_interpreter (at least on Windows it does).

By making local_internals a pointer we avoid its destruction and the problem goes away.

This PR allows to avoid explicit call to detail::get_local_internals() from the user side in the situation described in this issue
#4172

Application of this change is wider than it is shown in the issue above. It also solves the problem if user calls finalize_interpreter directly in the destructor or situation, when interpreter was created after the singleton wrapper constructor returned.

Suggested changelog entry:

* `get_local_internals()` was made compatible with `finalize_interpreter()`, fixing potential freezes during interpreter finalization.

[rwgk]

@jbms could you please take a look?
(LGTM, it's a standard solution to the problem.)

[jbms]

LGTM

[rwgk]

This makes a lot of sense, thanks! I'm almost surprised that this didn't bite sooner.

Did you look for other similar situations in the code?

[bogdan-lab]

No, I didn't. I have found this problem only because it caused that freezing in finalize_interpreter in my program.

[Skylion007]

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

[Skylion007]

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

[jbms]

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.

[Skylion007]

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

[jbms]

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.

[bogdan-lab]

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...

[rwgk]

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.

[jbms]

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.

[henryiii]

Might be nice to follow up, but looks like a safe patch with an improvement for now.