native_enum: add capsule containing enum information and cleanup logic#5871
native_enum: add capsule containing enum information and cleanup logic#5871oremanj merged 3 commits intopybind:masterfrom
Conversation
| break | ||
| else: | ||
| pytest.fail( | ||
| f"Could not delete bindings such as {next(wr for wr in wrs if wr() is not None)!r}" |
There was a problem hiding this comment.
f"Could not delete bindings such as {next(wr for wr in wrs if wr() is not None)!r} with {num_gc_collect} gc.collect() cycles"
That's mainly to insert a hint for future maintainers, to more immediately realize that we're only trying a certain number of cycles.
BTW: Why 5? Why not e.g. 10, to minimize distractions later?
There was a problem hiding this comment.
I've seen this idiom in many projects and none of them even went as many as 5 times (2-4 is common). The only reason it's necessary to do more than once is because finalizers (__del__ methods and weakref callbacks) for objects collected in the first cycle may create more garbage.
The comment on our gc_collect() says it goes three times, but the implementation goes five. I have some suspicion that this was added for GraalPy before we realized that gc.collect() isn't reliable on GraalPy at all.
At some point, pounding it harder isn't going to help, and the extra GC passes take a noticeable amount of time.
I added a comment on the definition of num_gc_collect and a comment before the failure explaining what likely caused it.
tests/test_native_enum.cpp
Outdated
| .value("low", altitude::low) | ||
| .finalize(); | ||
| }); | ||
| m.attr("bind_altitude")(m); |
There was a problem hiding this comment.
Do we need this here? Vs just calling it from test_native_enum.py when we need it? And deleting it when we're done with the test (rather than restoring it as you have it right now at the end of test_unregister_native_enum_when_destroyed())?
My main worry is stability when running tests in parallel.
There was a problem hiding this comment.
Sure, that's cleaner.
rwgk
left a comment
rwgk
left a comment
There was a problem hiding this comment.
Awesome, that's all very clear now.
2 participants
Description
Add a capsule to each
pybind11::native_enumthat wraps a small structure containing some information about the enum. Since we don't create atype_infofor native enums, this is the only way to introspect a native enum for its C++-level properties at runtime. The capsule's destructor also unregisters the native enum from the pybind11 internals, which fixes a bug where to-Python conversions of native enums could try to access a destroyed type object. See the newly added test for a reproducer.This was split off from #5800. The size and signedness added here will be used in that PR.
Suggested changelog entry:
Each Python enum type created by a
py::native_enumbinding statement will now unregister its pybind11 binding when the Python enum type is destroyed. This prevents a use-after-free when returning an instance of a destroyed enum type to Python.