fix: kcp2k V1.27. fixes #3337

Assets/Mirror/Transports/KCP/kcp2k/VERSION.txt

@@ -1,3 +1,10 @@
+V1.27 [2023-01-08]
+- KcpClient.Connect: invoke own events directly instead of going through peer,
+  which calls our own events anyway
+- fix: KcpPeer/Client/Server callbacks are readonly and assigned in constructor
+  to ensure they are safe to use at all times.
+  fixes https://github.com/MirrorNetworking/Mirror/issues/3337
+
 V1.26 [2022-12-22]
 - KcpPeer.RawInput: fix compile error in old Unity Mono versions
 - fix: KcpServer sets up a new connection's OnError immediately.

Assets/Mirror/Transports/KCP/kcp2k/highlevel/KcpClient.cs

@@ -28,10 +28,14 @@ public class KcpClient
         // even for errors, to allow liraries to show popups etc.
         // instead of logging directly.
         // (string instead of Exception for ease of use and to avoid user panic)
-        public Action OnConnected;
+        //
-        public Action<ArraySegment<byte>, KcpChannel> OnData;
+        // events are readonly, set in constructor.
-        public Action OnDisconnected;
+        // this ensures they are always initialized when used.
-        public Action<ErrorCode, string> OnError;
+        // fixes https://github.com/MirrorNetworking/Mirror/issues/3337 and more
+        readonly Action OnConnected;
+        readonly Action<ArraySegment<byte>, KcpChannel> OnData;
+        readonly Action OnDisconnected;
+        readonly Action<ErrorCode, string> OnError;
 
         // state
         public bool connected;
@@ -41,6 +45,7 @@ public KcpClient(Action OnConnected,
                          Action OnDisconnected,
                          Action<ErrorCode, string> OnError)
         {
+            // initialize callbacks first to ensure they can be used safely.
             this.OnConnected = OnConnected;
             this.OnData = OnData;
             this.OnDisconnected = OnDisconnected;
@@ -56,21 +61,16 @@ public void Connect(string address, ushort port, KcpConfig config)
             }
 
             // create fresh peer for each new session
-            peer = new KcpPeer(RawSend, config);
+            peer = new KcpPeer(RawSend, OnAuthenticatedWrap, OnData, OnDisconnectedWrap, OnError, config);
 
-            // setup events
+            // some callbacks need to wrapped with some extra logic
-            peer.OnAuthenticated = () =>
+            void OnAuthenticatedWrap()
             {
                 Log.Info($"KcpClient: OnConnected");
                 connected = true;
                 OnConnected();
-            };
+            }
-            peer.OnData = (message, channel) =>
+            void OnDisconnectedWrap()
-            {
-                //Log.Debug($"KcpClient: OnClientData({BitConverter.ToString(message.Array, message.Offset, message.Count)})");
-                OnData(message, channel);
-            };
-            peer.OnDisconnected = () =>
             {
                 Log.Info($"KcpClient: OnDisconnected");
                 connected = false;
@@ -79,11 +79,7 @@ public void Connect(string address, ushort port, KcpConfig config)
                 socket = null;
                 remoteEndPoint = null;
                 OnDisconnected();
-            };
+            }
-            peer.OnError = (error, reason) =>
-            {
-                OnError(error, reason);
-            };
 
             Log.Info($"KcpClient: connect to {address}:{port}");
 
@@ -91,8 +87,8 @@ public void Connect(string address, ushort port, KcpConfig config)
             if (!Common.ResolveHostname(address, out IPAddress[] addresses))
             {
                 // pass error to user callback. no need to log it manually.
-                peer.OnError(ErrorCode.DnsResolve, $"Failed to resolve host: {address}");
+                OnError(ErrorCode.DnsResolve, $"Failed to resolve host: {address}");
-                peer.OnDisconnected();
+                OnDisconnected();
                 return;
             }
 

Assets/Mirror/Transports/KCP/kcp2k/highlevel/KcpPeer.cs

@@ -22,13 +22,16 @@ public class KcpPeer
         // leftover from KcpConnection. remove it after refactoring later.
         KcpState state = KcpState.Connected;
 
