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.

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

Signal trying to access freed listeners C# #74984

Open
malaVydra opened this issue Mar 16, 2023 · 9 comments
Open

Signal trying to access freed listeners C# #74984

malaVydra opened this issue Mar 16, 2023 · 9 comments

Comments

@malaVydra
Copy link

malaVydra commented Mar 16, 2023

Godot version

4.0.stable.mono

System information

macOS 13.2.1 - Apple M1 (2020)

Issue description

I have an auto-loaded scene (single node) with a script that contains a signal, and a public method that is used to emit the signal:
[Signal] public delegate void ExperienceCollectedEventHandler(float amount); public void EmitExperienceCollected(float amount){ EmitSignal(SignalName.ExperienceVialCollected, amount); }

When the main screen loads, the listeners are appended using the += operator (which the documentation suggests):
GameEvents gameEvents = GetNode<GameEvents>("/root/GameEvents"); gameEvents += OnExperienceCollected;

Everything works fine until I relaod the main scene.

After the original listeners of the mentioned signal are freed, every time the signal is emitted, it's trying to access the disposed objects and call methods on them.

The solution I found is, instead of using the C# syntax to add a listener (+=), to use a GodotObject.Connect(...). This way, after the nodes are freed, the signal isn't trying to reach them anymore.

Steps to reproduce

  • Create an auto-load scene containing only one Node and a script with a signal.
  • Create a main scene which should be the actual game scene.
  • Add listeners to the signal from auto-loaded scene using the C# syntax (autoloaded.signal += listenerMethod).
  • Emit the signal from auto-loaded scene.
  • Reload the main scene using ReloadCurrentScene() or ChangeSceneToFile(...)
  • Emit the signal again.

Minimal reproduction project

If the project is needed let me know.

@malaVydra malaVydra changed the title Problem with appending listeners to a signal C# Signal trying to access freed listeners C# Mar 16, 2023
@essial
Copy link

essial commented Mar 16, 2023

I don't know about godot specifically, but in C#, += a listener to a delegate requires that you unsubscribe with -= if the producer will outlive the consumer. Can you not add a -= to the disposable of the instance?

I would put a print statement in your callback when you do the .connect() method to verify if it's hitting BOTH instances, I'm suspicious that that reference in connect() pins the instance and keeps it alive even though it's not being actively processed. If this is the case you'd get the print twice.

Do let me know as I currently am thinking this is expected due to how C# delegates work, and that your connect() version is actually leaking memory. I've seen many an application leak memory due to delegates pinning objects.

@malaVydra
Copy link
Author

That's a good point.
However, Godot handles unsubscribing when the node is freed.
Screenshot 2023-03-16 at 21 13 02

I also tried your suggestion just in case, and I only got one print.

@essial
Copy link

essial commented Mar 16, 2023

Interesting. Although I use C# professionally, I use GDScript in Godot. I'd be interested in how Godot even knows how to unsubscribe events like that given even standard C# apps don't support this, and will absolutely cause a memory leak.

@fahadTheTechIdea
Copy link

i faced the same problem, my solution was to is i manualy unsubscribed the event in tree exisiting event.
private void _on_OptionMenuControl_tree_exiting()
{
GameManager.NodeChangeRequest -= NodeRequest;
}

@essial
Copy link

essial commented Mar 17, 2023

IMO the documentation should be changed to match what @fahadTheTechIdea said. Even if it did work properly in Godot, it's an anti-pattern and makes no sense in the .net world.

@malaVydra
Copy link
Author

When are you calling that method?
I tried that but was having a problem with the _Notification override trying to run on NotificationPredelete.
The problem is I don't free the Node containing the script manually but am freeing the top parent Main node, so I never have the reference to the node before freeing.
I mean I could definitely find a way around it, but it just seems way dirtier to me to do so than what the documentation suggests, as I might have other nodes connect to some GameEvent signals.
If the documentation is wrong, what costs me to just stick with my Connect(...) way, as it doesn't seem to have any leaks?

@fahadTheTechIdea
Copy link

i'm calling on the tree_exiting() event for the scene ( Scene is "OptionMenuControl")
example:
public override void _Ready()
{
GameManager.NodeChangeRequest += NodeRequest;

}
private void _on_OptionMenuControl_tree_exiting()
{
GameManager.NodeChangeRequest -= NodeRequest;
}

@malaVydra
Copy link
Author

Thank you. Now just to confirm, is there something wrong with doing it this way:

gameEvents.PlayerDamaged += OnPlayerDamaged;
this.TreeExiting += () => {
    gameEvents.PlayerDamaged -= OnPlayerDamaged;
};

Or with overriding of the _ExitTree() method to do the unsubscribing like so:

public override void _ExitTree()
{
    gameEvents.PlayerDamaged -= OnPlayerDamaged;
    base._ExitTree();
}

Still, the documentation about this is a bit confusing.

@RedworkDE
Copy link
Member

Duplicate of #70414

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants