fix: NetworkManager.ShutdownInProgress not being reset #2661
Conversation
| @@ -1013,6 +1013,9 @@ internal void ShutdownInternal() | |||
| OnServerStopped?.Invoke(ConnectionManager.LocalClient.IsClient); | |||
| } | |||
|
|
|||
| // In the event shutdown is invoked within OnClientStopped or OnServerStopped, set it to false again | |||
| m_ShuttingDown = false; | |||
There was a problem hiding this comment.
Why not just move the existing place where it's set to false, instead of setting it twice? Is there a reason it should always be false in OnServerStopped and OnClientStopped?
There was a problem hiding this comment.
If someone is checking ShutdownInProgress it would return true if it was not set to false...
Yeah... it is a weird thing... so... OnServerStopped and OnClientStopped could invoke other (generic) methods that depend upon that value being false (i.e. validating that the shutdown has completed...which it really hasn't 100% completed...but we are invoking those callbacks at that point in time... so... one of those scenarios... keep it true and someone will complain that it still thinks it is shutting down when it is notifying that it stopped...set it to false...and someone might invoke shutdown again... for--->insert various reasons here<---... 😹 )
The compromise was to just make sure it was no longer shutting down after potentially invoking user code.
There was a problem hiding this comment.
That feels like it could use some conceptual rework... but not for 1.6. We can go with this for now...
There was a problem hiding this comment.
Yep... 💯%
| @@ -10,8 +10,11 @@ Additional documentation and release notes are available at [Multiplayer Documen | |||
|
|
|||
| ### Added | |||
|
|
|||
| - Added a protected virtual method `NetworkTransform.OnInitialize(ref NetworkTransformState replicatedState)` that just returns the replicated state reference. | |||
There was a problem hiding this comment.
This seems unrelated to the PR, is it supposed to be here?
There was a problem hiding this comment.
Well, it makes it easier if we merge this into the changelog now since that adjustment was made to #2657
There was a problem hiding this comment.
I could take it out... it should just collapse when merged.
There was a problem hiding this comment.
Don't think we need it in two PRs... but either way, can you add the PR number for this changelog entry?
There was a problem hiding this comment.
Eh.. you are right... no point in having it in two places and it might be confusing being in this PR... your initial gut reaction is probably the right way...
"This is the way..."
Removing the adjusted changelog entry
SamuelBellomo
deleted the
fix/networkmanager-shuttingdown-not-being-reset
branch
This resolves the issue where invoking
NetworkManager.ShutdownwithinNetworkManager.OnClientStoppedorNetworkManager.OnServerStoppedwould forceNetworkManager.ShutdownInProgressto remain true after completing the shutdown process.Changelog
NetworkManager.ShutdownwithinNetworkManager.OnClientStoppedorNetworkManager.OnServerStoppedwould forceNetworkManager.ShutdownInProgressto remain true after completing the shutdown process.Testing and Documentation
NetworkManagerTests.ValidateShutdown