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

Error when deleting a "MultiplayerSpawner" tracked node after a client has left. #92358

Closed
DanielSnd opened this issue May 25, 2024 · 2 comments · Fixed by #92359
Closed

Error when deleting a "MultiplayerSpawner" tracked node after a client has left. #92358

DanielSnd opened this issue May 25, 2024 · 2 comments · Fixed by #92359

Comments

@DanielSnd
Copy link
Contributor

Tested versions

  • Reproducible in v4.3.dev6.official [89850d5], 4.2.1 stable and master.

System information

Godot v4.3.dev6 - Windows 10.0.22631 - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 3060 Laptop GPU (NVIDIA; 31.0.15.3758) - AMD Ryzen 9 5900HS with Radeon Graphics (16 Threads)

Issue description

When a client leaves, if you delete a node that was spawned by a Multiplayer Spawner, the following error pops up:

E 0:00:08:0910   _remove_node_cache: Condition "!pinfo" is true. Continuing.
  <C++ Source>   modules/multiplayer/scene_cache_interface.cpp:60 @ _remove_node_cache()

This happens at this point in the code (Taken from current master):

void SceneCacheInterface::_remove_node_cache(ObjectID p_oid) {
	NodeCache *nc = nodes_cache.getptr(p_oid);
	if (!nc) {
		return;
	}
	if (nc->cache_id) {
		assigned_ids.erase(nc->cache_id);
	}
	for (KeyValue<int, int> &E : nc->recv_ids) {
		PeerInfo *pinfo = peers_info.getptr(E.key);
		ERR_CONTINUE(!pinfo);            ////////////   <<--- Error happens here.
		pinfo->recv_nodes.erase(E.value);
	}
	for (KeyValue<int, bool> &E : nc->confirmed_peers) {
		PeerInfo *pinfo = peers_info.getptr(E.key);
		ERR_CONTINUE(!pinfo);
		pinfo->sent_nodes.erase(p_oid);
	}
	nodes_cache.erase(p_oid);
}

Steps to reproduce

  • Run two instances of the minimum repro project.
  • In one instance click Host
  • In the other instance click Join
  • Close the instance that clicked Join.
  • See the error pop up on the Host.

Minimal reproduction project (MRP)

network_issue_minimum_repro.zip

@DanielSnd
Copy link
Contributor Author

It was a simple fix, did a pull request for it.

When a client disconnects on on_peer_change it looks like the intention of the code was to remove the peer ID from the list of received ids of that node with nc->recv_ids.erase(p_id); but what is actually in the code today is nc->recv_ids.erase(E.key); which in this case is trying to erase the remote cache id of the node from the list of received ids of the node, which doesn't really make sense since that is a list of peer ids that received that node.

The error happens because since the id wasn't being removed from the list of recv_ids when the node was eventually freed it tried to get the peer_info for that id and failed since the peer at this point no longer exists, showing the error.

@DanielSnd
Copy link
Contributor Author

Oops. Didn't mean to close with the previous comment.

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

Successfully merging a pull request may close this issue.

3 participants