Memory leak with circular reference in GDscript

[touilleMan]

Hi,

Godot reference counting mechanism doesn't support circular references. This is fine for simple construction (e.g. loading References) but given GDscript is build on this, you can write leaking program pretty easily:

func _ready():
	set_process(true)

class Leaker:
	var stuff = null
	var other = null
	func _init():
		# Dummy data to make the leak more visible
		self.stuff = []
		for x in range(1000):
			self.stuff.append(x)

	func connect(other):
		self.other = other

func _process(delta):
	var a = Leaker.new()
	var first = a
	for i in range(100):
		var b = Leaker.new()
		a.connect(b)
		a = b
	a.connect(first)  # Comment this line to remove the leak !

This is not a trouble for me (beside a circular dependency checker is complex to code and slow to run) but I think it's something users should be aware of.
I couldn't see any information about this behavior in the documentation so I think there should be a warning about this in GDscript more efficiently

[reduz]

I think there was something about this somewhere, but I can't remember
where.
I thought about making a circular reference checker, it shouldn't be that
difficult, but not that many users complained about it so far..

On Sat, Nov 5, 2016 at 5:32 AM, Emmanuel Leblond notifications@github.com
wrote:

Hi,

Godot reference counting mechanism doesn't support circular references.
This is fine for simple construction (e.g. loading References) but given
GDscript is build on this, you can write leaking program pretty easily:

func _ready():
set_process(true)
class Leaker:
var stuff = null
var other = null
func _init():
# Dummy data to make the leak more visible
self.stuff = []
for x in range(1000):
self.stuff.append(x)

func connect(other):
self.other = other
func _process(delta):
var a = Leaker.new()
var first = a
for i in range(100):
var b = Leaker.new()
a.connect(b)
a = b
a.connect(first) # Comment this line to remove the leak !

This is not a trouble for me (beside a circular dependency checker is
complex to code and slow to run) but I think it's something users should be
aware of.
I couldn't see any information about this behavior in the documentation so
I think there should be a warning about this in GDscript more efficiently
http://docs.godotengine.org/en/stable/reference/gdscript_more_efficiently.html#pointers-referencing

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#7038, or mute the thread
https://github.com/notifications/unsubscribe-auth/AF-Z2z8V7M3ms4VZ2VriNfJfbZCoRNYCks5q7D8BgaJpZM4KqNGR
.

[vnen]

There's weakref() but not much info about it.

[ghost]

First of all thank you for your report and sorry for the delay.

We released Godot 3.0 in January 2018 after 18 months of work, fixing many old issues either directly, or by obsoleting/replacing the features they were referring to.

We still have hundreds of issues whose relevance/reproducibility needs to be checked against the current stable version, and that's where you can help us.
Could you check if the issue that you described initially is still relevant/reproducible in Godot 3.0 or any newer version, and comment about it here?

For bug reports, please also make sure that the issue contains detailed steps to reproduce the bug and, if possible, a zipped project that can be used to reproduce it right away. This greatly speeds up debugging and bugfixing tasks for our contributors.

Our Bugsquad will review this issue more in-depth in 15 days, and potentially close it if its relevance could not be confirmed.

Thanks in advance.

Note: This message is being copy-pasted to many "stale" issues (90+ days without activity). It might happen that it is not meaningful for this specific issue or appears oblivious of the issue's context, if so please comment to notify the Bugsquad about it.

[touilleMan]

@Noshyaar I've just reproduce the leak with Godot 3.0.2, so this issue is still valid

[reduz]

GDScript probably needs something to detect cycles and warn the user, but I don't think this is a bug.. rather an enhancement. If anyone wants to work on it, feel welcome.

[reduz]

Also, so far not happening for 3.1

[KoBeWi]

I tried the reproduction code but it doesn't seem to leak. (3.3 rc7)

EDIT:
Nevermind, checked wrong console. But seems fixed on master.

[nathanfranke]

If it works in master, should the milestone be updated to 3.x?

[akien-mga]

We should confirm that it doesn't leak in master, and probably close it if that's the case. At this point it's highly unlikely that it can be fixed easily in 3.x if it couldn't be before rewriting GDScript.

[nathanfranke]

I can't even reproduce in 3.3.4, which makes sense considering this issue is 5 years old.

Running: /usr/bin/godot --path /home/nathan/workspace/test/Test --remote-debug 127.0.0.1:6007 --allow_focus_steal_pid 70944 --position 2368,240
Godot Engine v3.3.4.stable.arch_linux - https://godotengine.org
XInput: Refreshing devices.
XInput: No touch devices found.
Detecting GPUs, set DRI_PRIME in the environment to override GPU detection logic.
Only one GPU found, using default.
Using GLES3 video driver
OpenGL ES 3.0 Renderer: NVIDIA GeForce GTX 960/PCIe/SSE2
OpenGL ES Batching: ON
        OPTIONS
        max_join_item_commands 16
        colored_vertex_format_threshold 0.25
        batch_buffer_size 16384
        light_scissor_area_threshold 1
        item_reordering_lookahead 4
        light_max_join_items 32
        single_rect_fallback False
        debug_flash False
        diagnose_frame False
