fix: client throws a not server exception when destroying a NetworkObject while shutting down [MTT-6210] #2510
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Making the NetworkLog message generate when the log level is error or lower and updating comments.
Adjusted the way we detect the test to use a more conditional wait approach for slower systems. Updated TrackServerLogSentMetric to detect the message sent. Added HasLogBeenReceived method to NetcodeLogAssert in order to be able to just wait for the console log (for slower systems).
0xFA11
reviewed
Apr 14, 2023
| public bool HasLogBeenReceived(LogType type, string message) | ||
| { | ||
| var found = false; | ||
| lock (m_Lock) |
There was a problem hiding this comment.
I'm a bit scared to see lock in the codebase, what's going on here?
0xFA11
reviewed
Apr 14, 2023
| @@ -51,35 +51,58 @@ public static class NetworkLog | |||
| /// <param name="message">The message to log</param> | |||
| public static void LogErrorServer(string message) => LogServer(message, LogType.Error); | |||
|
|
|||
| internal static NetworkManager NetworkManagerOverride; | |||
There was a problem hiding this comment.
is this strictly for tests?
There was a problem hiding this comment.
It is indeed. Since it was always using the singleton, up until this we have never fully tested server network logs generated clients.
0xFA11
reviewed
Apr 14, 2023
Comment on lines
-62
to
+92
| LogInfoServerLocal(message, localId); | ||
| if (isServer) | ||
| { | ||
| LogInfoServerLocal(message, localId); | ||
| } | ||
| else | ||
| { | ||
| LogInfo(message); | ||
| } | ||
| break; | ||
| case LogType.Warning: | ||
| LogWarningServerLocal(message, localId); | ||
| if (isServer) | ||
| { | ||
| LogWarningServerLocal(message, localId); | ||
| } | ||
| else | ||
| { | ||
| LogWarning(message); | ||
| } | ||
| break; | ||
| case LogType.Error: | ||
| LogErrorServerLocal(message, localId); | ||
| if (isServer) | ||
| { | ||
| LogErrorServerLocal(message, localId); | ||
| } | ||
| else | ||
| { | ||
| LogError(message); | ||
| } |
There was a problem hiding this comment.
I'm not sure if these changes are necessary
0xFA11
reviewed
Apr 14, 2023
| (IsSceneObject == null || (IsSceneObject.Value != true))) | ||
| { | ||
| throw new NotServerException($"Destroy a spawned {nameof(NetworkObject)} on a non-host client is not valid. Call {nameof(Destroy)} or {nameof(Despawn)} on the server/host instead."); |
There was a problem hiding this comment.
could this just simply be replaced with an error log without changing things around it too much?
if we're only trying to error-and-continue instead of exception-throw to keep the shutdown sequence running, let's "just" do that and nothing else as the minimal fix?
0xFA11
suggested changes
Apr 14, 2023
0xFA11
approved these changes
Apr 14, 2023
0xFA11
deleted the
fix/client-exception-not-server-destroying-networkobject
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes the edge case scenarios where a client could abruptly shutdown (host drops connection or the like) while NetworkObjects are still spawned. Under these conditions, the client-side would throw an exception if it was in the middle of the shutdown process which would disrupt the shutdown sequence.
This includes a minor modification to the NetworkLog server logging where a client generating any type of server log would get the standard [Netcode] message header/prefix and on the server-side it would generate the typical [Netcode-Server Sender=#] header/prefix. It additionally provides a means to test and validate server NetworkLog messages.
MTT-6210
Pertains to #2480
Changelog
NetworkObject(s).Testing and Documentation