-        public Action OnAuthenticated;
+        // events are readonly, set in constructor.
-        public Action<ArraySegment<byte>, KcpChannel> OnData;
+        // this ensures they are always initialized when used.
-        public Action OnDisconnected;
+        // fixes https://github.com/MirrorNetworking/Mirror/issues/3337 and more
+        readonly Action OnAuthenticated;
+        readonly Action<ArraySegment<byte>, KcpChannel> OnData;
+        readonly Action OnDisconnected;
         // error callback instead of logging.
         // allows libraries to show popups etc.
         // (string instead of Exception for ease of use and to avoid user panic)
-        public Action<ErrorCode, string> OnError;
+        readonly Action<ErrorCode, string> OnError;
 
         // If we don't receive anything these many milliseconds
         // then consider us disconnected
@@ -139,8 +142,19 @@ public static int ReliableMaxMessageSize(uint rcv_wnd) =>
         // => useful to start from a fresh state every time the client connects
         // => NoDelay, interval, wnd size are the most important configurations.
         //    let's force require the parameters so we don't forget it anywhere.
-        public KcpPeer(Action<ArraySegment<byte>> output, KcpConfig config)
+        public KcpPeer(
+            Action<ArraySegment<byte>> output,
+            Action OnAuthenticated,
+            Action<ArraySegment<byte>, KcpChannel> OnData,
+            Action OnDisconnected,
+            Action<ErrorCode, string> OnError,
+            KcpConfig config)
         {
+            // initialize callbacks first to ensure they can be used safely.
+            this.OnAuthenticated = OnAuthenticated;
+            this.OnData = OnData;
+            this.OnDisconnected = OnDisconnected;
+            this.OnError = OnError;
             this.RawSend = output;
 
             // set up kcp over reliable channel (that's what kcp is for)

Assets/Mirror/Transports/KCP/kcp2k/highlevel/KcpServer.cs

@@ -14,10 +14,14 @@ public class KcpServer
         // even for errors, to allow liraries to show popups etc.
         // instead of logging directly.
         // (string instead of Exception for ease of use and to avoid user panic)
-        public Action<int> OnConnected;
+        //
-        public Action<int, ArraySegment<byte>, KcpChannel> OnData;
+        // events are readonly, set in constructor.
-        public Action<int> OnDisconnected;
+        // this ensures they are always initialized when used.
-        public Action<int, ErrorCode, string> OnError;
+        // fixes https://github.com/MirrorNetworking/Mirror/issues/3337 and more
+        readonly Action<int> OnConnected;
+        readonly Action<int, ArraySegment<byte>, KcpChannel> OnData;
+        readonly Action<int> OnDisconnected;
+        readonly Action<int, ErrorCode, string> OnError;
 
         // configuration
         readonly KcpConfig config;
@@ -42,6 +46,7 @@ public KcpServer(Action<int> OnConnected,
                          Action<int, ErrorCode, string> OnError,
                          KcpConfig config)
         {
+            // initialize callbacks first to ensure they can be used safely.
             this.OnConnected = OnConnected;
             this.OnData = OnData;
             this.OnDisconnected = OnDisconnected;
@@ -194,13 +199,71 @@ protected virtual void RawSend(int connectionId, ArraySegment<byte> data)
 
         protected virtual KcpServerConnection CreateConnection(int connectionId)
         {
-            // attach connectionId to RawSend.
+            // events need to be wrapped with connectionIds
-            // kcp needs a simple RawSend(byte[]) function.
             Action<ArraySegment<byte>> RawSendWrap =
                 data => RawSend(connectionId, data);
 
-            KcpPeer peer = new KcpPeer(RawSendWrap, config);
+            // create empty connection without peer first.
-            return new KcpServerConnection(peer, newClientEP);
+            // we need it to set up peer callbacks.
+            // afterwards we assign the peer.
+            KcpServerConnection connection = new KcpServerConnection(newClientEP);
+
+            // set up peer with callbacks
+            KcpPeer peer = new KcpPeer(RawSendWrap, OnAuthenticatedWrap, OnDataWrap, OnDisconnectedWrap, OnErrorWrap, config);
+
+            // assign peer to connection
+            connection.peer = peer;
+            return connection;
+
+            // setup authenticated event that also adds to connections
+            void OnAuthenticatedWrap()
+            {
+                // only send handshake to client AFTER we received his
+                // handshake in OnAuthenticated.
+                // we don't want to reply to random internet messages
+                // with handshakes each time.
+                connection.peer.SendHandshake();
+
+                // add to connections dict after being authenticated.
+                connections.Add(connectionId, connection);
+                Log.Info($"KcpServer: added connection({connectionId})");
+
+                // setup Data + Disconnected events only AFTER the
+                // handshake. we don't want to fire OnServerDisconnected
+                // every time we receive invalid random data from the
+                // internet.
+
+                // setup data event
+
+
+                // finally, call mirror OnConnected event
+                Log.Info($"KcpServer: OnConnected({connectionId})");
+                OnConnected(connectionId);
+            }
+
+            void OnDataWrap(ArraySegment<byte> message, KcpChannel channel)
+            {
+                // call mirror event
+                //Log.Info($"KCP: OnServerDataReceived({connectionId}, {BitConverter.ToString(message.Array, message.Offset, message.Count)})");
+                OnData(connectionId, message, channel);
+            }
+
+            void OnDisconnectedWrap()
+            {
+                // flag for removal
+                // (can't remove directly because connection is updated
+                //  and event is called while iterating all connections)
+                connectionsToRemove.Add(connectionId);
+
+                // call mirror event
+                Log.Info($"KcpServer: OnDisconnected({connectionId})");
+                OnDisconnected(connectionId);
+            }
+
+            void OnErrorWrap(ErrorCode error, string reason)
+            {
+                OnError(connectionId, error, reason);
+            }
         }
 
         // receive + add + process once.
@@ -234,56 +297,6 @@ void ProcessMessage(ArraySegment<byte> segment, int connectionId)
                 //
                 // for now, this is fine.
 
-                // setup error event first.
-                // initialization may already log errors.
-                connection.peer.OnError = (error, reason) =>
-                {
-                    OnError(connectionId, error, reason);
-                };
-
-                // setup authenticated event that also adds to connections
-                connection.peer.OnAuthenticated = () =>
-                {
-                    // only send handshake to client AFTER we received his
-                    // handshake in OnAuthenticated.
-                    // we don't want to reply to random internet messages
-                    // with handshakes each time.
-                    connection.peer.SendHandshake();
-
-                    // add to connections dict after being authenticated.
-                    connections.Add(connectionId, connection);
-                    Log.Info($"KcpServer: added connection({connectionId})");
-
-                    // setup Data + Disconnected events only AFTER the
-                    // handshake. we don't want to fire OnServerDisconnected
-                    // every time we receive invalid random data from the
-                    // internet.
-
-                    // setup data event
-                    connection.peer.OnData = (message, channel) =>
-                    {
-                        // call mirror event
-                        //Log.Info($"KCP: OnServerDataReceived({connectionId}, {BitConverter.ToString(message.Array, message.Offset, message.Count)})");
-                        OnData.Invoke(connectionId, message, channel);
-                    };
-
-                    // setup disconnected event
-                    connection.peer.OnDisconnected = () =>
-                    {
-                        // flag for removal
-                        // (can't remove directly because connection is updated
-                        //  and event is called while iterating all connections)
-                        connectionsToRemove.Add(connectionId);
-
-                        // call mirror event
-                        Log.Info($"KcpServer: OnDisconnected({connectionId})");
-                        OnDisconnected(connectionId);
-                    };
-
-                    // finally, call mirror OnConnected event
-                    Log.Info($"KcpServer: OnConnected({connectionId})");
-                    OnConnected(connectionId);
-                };
 
                 // now input the message & process received ones
                 // connected event was set up.

Assets/Mirror/Transports/KCP/kcp2k/highlevel/KcpServerConnection.cs

@@ -7,12 +7,15 @@ namespace kcp2k
     // struct to avoid memory indirection
     public struct KcpServerConnection
     {
-        public readonly KcpPeer peer;
+        // peer can't be set from constructor at the moment.
+        // because peer callbacks need to know 'connection'.
+        // see KcpServer.CreateConnection.
+        public KcpPeer peer;
         public readonly EndPoint remoteEndPoint;
 
-        public KcpServerConnection(KcpPeer peer, EndPoint remoteEndPoint)
+        public KcpServerConnection(EndPoint remoteEndPoint)
         {
-            this.peer = peer;
+            peer = null;
             this.remoteEndPoint = remoteEndPoint;
         }
     }