godot: USER ERROR: Disconnecting nonexistent signal 'changed', callable: MeshInstance3D::_mesh_changed.


Bugsquad note: This issue has been confirmed several times already. No need to confirm it further.


Godot version

4.0.alpha6.official

System information

Garuda Linux, Vulkan, Nvidia GeForce GTX 970

Issue description

queue_freeing() a MeshInstance3D sometimes causes Godot to spam my logs with megabytes of:

USER ERROR: Disconnecting nonexistent signal 'changed', callable: MeshInstance3D::_mesh_changed.

I would guess #53285 is a similar issue

Steps to reproduce

something to do with queue_freeing a MeshInstance3D but it doesn’t always work

Minimal reproduction project

No response

About this issue

  • Original URL
  • State: open
  • Created 2 years ago
  • Reactions: 2
  • Comments: 27 (14 by maintainers)

Most upvoted comments

Thought I’d chime in here since this issue has been plaguing my project for a while (using Godot 3.4.5-stable) - I’ve never managed to find the exact cause of the issue, since it seems to happen completely randomly, both when closing the game and in the middle of running it (I assume when an object is freed from memory). And par for the course with annoying bugs, whenever I’ve tried to catch it as it happens, it doesn’t happen 😠

I’ve had a deep-dive into the source code to try and figure out what’s going on, and I’ve made a detailed post on my project’s issue here: drwhut/tabletop-club#131. But the TL;DR is that I think it’s when an Object tries to disconnect signals in it’s destructor, which produces the error and stops the destructor, so the object isn’t freed from memory and it constantly tries to free the object again and again:

Object::~Object() {
	// ...

	//signals from nodes that connect to this node
	while (connections.size()) {
		Connection c = connections.front()->get();
		c.source->_disconnect(c.signal, c.target, c.method, true); // <-- ERROR?
	}

Absolutely!

This one, and for another case see my comment on #34752, haven’t run into any others but I don’t doubt there are because they’re hard to trigger as any race condition, any case where a resource being attached connects a signal (i.e. pretty much most).

It looks like this is the cause at least from this bug report, as I can’t see any other way this could happen except if a race condition in signal connection happens (location where it might happen above)

For example:

  1. On thread, load resource X, it is loaded from cache
  2. On thread, attach it to some Node, this automatically connects some signal
  3. On main, through queue_free, delete some node using that same resource instance

If there’s a race condition between 2 and 3, when the last connection is removed from X by 3, then you can get a broken state

I believe at least two race conditions can occur, in both the resources will have connections, that are not matched in the signals:

  1. A and B connect at the same time, both creating a list of connections
  2. A is removed and B is added, causing the list to be cleared without B being connected

The new thread safety functions on Node could be relevant here, the call_thread_safe like my similar suggestion on said comment, to connect in a safe way, either immediately if on the main thread, or deferred otherwise

Now I recognise that using thread safety like locking can be undesirable here, and I agree, but it’s a trade-off, on one hand the performance issue of using locking for connecting and disconnecting, but on the other the uncertainty of deferring connecting, and both have pros and cons

My suggestion would be to apply similar safety to Resource as to Node, and tbh I think that is a more relevant area as it can be extremely difficult to gauge if a resource is thread safe at any moment, die to potentially unpredictable sharing and caching, one option would be to base the behaviour on wether the resource is shared or not.

And as an argument here I’d forward that despite being highly experienced with programming in general, and decently with threading in particular, and I didn’t expect attaching a resource to a node to be “modifying” the resource in the sense of breaking thread safety before I investigated this bug, nor does the thread safety instructions imply or suggest this, this is arguably made worse by that in c++ the argument passing a resource like set_texture is a const Ref<X> &, which can easily create the illusion that it’s unmodified

I believe that thread safety is largely the responsibility of the user, not the engine, but as per my comment that requires that the way to achieve it is a) clear and b) under user control

I’m sorry this got a bit wordy, and edity, will write a cleaner comment on some of the same points on #34752 as well as it’s more specific to this issue (regardless of the cause of this bug, despite being pretty certain of the cause)

@krizej Can you (or anyone else) still reproduce this bug in 4.0.stable or any later release?

If yes, please ensure that an up-to-date Minimal Reproduction Project (MRP) is included in this report (a MRP is a zipped Godot project with the minimal elements necessary to reliably trigger the bug). You can upload ZIP files in an issue comment with a drag and drop.

I can reproduce it in my main project on v4.0.stable.mono.official [92bee43ad] I’m trying to put together an MRP but it is hard since i don’t exactly know what behavior that triggers it on the mesh

[…] but signals might, which will help with cases that might not be obvious as thread unsafe, like attaching a resource to a node or deleting the object holding it.

@AThousandShips, can you elaborate on this? What concrete signals or functions are involved in a thread-unsafe manner?

I think I missed that. Thanks for the explanation. I guess I’ll have to use some kind of timer in my _Process() function then.

Thank you for taking the time to look through my code! Now that you mention it, I hadn’t thought about packed scenes having to also load sub-resources, I think you’ve hit the nail on the head there. I was planning to do a big re-write of my game soon, so that’ll be a good opportunity to re-think the threading implementation.

So as far as I can tell, until resources become thread-safe on the engine level, if one wants to load resources in another thread, the resources also have to be freed on that same thread? And we need to make sure we’re not accidentally loading those resources on the main thread as a sub-resource to something else…

That still leaves one question, though: from what I remember, I had trouble in the past using just free() on nodes rather than queue_free(). So, would it even be possible to reliably free nodes with resources in another thread if we need to use queue_free() (which I’m assuming always activates on the main thread)?

If it is indeed signal threading related #74404 should fix it.

Ok so it seems like the problem is here https://github.com/godotengine/godot/blob/master/scene/3d/mesh_instance_3d.cpp between row 108 to 133

I still can’t trigger it in my MRP project though