Custom class instance extending Reference already freed when it receives NOTIFICATION_PREDELETE #6784
Comments
|
I experienced this crash today when doing some tests. This is the cause:
ping @reduz |
|
I think we discussed this before, and the proposed solution was to simply not send that notification to the script, as it does not make much sense. Was this implemented somehow? |
|
No, it wasn't. You proposed not calling |
|
Ah, alright, then will simply do that.. should be enough
…On Aug 5, 2017 2:43 PM, "Ignacio Etcheverry" ***@***.***> wrote:
No, it wasn't. You proposed not calling NOTIFICATION_PREDELETE for
scripts. I don't have anything against it, but I wonder if there is people
who rely on this notification.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6784 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AF-Z27RcQ2dFcYdvi3L_s0z0vkjp_Hdrks5sVKnCgaJpZM4KTJ2c>
.
|
|
ups. |
|
@karroffel This is what happens when you switch from rats to squids. |
|
@reduz Maybe, instead of disabling |
|
I would disable the predelete notification in Mono, not needed in the language and no need for hacks. |
|
I can disable it in Mono if not really needed. But this is a GDScript issue. What happened with 65cc56c? I can't see it in the history. |
|
|
So, what do we do with this? |
|
¯_(ツ)_/¯ I was re-reading the part of the code I mentioned above is causing the problem, and I noticed we could change the godot/modules/gdscript/gdscript_function.cpp Lines 368 to 374 in 5307043 However, it's still not safe, during this call, to pass Reference *ref = Object::cast_to<Reference>(obj); // obj is the `self` we passed to the engine
if (ref) {
Ref<Reference> r(ref);
}It may be simply better to not send |
|
I stumbled upon this issue in 3.2, linking a more thorough and updated reproduction project: Also wonder if there's any connection with #17681. |
|
The motivation to run predelete on Reference is when I want to free some nested data structures: class_name DoublyLinkedList
extends Reference
var front : Element
var back : Element
# implementation details...
func clear():
var e = front
while e:
var next = e.next
e.free()
e = null
e = next
func _notification(what):
match what:
NOTIFICATION_PREDELETE:
clear() # hmm, instance is gone, can't free elements
# implementation details...
class Element extends Object:
var data
var next : Element
var prev : Element
func _init(p_data):
data = p_dataExtending |
akien-mga
changed the title
Reference and overrides _notification and receives notification
|
I tried the repro in 19f72be and neither object seems to receive NOTIFICATION_PREDELETE... The issue is still reproducible in 3.2.3 though. |
|
Hi, freinds, I need this notification for my code. class_name Tree
class TreeNode:
var data
var parent : TreeNode
var left : TreeNode
var right : TreeNode
var head : TreeNode
func _notification(what):
if what == NOTIFICATION_PREDELETE:
clear() # Crashed, so I can't release all the TreeNode auto
func clear():
free_node(head)
func free_node(node:TreeNode):
if node==null:
return
if node.left:
free_node(node.left)
node.left = null
if node.right:
free_node(node.right)
node.right = nullAny update for this issue? |
|
The issue is still reproducible in 4.2 though. |
|
Ok as a thread necromancer, it still doesn't work in version v4.2.1.stable.official [b09f793] (what a surprise). As a hint for the people with the same problem: #82841 shows a workaround for the problem. Apparently script variables are not yet deleted at that point, so they can still be used. So just save everything to variables and the problem is solved, I suppose. Big question: Why are script variables still valid, but the self pointer is not? Finally, a very uneducated suggestion: Can the refcounter emit the notification just before the counter jumps to 0? Even if it should mean that - if you want to have a deconstructor (with a valid self pointer) - you have to inherit from RefCouned? |
|
Unfortunately, I have to confirm what @Wesmer says. The |
|
Bumping this issue, as I'm having the same problem. Has anyone found any good workarounds? |
I played around a bit. (Version: v4.2.1.stable.official [b09f793]) extends Node
class TEST extends Texture2D:
var test_class = "class_abc"
var this: TEST = self
func _init() -> void:
# Decrement the internal reference counter again
# because this = self increments it
unreference()
func _notification(what):
if what == NOTIFICATION_PREDELETE:
print(test_class) # OK class_abc
print(self.test_class) # OK class_abc
print(self, self == null) # null , true
print(this.has_alpha()) # OK true
# print(self.has_alpha()) # crash
# print(has_alpha()) # crash
this.test_method() # OK hi
func test_method():
print("hi")
func _ready() -> void:
var t = TEST.new()
|
|
That's awesome @Wesmer ! It also doesn't seem to cause any memory leaks. (Also tested with resource monitor on Windows.) extends Node
class RefTest extends RefCounted:
var foo := "bar"
var this : RefTest = self
func _init() -> void:
unreference()
func _notification(what):
if what == NOTIFICATION_PREDELETE:
pass
func _ready() -> void:
print(OS.get_static_memory_usage())
for i in range(10000000):
var r := RefTest.new()
print(OS.get_static_memory_usage()) # Outputs roughly the same as above.I'd be a bit worried about using this in production though as it seems to rely on undocumented engine behavior? |
Well, it depends on how much you like hacky engineering. If you absolutely don't like the solution, use a node/objekt, as this problem (apparently) only applies to RefCounted-Objects. extends Node
func deconstructor_code():
prints("DECONSTRUCTOR", self) # valid
is_queued_for_deletion() # ok
func _notification(what):
if what == NOTIFICATION_PREDELETE:
prints("Node deconstructor", self) # self is Valid
deconstructor_code() # OK
self.get_script() #OKOr use a second class: extends Node
class Cleaner extends RefCounted:
func deconstructor_code():
prints(" Cleaner DECONSTRUCTOR", self) # valid
self.get_class() # ok
some_method() # OK
func some_method():
pass
class SomeClass extends RefCounted:
var t = Cleaner.new()
func _notification(what):
if what == NOTIFICATION_PREDELETE:
prints("Objekt deconstructor", self) # unvalid
t.deconstructor_code() # ok
func _ready() -> void:
var t = SomeClass.new()It is best to avoid the deconstructor of Refcounted objects. Otherwise you will probably not be able to avoid a workaround. However, the two-class solution is better in the sense that it avoids tinkering with the counter. Because if you forget |
|
Sorry for raising this issue from the dead, but I have encountered it while trying to free a |
Operating system or device - Godot version:
macOS 10.11.6 - v2.1.stable.official
Issue description (what happened, and what was expected):
Created subclass, overrides
_notification. When the instance should be sentNOTIFICATION_PREDELETE(the only notification Reference gets?) and freed the game crashes.Expected to receive notification.
Steps to reproduce:
Referenceand override_notificationLink to minimal example project (optional but very welcome):
godot-ref-test.zip
The text was updated successfully, but these errors were encountered: