adding try/catch to handler so server doesn't crash from user exceptions (#2435)

* adding try/catch to hanlder

user code can throw errors.

* Update Assets/Mirror/Runtime/MessagePacker.cs

* Update Assets/Mirror/Runtime/MessagePacker.cs

* test for command that throws

* disconnecting client after Exception in message handler

Co-authored-by: vis2k <info@noobtuts.com>

Assets/Mirror/Runtime/MessagePacker.cs

@@ -108,7 +108,17 @@ internal static NetworkMessageDelegate WrapHandler<T, C>(Action<C, T> handler, b
                 NetworkDiagnostics.OnReceive(message, channelId, reader.Length);
             }
 
+            // user handler exception should not stop the whole server
+            try
+            {
+                // user implemented handler
                 handler((C)conn, message);
+            }
+            catch (Exception e)
+            {
+                logger.LogError($"Exception in MessageHandler: {e.GetType().Name} {e.Message} {e.StackTrace}");
+                conn.Disconnect();
+            }
         };
     }
 }

Assets/Mirror/Tests/Editor/CommandTest.cs

@@ -1,4 +1,5 @@
 using System;
+using System.Text.RegularExpressions;
 using NUnit.Framework;
 using UnityEngine;
 using UnityEngine.TestTools;
@@ -49,6 +50,17 @@ public void CmdSendInt(int someInt, NetworkConnectionToClient conn = null)
         }
     }
 
+    class ThrowBehaviour : NetworkBehaviour
+    {
+        public const string ErrorMessage = "Bad things happened";
+
+        [Command]
+        public void SendThrow(int someInt)
+        {
+            throw new Exception(ErrorMessage);
+        }
+    }
+
     public class CommandTest : RemoteTestBase
     {
         [Test]
@@ -166,5 +178,22 @@ public void SenderConnectionIsSetWhenCommandIsRecievedWithIgnoreAuthority()
             ProcessMessages();
             Assert.That(callCount, Is.EqualTo(1));
         }
+
+        [Test]
+        public void CommandThatThrowsShouldBeCaught()
+        {
+            ThrowBehaviour hostBehaviour = CreateHostObject<ThrowBehaviour>(true);
+
+            const int someInt = 20;
+            NetworkConnectionToClient connectionToClient = NetworkServer.connections[0];
+            Debug.Assert(connectionToClient != null, $"connectionToClient was null, This means that the test is broken and will give the wrong results");
+
+            LogAssert.Expect(LogType.Error, new Regex($".*{ThrowBehaviour.ErrorMessage}.*"));
+            Assert.DoesNotThrow(() =>
+            {
+                hostBehaviour.SendThrow(someInt);
+                ProcessMessages();
+            }, "Processing new message should not throw, the execption from SendThrow should be caught");
+        }
     }
 }