`LogStream` memory leak issue (orphan Nodes)

[gergely-g]

Hi, first of all, thanks for the addon, I use it a lot. :) I’ve been working on a strategy game with heavy non-UI, data-only logic where I instantiate custom LogStreams from my classes like this:

class_name Pathfinding
extends RefCounted

var _log := LogStream.new("Pathfinding", LogStream.LogLevel.DEBUG)

This is very similar to what I'm used to from Java/Python which I appreciate a lot. :)

However I noticed that my GUT unit tests complain about orphan nodes:

==============================================
= 43 Orphans
==============================================
res://test/unit/test_pathfinding.gd
    - Outside Tests
        * ...
test_pathfinding.gd
    - test_path_on_plains_is_cheapest
        * [LogStream[Pathfinding](log-stream.gd)]
    - test_path_avoids_costly_terrain_like_forest
        * [LogStream[Pathfinding](log-stream.gd)]

GUT explanation for orphans link:

• Any Node (or a subclass of Node) that is not currently in the tree is considered an orphan. Children of orphaned Nodes are also considered orphans.

Technical background

This is what I learned after looking into this (I'm still new to Godot).

Godot’s memory model (docs here):

• Node and other Object-derived classes are not reference-counted.
• They remain in memory until explicitly freed (free() / queue_free()).
• Only RefCounted instances are freed automatically when the last reference is dropped.

Because the current version add-on encourages creating streams like this:

var network_log = LogStream.new("Networking")
network_log.warn("No internet")

the result is a permanently orphaned Node (not attached to the scene tree, never freed).

This means every such stream instance will stay in memory until the engine shuts down, or the developer manually frees it -- which is not mentioned in the documentation.

This can be especially bad if it's on an object that has many instances, like an enemy unit in the game for example.

My suggestion: consider switching LogStream to extend RefCounted

From what I understand, changing LogStream to extend RefCounted instead of Node would eliminate the orphan problem.

This would make each stream self-managed and automatically freed once there are no references.

Draft steps

1. Remove _ready() logic RefCounted objects don’t receive _ready(). The current _ready() in log-stream.gd connects a tree_exiting cleanup signal, but this is already handled by plugin.gd when the plugin is enabled:

   # plugin.gd
   tree_exiting.connect(_LogInternalPrinter._cleanup)
   

   So this code can safely be removed or moved to the plugin initialization.

2. Verify signal/callback behavior All Node signal connections (e.g. tree_exiting) that assume a scene-tree context should be removed or replaced with manual disconnection when appropriate.

Risks?

I might be missing some lifecycle stuff and RefCounted can still lead to memory leaks if there are circular references.

So we should be careful.

If LogStream keeps deriving from Node

If you prefer to keep LogStream as a Node, the docs should at least recommend a cleanup pattern. Something like this maybe (?):

var log = LogStream.new("MyLogger")
get_tree().root.add_child(log)
# or explicitly free when done
log.queue_free()

and note that otherwise, it will stay in memory.

I'd be happy to help with the implementation, please let me know what you think.

[albinaask]

I wholesomely agree with you that this is a quite ugly solution.

The reason why Log is derived from Node is historical. Back when Log was originally designed (for godot 3.4 or so) and 1.0 was released, there were no static methods why calls to the main stream, aka Log.dbg() etc had to be implemented as a singleton node. If LogStream should derive from Node or RefCounted was one of the main design decisions: A Node derived LogStream would save me a wrapper class and keep full backward compatibility, however an uglier solution and potentially leaking memory in case of the streams no sharing lifespan with the application (if a log stream is used under the entire application lifespan and then leaked just before the application is ended, no memory functionally lost). My decision was therefore to keep it Node based.

Nowadays, I think it makes little sense to keep it derived from Node, why I cautiously support a conversion to a RefCounted approach. This however comes with an issue of backward compatibility, which is and has been my number one priority of Log. The issue becomes how to keep as much functionality as humanly possible while making this change. I see a few options:

1. Don't keep it. Just remove the Log singleton and implement static versions of the methods in LogStream, the new static signatures would look like "LogStream.s_debug(...)". This would also force people who have added LogStreams to their trees rewrite all their code. I don't like this option for two reasons: Firstly this wouldn't keep backwards compatibility, secondly each level would need 4 methods for each log level (8 for shorthand methods) since there is already a version of the methods with variadic arguments (see #21). We could make an LTS version of Log 2.x and start drafting a Log 3.0... Bumping the major version, we could also revise the approach to variadic arguments since I in hindsight don't like the dual method approach.
2. Don't bother changing anything. This has been my strategy so far, and while comfy it comes with the drawbacks you mention and is a rather outdated and frankly rather cumbersome solution that has honestly created a lot of issues over the years. The poor option here would be to just collect a reference to all streams and clean them up on application quit... This would make the warning go away, but not the problem...
3. Make a wrapper class named Log to keep this functionality of the backwards compatibility while taking advantage of the newer approach to static methods. This wouldn't keep the functionality of adding streams to the tree however. This alos comes with the design challenge of retaining the signatures Log.debug and the like to make old scripts fit into the new API. The question then becomes how much of the old functionality to retain considering people may actually have added LogStreams to their scenes. While I was about to say that I don't see why anyone would do this, the test suite of Log is done this way, so see who's talking...

I'd be eager to hear your thoughts on these issues, I believe we really ought to think this through since it might imply a rather big, but perhaps good change in the API. Making a plugin for just you and me, option 1 would be the obvious winner, but backwards compatibility really throws a spanner in the works here.