Skip to content
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

Add NOTIFICATION_PREDELETE_CLEANUP notification to fix C# Dispose() #83670

Merged
merged 1 commit into from
Nov 9, 2023
Merged

Add NOTIFICATION_PREDELETE_CLEANUP notification to fix C# Dispose() #83670

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions core/object/object.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ bool Object::_predelete() {
notification(NOTIFICATION_PREDELETE, true);
if (_predelete_ok) {
_class_name_ptr = nullptr; // Must restore, so constructors/destructors have proper class name access at each stage.
notification(NOTIFICATION_PREDELETE_CLEANUP, true);
}
return _predelete_ok;
}
Expand Down
2 changes: 2 additions & 0 deletions core/object/object.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -801,6 +801,8 @@ class Object {
NOTIFICATION_POSTINITIALIZE = 0,
NOTIFICATION_PREDELETE = 1,
NOTIFICATION_EXTENSION_RELOADED = 2,
// Internal notification to send after NOTIFICATION_PREDELETE, not bound to scripting.
NOTIFICATION_PREDELETE_CLEANUP = 3,
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be clearly reserved so future notifications don't use it, especially if we use a system based on registered values to track overlap

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems we use different magnitudes1 to avoid overlap (e.g.: the Node notifications are 1X, CanvasItem notifications are 3X, Control/Node3D notifications are 4X, Window notifications are 10XX). But I agree it can be error prone.

Footnotes

  1. Not sure if magnitude is the right word. ↩

};

/* TYPE API */
Expand Down
27 changes: 17 additions & 10 deletions modules/mono/csharp_script.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1985,24 +1985,31 @@ const Variant CSharpInstance::get_rpc_config() const {

void CSharpInstance::notification(int p_notification, bool p_reversed) {
if (p_notification == Object::NOTIFICATION_PREDELETE) {
// When NOTIFICATION_PREDELETE is sent, we also take the chance to call Dispose().
// It's safe to call Dispose() multiple times and NOTIFICATION_PREDELETE is guaranteed
if (base_ref_counted) {
// At this point, Dispose() was already called (manually or from the finalizer).
// The RefCounted wouldn't have reached 0 otherwise, since the managed side
// references it and Dispose() needs to be called to release it.
// However, this means C# RefCounted scripts can't receive NOTIFICATION_PREDELETE, but
// this is likely the case with GDScript as well: https://github.com/godotengine/godot/issues/6784
return;
}
} else if (p_notification == Object::NOTIFICATION_PREDELETE_CLEANUP) {
// When NOTIFICATION_PREDELETE_CLEANUP is sent, we also take the chance to call Dispose().
// It's safe to call Dispose() multiple times and NOTIFICATION_PREDELETE_CLEANUP is guaranteed
// to be sent at least once, which happens right before the call to the destructor.

predelete_notified = true;

if (base_ref_counted) {
// It's not safe to proceed if the owner derives RefCounted and the refcount reached 0.
// At this point, Dispose() was already called (manually or from the finalizer) so
// that's not a problem. The refcount wouldn't have reached 0 otherwise, since the
// managed side references it and Dispose() needs to be called to release it.
// However, this means C# RefCounted scripts can't receive NOTIFICATION_PREDELETE, but
// this is likely the case with GDScript as well: https://github.com/godotengine/godot/issues/6784
// At this point, Dispose() was already called (manually or from the finalizer).
// The RefCounted wouldn't have reached 0 otherwise, since the managed side
// references it and Dispose() needs to be called to release it.
return;
}

_call_notification(p_notification, p_reversed);

// NOTIFICATION_PREDELETE_CLEANUP is not sent to scripts.
// After calling Dispose() the C# instance can no longer be used,
// so it should be the last thing we do.
GDMonoCache::managed_callbacks.CSharpInstanceBridge_CallDispose(
gchandle.get_intptr(), /* okIfNull */ false);

Expand Down
Loading