fix: #4046 [Command]s are now immediately invoked in host mode in order to fix edge cases & race conditions (#4048)

Author: miwarnec

Assets/Mirror/Editor/Weaver/Processors/CommandProcessor.cs

@@ -10,6 +10,35 @@ public static class CommandProcessor
             // generates code like:
             public void CmdThrust(float thrusting, int spin)
             {
+                // no host, invoke the original function immediately.
+                // -> Mirror's Host mode is just a server, we can't simulate an independent client in Unity
+                // -> delaying this for later introduces cooldown & prediction issues in games.
+                //    for example, assume CmdFireWeapon function with 100ms cooldown between shots.
+                //
+                //    client-only mode:
+                //      simply calling CmdFireWeapon and waiting for [SyncVar] cooldown would be too irregular (i.e. 150ms)
+                //      client has to predict the cooldown locally in order to call CmdFireWeapon every 100ms
+                //      this works fine, and that's how games need to predict weapon firing / skill usage / etc.
+                //
+                //    host mode:
+                //       Cmds usued to be queued up for network message processing to 'simulate' a client
+                //       this introduces a massive headache:
+                //          firing 3x would queue up CmdFireWeapon 3 times, without ever setting the cooldown yet
+                //          eventually messages are processed:
+                //            CmdFireWeapon first call goes through, sets cooldown
+                //            CmdFireWeapon second/third call would be rejected: "user attempted to fire on cooldown"
+                //          in other words, we would need to predict cooldowns on host too, which is super weird since host is the server
+                //
+                //     common sense: on host, calling a Cmd should happen immediately, anything else is too much magic
+                //                   and causes edge cases until Unity supports true server/client separation on host!
+                //
+                if (isServer && isClient) // isHost
+                {
+                    UserCode_CmdThrust(value);
+                    return;
+                }
+
+                // otherwise send a command message over the network
                 NetworkWriterPooled writer = NetworkWriterPool.Get();
                 writer.Write(thrusting);
                 writer.WritePackedUInt32((uint)spin);
@@ -38,6 +67,32 @@ public static MethodDefinition ProcessCommandCall(WeaverTypes weaverTypes, Write
 
             NetworkBehaviourProcessor.WriteSetupLocals(worker, weaverTypes);
 
+            Instruction skipIfNotHost = worker.Create(OpCodes.Nop);
+
+            // Check if isServer && isClient
+            // note that we don't use NetworkServer/Client.active here,
+            // otherwise [Command] tests which simulate server/client separation would fail.
+            worker.Emit(OpCodes.Ldarg_0); // loads this. for isServer check later
+            worker.Emit(OpCodes.Call, weaverTypes.NetworkBehaviourIsServerReference);
+            worker.Emit(OpCodes.Brfalse, skipIfNotHost);
+
+            worker.Emit(OpCodes.Ldarg_0); // loads this. for isClient check later
+            worker.Emit(OpCodes.Call, weaverTypes.NetworkBehaviourIsClientReference);
+            worker.Emit(OpCodes.Brfalse, skipIfNotHost);
+
+            // Load 'this' reference (Ldarg_0)
+            worker.Emit(OpCodes.Ldarg_0);
+
+            // Load all the remaining arguments (Ldarg_1, Ldarg_2, ...)
+            for (int i = 1; i < md.Parameters.Count + 1; i++)
+                worker.Emit(OpCodes.Ldarg, i);
+
+            // Call the original function directly (UserCode_CmdTest__Int32)
+            worker.Emit(OpCodes.Call, cmd);
+            worker.Emit(OpCodes.Ret);
+
+            worker.Append(skipIfNotHost);
+
             // NetworkWriter writer = new NetworkWriter();
             NetworkBehaviourProcessor.WriteGetWriter(worker, weaverTypes);
 

Assets/Mirror/Editor/Weaver/WeaverTypes.cs

@@ -10,6 +10,8 @@ public class WeaverTypes
     {
         public MethodReference ScriptableObjectCreateInstanceMethod;
 
+        public MethodReference NetworkBehaviourIsClientReference;
+        public MethodReference NetworkBehaviourIsServerReference;
         public FieldReference NetworkBehaviourDirtyBitsReference;
         public MethodReference GetWriterReference;
         public MethodReference ReturnWriterReference;
@@ -90,6 +92,9 @@ public WeaverTypes(AssemblyDefinition assembly, Logger Log, ref bool WeavingFail
 
             TypeReference NetworkBehaviourType = Import<NetworkBehaviour>();
 
+            NetworkBehaviourIsClientReference = Resolvers.ResolveMethod(NetworkBehaviourType, assembly, Log, "get_isClient", ref WeavingFailed);
+            NetworkBehaviourIsServerReference = Resolvers.ResolveMethod(NetworkBehaviourType, assembly, Log, "get_isServer", ref WeavingFailed);
+
             NetworkBehaviourDirtyBitsReference = Resolvers.ResolveField(NetworkBehaviourType, assembly, Log, "syncVarDirtyBits", ref WeavingFailed);
 
             generatedSyncVarSetter = Resolvers.ResolveMethod(NetworkBehaviourType, assembly, Log, "GeneratedSyncVarSetter", ref WeavingFailed);

Assets/Mirror/Tests/Editor/NetworkServer/NetworkServerTest.cs

@@ -800,20 +800,22 @@ public void SendCommand_OnlyAllowedOnOwnedObjects()
         {
             // listen & connect
             NetworkServer.Listen(1);
-            ConnectHostClientBlockingAuthenticatedAndReady();
+            ConnectClientBlockingAuthenticatedAndReady(out _);
 
             // add an identity with two networkbehaviour components
             // spawned, otherwise command handler won't find it in .spawned.
             // WITH OWNER = WITH AUTHORITY
-            CreateNetworkedAndSpawn(out GameObject _, out NetworkIdentity identity, out CommandTestNetworkBehaviour comp, NetworkServer.localConnection);
+            CreateNetworkedAndSpawn(out _, out NetworkIdentity serverIdentity, out CommandTestNetworkBehaviour serverComponent,
+                                    out _, out NetworkIdentity clientIdentity, out CommandTestNetworkBehaviour clientComponent,
+                                    NetworkServer.localConnection);
 
             // change identity's owner connection so we can't call [Commands] on it
-            identity.connectionToClient = new LocalConnectionToClient();
+            serverIdentity.connectionToClient = new LocalConnectionToClient();
 
             // call the command
-            comp.TestCommand();
+            clientComponent.TestCommand();
             ProcessMessages();
-            Assert.That(comp.called, Is.EqualTo(0));
+            Assert.That(serverComponent.called, Is.EqualTo(0));
         }
 
         [Test]
@@ -826,13 +828,13 @@ public void SendCommand_RequiresAuthority()
             // add an identity with two networkbehaviour components
             // spawned, otherwise command handler won't find it in .spawned.
             // WITHOUT OWNER = WITHOUT AUTHORITY for this test
-            CreateNetworkedAndSpawn(out _, out _, out CommandTestNetworkBehaviour comp,
-                                    out _, out _, out _);
+            CreateNetworkedAndSpawn(out _, out _, out CommandTestNetworkBehaviour serverComponent,
+                                    out _, out _, out CommandTestNetworkBehaviour clientComponent);
 
             // call the command
-            comp.TestCommand();
+            clientComponent.TestCommand();
             ProcessMessages();
-            Assert.That(comp.called, Is.EqualTo(0));
+            Assert.That(serverComponent.called, Is.EqualTo(0));
         }
 
         [Test]

Assets/Mirror/Tests/Editor/Rpcs/CommandTest.cs

@@ -277,5 +277,25 @@ public void Command_RequiresAuthorityFalse_ForOtherObjectWithoutConnectionToServ
             ProcessMessages();
             Assert.That(called, Is.EqualTo(1));
         }
+
+        // cmds must be invoked immediately on host, without processing.
+        // (see Weaver CommandProcessor comments)
+        [Test]
+        public void Command_IsInvokeImmediately()
+        {
+            // spawn without owner (= without connectionToClient)
+            CreateNetworkedAndSpawn(out _, out _, out IgnoreAuthorityBehaviour comp);
+
+            // setup callback
+            int called = 0;
+            comp.onSendInt += _ => { ++called; };
+
+            // call command. don't require authority.
+            // the object doesn't have a .connectionToServer (like a scene object)
+            Assert.That(comp.connectionToServer, Is.Null);
+            comp.CmdSendInt(0);
+            // ProcessMessages(); <- should be invoked IMMEDIATELY without processing
+            Assert.That(called, Is.EqualTo(1));
+        }
     }
 }