fix: Fixed a few memory leak cases when shutting down NetworkManager during Incoming Message Queue processing #1323
Conversation
…during Incoming Message Queue processing
| @@ -113,6 +114,16 @@ public void Dispose() | |||
| { | |||
| CleanupDisconnectedClient(kvp.Key); | |||
| } | |||
|
|
|||
| // If there's a message currently being processed, it'll dispose itself, so we can't dispose here or that'll | |||
There was a problem hiding this comment.
Given we're single-threaded, is it possible we could be in the middle of processing a message and get here too?
There was a problem hiding this comment.
So just to put this here even though we discussed it on slack... It's not an issue of concurrency, it's an issue of call stack.
Three messages come in: FooMessage, ClientRpc, BarMessage
We process FooMessage and FooMessage is Dispose()d
We start to process ClientRpc
ClientRpc message calls NetworkManager.Shutdown(), which calls MessagingSystem.Dispose()
After returning from ClientRpc, the ClientRpc message is Dispose()d.
Since MessagingSystem has now been Dispose()d as well, we can no longer process BarMessage, so we exit the loop.
Thus:
- We can't Dispose() FooMessage because it was already Dispose()d
- We can't Dispose() the current message being processed (ClientRpc) because HandleMessage is above us in the call stack and will Dispose() it before it returns
- We must dispose BarMessage because HandleMessage will not get to that one as we're Dispose()ing the list that contains it.
That's what this comment means - it's not a threading concurrency, it's just that Shutdown() can be called from within HandleMessage(), and if that happens, the message currently being processed can't be Dispose()d.
|
So I have a question and this just for my understanding... Is there any reason why we just have some sort of flag that says currently we are processing messages and if you call dispose while we are processing these messages or Shutdown on |
|
|
…handling incoming messages. -Added tests for both ways to shut down to make sure they both work correctly -Fixed an issue in the snapshot system that was causing the new tests to fail
| var networkObject = m_NetworkManager.SpawnManager.CreateLocalNetworkObject(false, | ||
| spawnCommand.GlobalObjectIdHash, spawnCommand.OwnerClientId, (spawnCommand.ParentNetworkId == spawnCommand.NetworkObjectId) ? spawnCommand.NetworkObjectId : spawnCommand.ParentNetworkId, spawnCommand.ObjectPosition, | ||
| spawnCommand.ObjectRotation); | ||
| NetworkObject networkObject; |
There was a problem hiding this comment.
This fix is technically not a part of this change, but the added tests won't pass without this change because it causes warnings to get logged.
…during Incoming Message Queue processing (#1323) * fix: Fixed memory leak on shutdown * fix: Fixed a few memory leak cases when shutting down NetworkManager during Incoming Message Queue processing * standards check * Fixed test not cleaning up quite correctly. * -Reworked the way shutdown is handled so it can't be processed while handling incoming messages. -Added tests for both ways to shut down to make sure they both work correctly -Fixed an issue in the snapshot system that was causing the new tests to fail * standards * Fixed broken test, slight naming change to be more accurate.
…during Incoming Message Queue processing (#1323) (#1466) * fix: Fixed memory leak on shutdown * fix: Fixed a few memory leak cases when shutting down NetworkManager during Incoming Message Queue processing * standards check * Fixed test not cleaning up quite correctly. * -Reworked the way shutdown is handled so it can't be processed while handling incoming messages. -Added tests for both ways to shut down to make sure they both work correctly -Fixed an issue in the snapshot system that was causing the new tests to fail * standards * Fixed broken test, slight naming change to be more accurate. Co-authored-by: Jaedyn Draper <284434+ShadauxCat@users.noreply.github.com>
|
I will be updating the changelog. |
…during Incoming Message Queue processing (Unity-Technologies#1323) * fix: Fixed memory leak on shutdown * fix: Fixed a few memory leak cases when shutting down NetworkManager during Incoming Message Queue processing * standards check * Fixed test not cleaning up quite correctly. * -Reworked the way shutdown is handled so it can't be processed while handling incoming messages. -Added tests for both ways to shut down to make sure they both work correctly -Fixed an issue in the snapshot system that was causing the new tests to fail * standards * Fixed broken test, slight naming change to be more accurate.
…during Incoming Message Queue processing (Unity-Technologies#1323) (Unity-Technologies#1466) * fix: Fixed memory leak on shutdown * fix: Fixed a few memory leak cases when shutting down NetworkManager during Incoming Message Queue processing * standards check * Fixed test not cleaning up quite correctly. * -Reworked the way shutdown is handled so it can't be processed while handling incoming messages. -Added tests for both ways to shut down to make sure they both work correctly -Fixed an issue in the snapshot system that was causing the new tests to fail * standards * Fixed broken test, slight naming change to be more accurate. Co-authored-by: Jaedyn Draper <284434+ShadauxCat@users.noreply.github.com>
Fixed a few memory leak cases when shutting down NetworkManager during Incoming Message Queue processing.
MTT-1440
(I'm not sure if I should update changelog.md since it still says [Unreleased] and I'm not sure if I should put this in that section or make a new one for 1.0.1)
Changelog
com.unity.netcode.gameobjects
Testing and Documentation