Auto-release static GDTypes at exit. #114790
Conversation
This fixes "unclaimed StringName" warnings and improves engine shutdown correctness.
Ivorforce
force-pushed
the
gdtype-autorelease
branch
from
f03208a to
17ef5af
Compare
|
|
||
| for (GDType **type : gdtype_autorelease_pool) { | ||
| if (!type) { | ||
| WARN_PRINT("GDType in autorelease pool was cleaned up before being auto-released. Ignoring."); |
There was a problem hiding this comment.
| WARN_PRINT("GDType in autorelease pool was cleaned up before being auto-released. Ignoring."); | |
| WARN_PRINT_ONCE("GDType in autorelease pool was cleaned up before being auto-released. Ignoring."); |
I'd say, as it doesn't contain any details it'll be noisy if it floods with many of these
There was a problem hiding this comment.
The warning isn't expected to ever print anyway, it's just a precaution for misuse. With multiple prints it would at least be possible to estimate how many GDTypes are affected.
There was a problem hiding this comment.
I'd suggest replacing it with a counter in that case to make it less noisy
There was a problem hiding this comment.
If this is meant to be an impossible scenario, wouldn't DEV_ASSERT be preferable?
There was a problem hiding this comment.
I would have agreed a year ago, but since working on Godot more I've grown more into the "don't crash if it's not fatal" mindset, at least for Godot. Warnings should be plenty to find the situation if it ever does arise.
There was a problem hiding this comment.
I'd argue that's only relevant when something's directly accessible by the end-user. But we don't clearly define if internals should be handled to a different standard, so I digress
There was a problem hiding this comment.
It could always be DEV_CHECK_ONCE() if the goal is to keep this out of normal builds? I'm not sure it matters too much in this particular case, though
dsnopek
left a comment
dsnopek
left a comment
There was a problem hiding this comment.
Thanks! This seems fine to me :-)
Between WARN_PRINT() and WARN_PRINT_ONCE(), I'd personally probably also lean towards WARN_PRINT_ONCE() - but it probably won't end up really mattering in this case
4 participants
Currently,
GDCLASSownedGDTypeobjects are never torn down. This leads to "unclaimed StringName" warnings at engine shutdown (when--verboseis used). This PR fixes this by tearing down theseGDTypeinstances at shutdown.Explanation
Every compile time Object subclass (
GDCLASS) statically owns aGDType *. This is allocated and initialized lazily onget_gdtype_staticaccess.Because of engine launch order, these
GDTypearen't immediately (or even necessarily) registered inClassDB. The types are only registered whenGDREGISTER_CLASS(or an alternative) is called, or when the first instance is created.Practically, this means there may be any number of
GDTypeinstances hanging around that are not registered inClassDB. To easily clean them up, I simply added an autorelease pool toClassDB.In the future, we might consider keeping a closer eye on static
GDType(for example, by requiring explicitGDREGISTER_CLASScalls for every type, or by only creating them on explicit register), which would eliminate the need for this autorelease pool.