PulseAudio: context other
PulseAudio: context other
PulseAudio: context other
PulseAudio: context ready
PulseAudio: Detecting channels for device: alsa_output.usb-C-Media_Electronics_Inc._USB_Audio_Device-00.analog-stereo
PulseAudio: detected 2 channels
PulseAudio: audio buffer frames: 512 calculated latency: 11ms
JoypadLinux: udev enabled and loaded successfully.
 
CORE API HASH: 3081034046582745905
EDITOR API HASH: 16295317243152768217
Loading resource: res://default_env.tres
Loading resource: res://Scene.tscn
Loading resource: res://Scene.gd
ERROR: ~List: Condition "_first != __null" is true.
   At: ./core/self_list.h:112.
ERROR: ~List: Condition "_first != __null" is true.
   At: ./core/self_list.h:112.
PulseAudio: context terminated

[akien-mga]

I can still reproduce the leak in 3.x, with the code in OP, after replacing connect with bitconnect otherwise it doesn't compile.

[nathanfranke]

I replaced it with konnect, but I don't see any memory leaks, running the project with verbose stdout. Do I need to do anything else to show the leaks in the console?

[akien-mga]

I think you have to run it directly (not through the editor) so that the --verbose flag is honored, or enable verbose stdout in the project settings.

Here's the output for me running the project directly (truncated because it was spammy):

Godot Engine v3.4.beta.custom_build.cdbfa37f1 - https://godotengine.org
XInput: Refreshing devices.
XInput: No touch devices found.
Detecting GPUs, set DRI_PRIME in the environment to override GPU detection logic.
Found renderers:
Renderer 0: Mesa Intel(R) HD Graphics 630 (KBL GT2) with priority: 20
Renderer 1: AMD VEGAM (DRM 3.42.0, 5.14.9-desktop-3.mga9, LLVM 12.0.1) with priority: 30
Using renderer: AMD VEGAM (DRM 3.42.0, 5.14.9-desktop-3.mga9, LLVM 12.0.1)
Found discrete GPU, setting DRI_PRIME=1 to use it.
Note: Set DRI_PRIME=0 in the environment to disable Godot from using the discrete GPU.
Using GLES3 video driver
OpenGL ES 3.0 Renderer: AMD VEGAM (DRM 3.42.0, 5.14.9-desktop-3.mga9, LLVM 12.0.1)
OpenGL ES Batching: ON
	OPTIONS
	max_join_item_commands 16
	colored_vertex_format_threshold 0.25
	batch_buffer_size 16384
	light_scissor_area_threshold 1
	item_reordering_lookahead 4
	light_max_join_items 32
	single_rect_fallback False
	debug_flash False
	diagnose_frame False
PulseAudio: context other
PulseAudio: context other
PulseAudio: context other
PulseAudio: context ready
PulseAudio: Detecting channels for device: alsa_output.pci-0000_00_1f.3.analog-stereo
PulseAudio: detected 2 channels
PulseAudio: audio buffer frames: 512 calculated latency: 11ms
JoypadLinux: udev enabled and loaded successfully.
 
CORE API HASH: 719258447014722995
EDITOR API HASH: 5183976363241295750
Loading resource: res://default_env.tres
Loaded builtin certs
Loading resource: res://Node2D.tscn
Loading resource: res://Node2D.gd
ERROR: Condition "_first != nullptr" is true.
   at: ~List (./core/self_list.h:108)
ERROR: Condition "_first != nullptr" is true.
   at: ~List (./core/self_list.h:108)
PulseAudio: context terminated
WARNING: ObjectDB instances leaked at exit (run with --verbose for details).
     at: cleanup (core/object.cpp:2063)
Leaked instance: Reference:2093
Leaked instance: Reference:2035
Leaked instance: Reference:2121

... plenty more ...

Leaked instance: Reference:2092
Leaked instance: Reference:1976
Leaked instance: Reference:1713
Leaked instance: GDScript:1229 - Resource path: 
Leaked instance: Reference:1698
Leaked instance: Reference:2213
Leaked instance: Reference:2003

... plenty more ...

Leaked instance: Reference:1997
Leaked instance: Reference:1490
Leaked instance: Reference:1316
Hint: Leaked instances typically happen when nodes are removed from the scene tree (with `remove_child()`) but not freed (with `free()` or `queue_free()`).
Orphan StringName: dconnect
Orphan StringName: Reference
Orphan StringName: other
Orphan StringName: GDScriptNativeClass
Orphan StringName: append
Orphan StringName: res://Node2D.gd
Orphan StringName: stuff
Orphan StringName: _init
Orphan StringName: GDScript
StringName: 9 unclaimed string names at exit.

[nathanfranke]

Okay, I see the leaked references with --verbose, but not with verbose_stdout, which doesn't seem to be implemented on EditorRun (Another issue?).

[vnen]

I don't think this is solved in master. If you create a circular reference it will leak, since there's still no mechanism to check for this.

[YuriSizov]

Would be nice to test this again, after @adamscott's improvements 👀

[adamscott]

@YuriSizov Unfortunately, I just tested and it still leaks. I'm investigating this.