Conversation
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).
| 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?
| /// <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.
| 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
| if (NetworkManager.IsListening && !NetworkManager.IsServer && IsSpawned && | ||
| (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
left a comment
There was a problem hiding this comment.
I think we could only convert throw new NotServerException into a Debug.LogError or NetworkLog.LogError as a minimal fix without touching anything else.
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