Free created types' tp_name in default metaclass #978
Conversation
jagerman
force-pushed
the
free-tp_name
branch
2 times, most recently
from
ec0937c to
a1fa350
Compare
|
👍 Don't think it's an overkill, looks quite straightforward. Keeping the the codebase valgrind-clean is nice. What about |
|
Plugging the leak is definitely important, but the implementation with a weakref callback looks like total overkill to me. Why not use the metaclass' |
|
I tried that, but at least with a simple approach it results in invalid frees because the metaclass's |
|
Another way we could go about this is to store the type names in a |
include/pybind11/common.h
Outdated
| @@ -484,6 +484,7 @@ struct internals { | |||
| std::forward_list<void (*) (std::exception_ptr)> registered_exception_translators; | |||
| std::unordered_map<std::string, void *> shared_data; // Custom data to be shared across extensions | |||
| std::vector<PyObject *> loader_patient_stack; // Used by `loader_life_support` | |||
| std::forward_list<std::string> type_names; // Stores the std::strings holding created types' tp_names | |||
There was a problem hiding this comment.
Minor: might as well name it static_strings or the like? (unlike all other fields of internals, this one you would never actually read, it's just there for storage with static lifetime and doesn't really have to do much with types)
Also, in case we ever need it again?
const char* detail::strdup(const std::string& what) {
auto& internals = get_internals();
internals.static_strings.emplace_front(what);
return internals.static_strings.front().c_str();
}
Types need `tp_name` set to a C-style string, but the current `strdup` ends up with a leak (issue pybind#977). This avoids the strdup by storing the `std::string` in internals so that during interpreter shutdown it will be properly destroyed.
|
Looks great. Merge? |
Fixes #977, via a weakref added to the type.
This feels like a bit of overkill, though; the leak in #977 seems fairly minor.