Add separate DeserializePacket() to INodePacketFactory (#11650)

JanProvaznik · web-flow · commit d9cc23c4b98d · 2025-04-07T10:33:29.000+02:00
### Context Minimum change to get shared IPC added in #11546 recompiling again for #11383 (since the original PR was reverted). ### Changes Made - Splits up `DeserializeAndRoutePacket()` in `PacketFactoryRecord` (private class) - Adds `DeserializePacket()` to `INodePacketFactory` and all implementations. Does not remove old method. - Adds ifdef's to shared pipe classes for compilation purposes No behavior changes to MSBuild.
diff --git a/eng/SourceBuildPrebuiltBaseline.xml b/eng/SourceBuildPrebuiltBaseline.xml
@@ -24,6 +24,8 @@
     <UsagePattern IdentityGlob="System.Diagnostics.EventLog/*9.0.0*" />
     <!-- dependency of System.Resources.Extensions -->
     <UsagePattern IdentityGlob="System.Formats.Nrbf/*9.0.0*" />
+    <!-- dependency of System.System.Threading.Channels -->
+    <UsagePattern IdentityGlob="Microsoft.Bcl.AsyncInterfaces/*9.0.0*" />
     <!-- dependency of System.Security.Cryptography.Pkcs -->
     <UsagePattern IdentityGlob="Microsoft.Bcl.Cryptography/*9.0.0*" />
   </IgnorePatterns>
diff --git a/src/Build.UnitTests/BackEnd/NodeEndpointInProc_Tests.cs b/src/Build.UnitTests/BackEnd/NodeEndpointInProc_Tests.cs
@@ -105,6 +105,11 @@ public void DeserializeAndRoutePacket(int nodeId, NodePacketType packetType, ITr
                 throw new NotImplementedException();
             }
 
+            public INodePacket DeserializePacket(NodePacketType packetType, ITranslator translator)
+            {
+                throw new NotImplementedException();
+            }
+
             public void RoutePacket(int nodeId, INodePacket packet)
             {
                 _dataReceivedContext = new DataReceivedContext(Thread.CurrentThread, packet);
diff --git a/src/Build/BackEnd/Client/MSBuildClientPacketPump.cs b/src/Build/BackEnd/Client/MSBuildClientPacketPump.cs
@@ -125,6 +125,16 @@ public void DeserializeAndRoutePacket(int nodeId, NodePacketType packetType, ITr
             _packetFactory.DeserializeAndRoutePacket(nodeId, packetType, translator);
         }
 
+        /// <summary>
+        /// Deserializes a packet.
+        /// </summary>
+        /// <param name="packetType">The packet type.</param>
+        /// <param name="translator">The translator to use as a source for packet data.</param>
+        public INodePacket DeserializePacket(NodePacketType packetType, ITranslator translator)
+        {
+            return _packetFactory.DeserializePacket(packetType, translator);
+        }
+
         /// <summary>
         /// Routes a packet to the appropriate handler.
         /// </summary>
diff --git a/src/Build/BackEnd/Components/Communications/NodeManager.cs b/src/Build/BackEnd/Components/Communications/NodeManager.cs
@@ -255,6 +255,16 @@ public void DeserializeAndRoutePacket(int nodeId, NodePacketType packetType, ITr
             _packetFactory.DeserializeAndRoutePacket(nodeId, packetType, translator);
         }
 
+        /// <summary>
+        /// Takes a serializer and deserializes the packet.
+        /// </summary>
+        /// <param name="packetType">The packet type.</param>
+        /// <param name="translator">The translator containing the data from which the packet should be reconstructed.</param>
+        public INodePacket DeserializePacket(NodePacketType packetType, ITranslator translator)
+        {
+            return _packetFactory.DeserializePacket(packetType, translator);
+        }
+
         /// <summary>
         /// Routes the specified packet. This is called by the Inproc node directly since it does not have to do any deserialization
         /// </summary>
diff --git a/src/Build/BackEnd/Components/Communications/NodeProviderInProc.cs b/src/Build/BackEnd/Components/Communications/NodeProviderInProc.cs
@@ -292,6 +292,16 @@ public void DeserializeAndRoutePacket(int nodeId, NodePacketType packetType, ITr
             ErrorUtilities.ThrowInternalErrorUnreachable();
         }
 
+        /// <summary>
+        /// Deserializes and routes a packet.  Not used in the in-proc node.
+        /// </summary>
+        public INodePacket DeserializePacket(NodePacketType packetType, ITranslator translator)
+        {
+            // Not used
+            ErrorUtilities.ThrowInternalErrorUnreachable();
+            return null;
+        }
+
         /// <summary>
         /// Routes a packet.
         /// </summary>
diff --git a/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcTaskHost.cs b/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcTaskHost.cs
@@ -287,6 +287,16 @@ public void DeserializeAndRoutePacket(int nodeId, NodePacketType packetType, ITr
             }
         }
 
+        /// <summary>
+        /// Takes a serializer and deserializes the packet.
+        /// </summary>
+        /// <param name="packetType">The packet type.</param>
+        /// <param name="translator">The translator containing the data from which the packet should be reconstructed.</param>
+        public INodePacket DeserializePacket(NodePacketType packetType, ITranslator translator)
+        {
+            return _localPacketFactory.DeserializePacket(packetType, translator);
+        }
+
         /// <summary>
         /// Routes the specified packet
         /// </summary>
diff --git a/src/Build/BackEnd/Components/Communications/TaskHostNodeManager.cs b/src/Build/BackEnd/Components/Communications/TaskHostNodeManager.cs
@@ -149,6 +149,16 @@ public void DeserializeAndRoutePacket(int nodeId, NodePacketType packetType, ITr
             throw new NotSupportedException("not used");
         }
 
+        /// <summary>
+        /// Takes a serializer, deserializes the packet and routes it to the appropriate handler.
+        /// </summary>
+        /// <param name="packetType">The packet type.</param>
+        /// <param name="translator">The translator containing the data from which the packet should be reconstructed.</param>
+        public INodePacket DeserializePacket(NodePacketType packetType, ITranslator translator)
+        {
+            throw new NotSupportedException("not used");
+        }
+
         /// <summary>
         /// Routes the specified packet. This is called by the Inproc node directly since it does not have to do any deserialization
         /// </summary>
diff --git a/src/Build/BackEnd/Node/InProcNode.cs b/src/Build/BackEnd/Node/InProcNode.cs
@@ -222,6 +222,16 @@ public void DeserializeAndRoutePacket(int nodeId, NodePacketType packetType, ITr
             ErrorUtilities.ThrowInternalError("Unexpected call to DeserializeAndRoutePacket on the in-proc node.");
         }
 
+        /// <summary>
+        /// Not necessary for in-proc node - we don't serialize.
+        /// </summary>
+        public INodePacket DeserializePacket(NodePacketType packetType, ITranslator translator)
+        {
+            // The in-proc endpoint shouldn't be serializing, just routing.
+            ErrorUtilities.ThrowInternalError("Unexpected call to DeserializePacket on the in-proc node.");
+            return null;
+        }
+
         /// <summary>
         /// Routes the packet to the appropriate handler.
         /// </summary>
diff --git a/src/Build/BackEnd/Node/OutOfProcNode.cs b/src/Build/BackEnd/Node/OutOfProcNode.cs
@@ -343,6 +343,16 @@ void INodePacketFactory.DeserializeAndRoutePacket(int nodeId, NodePacketType pac
             _packetFactory.DeserializeAndRoutePacket(nodeId, packetType, translator);
         }
 
+        /// <summary>
+        /// Deserializes a packet.
+        /// </summary>
+        /// <param name="packetType">The packet type.</param>
+        /// <param name="translator">The translator to use as a source for packet data.</param>
+        INodePacket INodePacketFactory.DeserializePacket(NodePacketType packetType, ITranslator translator)
+        {
+            return _packetFactory.DeserializePacket(packetType, translator);
+        }
+
         /// <summary>
         /// Routes a packet to the appropriate handler.
         /// </summary>
diff --git a/src/Build/BackEnd/Node/OutOfProcServerNode.cs b/src/Build/BackEnd/Node/OutOfProcServerNode.cs
@@ -210,6 +210,16 @@ void INodePacketFactory.DeserializeAndRoutePacket(int nodeId, NodePacketType pac
             _packetFactory.DeserializeAndRoutePacket(nodeId, packetType, translator);
         }
 
+        /// <summary>
+        /// Deserializes a packet.
+        /// </summary>
+        /// <param name="packetType">The packet type.</param>
+        /// <param name="translator">The translator to use as a source for packet data.</param>
+        INodePacket INodePacketFactory.DeserializePacket(NodePacketType packetType, ITranslator translator)
+        {
+            return _packetFactory.DeserializePacket(packetType, translator);
+        }
+
         /// <summary>
         /// Routes a packet to the appropriate handler.
         /// </summary>
diff --git a/src/Build/Instance/TaskFactories/TaskHostTask.cs b/src/Build/Instance/TaskFactories/TaskHostTask.cs
@@ -374,6 +374,16 @@ public void DeserializeAndRoutePacket(int nodeId, NodePacketType packetType, ITr
             _packetFactory.DeserializeAndRoutePacket(nodeId, packetType, translator);
         }
 
+        /// <summary>
+        /// Takes a serializer and deserializes the packet.
+        /// </summary>
+        /// <param name="packetType">The packet type.</param>
+        /// <param name="translator">The translator containing the data from which the packet should be reconstructed.</param>
+        public INodePacket DeserializePacket(NodePacketType packetType, ITranslator translator)
+        {
+            return _packetFactory.DeserializePacket(packetType, translator);
+        }
+
         /// <summary>
         /// Routes the specified packet
         /// </summary>
diff --git a/src/MSBuild/OutOfProcTaskHostNode.cs b/src/MSBuild/OutOfProcTaskHostNode.cs
@@ -591,6 +591,16 @@ public void DeserializeAndRoutePacket(int nodeId, NodePacketType packetType, ITr
             _packetFactory.DeserializeAndRoutePacket(nodeId, packetType, translator);
         }
 
+        /// <summary>
+        /// Takes a serializer and deserializes the packet.
+        /// </summary>
+        /// <param name="packetType">The packet type.</param>
+        /// <param name="translator">The translator containing the data from which the packet should be reconstructed.</param>
+        public INodePacket DeserializePacket(NodePacketType packetType, ITranslator translator)
+        {
+            return _packetFactory.DeserializePacket(packetType, translator);
+        }
+
         /// <summary>
         /// Routes the specified packet
         /// </summary>
diff --git a/src/Shared/INodePacketFactory.cs b/src/Shared/INodePacketFactory.cs
@@ -43,7 +43,14 @@ internal interface INodePacketFactory
         void DeserializeAndRoutePacket(int nodeId, NodePacketType packetType, ITranslator translator);
 
         /// <summary>
-        /// Routes the specified packet
+        /// Takes a serializer and deserializes the packet.
+        /// </summary>
+        /// <param name="packetType">The packet type.</param>
+        /// <param name="translator">The translator containing the data from which the packet should be reconstructed.</param>
+        INodePacket DeserializePacket(NodePacketType packetType, ITranslator translator);
+
+        /// <summary>
+        /// Routes the specified packet.
         /// </summary>
         /// <param name="nodeId">The node from which the packet was received.</param>
         /// <param name="packet">The packet to route.</param>
diff --git a/src/Shared/NodePacketFactory.cs b/src/Shared/NodePacketFactory.cs
@@ -55,15 +55,35 @@ public void DeserializeAndRoutePacket(int nodeId, NodePacketType packetType, ITr
                 ErrorUtilities.ThrowInternalError("No packet handler for type {0}", packetType);
             }
 
-            record.DeserializeAndRoutePacket(nodeId, translator);
+            INodePacket packet = record.DeserializePacket(translator);
+            record.RoutePacket(nodeId, packet);
+        }
+
+        /// <summary>
+        /// Creates a packet with data from a binary stream.
+        /// </summary>
+        public INodePacket DeserializePacket(NodePacketType packetType, ITranslator translator)
+        {
+            // PERF: Not using VerifyThrow to avoid boxing of packetType in the non-error case
+            if (!_packetFactories.TryGetValue(packetType, out PacketFactoryRecord record))
+            {
+                ErrorUtilities.ThrowInternalError("No packet handler for type {0}", packetType);
+            }
+
+            return record.DeserializePacket(translator);
         }
 
         /// <summary>
         /// Routes the specified packet.
         /// </summary>
         public void RoutePacket(int nodeId, INodePacket packet)
         {
-            PacketFactoryRecord record = _packetFactories[packet.Type];
+            // PERF: Not using VerifyThrow to avoid boxing of packetType in the non-error case
+            if (!_packetFactories.TryGetValue(packet.Type, out PacketFactoryRecord record))
+            {
+                ErrorUtilities.ThrowInternalError("No packet handler for type {0}", packet.Type);
+            }
+
             record.RoutePacket(nodeId, packet);
         }
 
@@ -77,12 +97,12 @@ private class PacketFactoryRecord
             /// <summary>
             /// The handler to invoke when the packet is deserialized.
             /// </summary>
-            private INodePacketHandler _handler;
+            private readonly INodePacketHandler _handler;
 
             /// <summary>
             /// The method used to construct a packet from a translator stream.
             /// </summary>
-            private NodePacketFactoryMethod _factoryMethod;
+            private readonly NodePacketFactoryMethod _factoryMethod;
 
             /// <summary>
             /// Constructor.
@@ -94,21 +114,14 @@ public PacketFactoryRecord(INodePacketHandler handler, NodePacketFactoryMethod f
             }
 
             /// <summary>
-            /// Creates a packet from a binary stream and sends it to the registered handler.
+            /// Creates a packet from a binary stream.
             /// </summary>
-            public void DeserializeAndRoutePacket(int nodeId, ITranslator translator)
-            {
-                INodePacket packet = _factoryMethod(translator);
-                RoutePacket(nodeId, packet);
-            }
+            public INodePacket DeserializePacket(ITranslator translator) => _factoryMethod(translator);
 
             /// <summary>
             /// Routes the packet to the correct destination.
             /// </summary>
-            public void RoutePacket(int nodeId, INodePacket packet)
-            {
-                _handler.PacketReceived(nodeId, packet);
-            }
+            public void RoutePacket(int nodeId, INodePacket packet) => _handler.PacketReceived(nodeId, packet);
         }
     }
 }
diff --git a/src/Shared/NodePipeClient.cs b/src/Shared/NodePipeClient.cs
@@ -84,7 +84,11 @@ private void PerformHandshake(int timeout)
             _pipeClient.WriteEndOfHandshakeSignal();
 
             CommunicationsUtilities.Trace("Reading handshake from pipe {0}", PipeName);
+#if NET
             _pipeClient.ReadEndOfHandshakeSignal(true, timeout);
+#else
+            _pipeClient.ReadEndOfHandshakeSignal(true);
+#endif
         }
     }
 }
diff --git a/src/Shared/NodePipeServer.cs b/src/Shared/NodePipeServer.cs
@@ -25,11 +25,13 @@ internal sealed class NodePipeServer : NodePipeBase
         /// </summary>
         private const int PipeBufferSize = 131_072;
 
+#if NET
         /// <summary>
         /// A timeout for the handshake. This is only used on Unix-like socket implementations, because the
         /// timeout on the PipeStream connection is ignore.
         /// </summary>
         private static readonly int s_handshakeTimeout = NativeMethodsShared.IsWindows ? 0 : 60_000;
+#endif
 
         private readonly NamedPipeServerStream _pipeServer;
 
@@ -179,7 +181,11 @@ private bool ValidateHandshake()
             for (int i = 0; i < HandshakeComponents.Length; i++)
             {
                 // This will disconnect a < 16.8 host; it expects leading 00 or F5 or 06. 0x00 is a wildcard.
+#if NET
                 int handshakePart = _pipeServer.ReadIntForHandshake(byteToAccept: i == 0 ? CommunicationsUtilities.handshakeVersion : null, s_handshakeTimeout);
+#else
+                int handshakePart = _pipeServer.ReadIntForHandshake(byteToAccept: i == 0 ? CommunicationsUtilities.handshakeVersion : null);
+#endif
 
                 if (handshakePart != HandshakeComponents[i])
                 {
@@ -190,7 +196,11 @@ private bool ValidateHandshake()
             }
 
             // To ensure that our handshake and theirs have the same number of bytes, receive and send a magic number indicating EOS.
+#if NET
             _pipeServer.ReadEndOfHandshakeSignal(false, s_handshakeTimeout);
+#else
+            _pipeServer.ReadEndOfHandshakeSignal(false);
+#endif
 
             CommunicationsUtilities.Trace("Successfully connected to parent.");
             _pipeServer.WriteEndOfHandshakeSignal();

Original file line number	Diff line number	Diff line change
`@@ -105,6 +105,11 @@ public void DeserializeAndRoutePacket(int nodeId, NodePacketType packetType, ITr`
`105`	`105`	`throw new NotImplementedException();`
`106`	`106`	`}`
`107`	`107`
	`108`	`+ public INodePacket DeserializePacket(NodePacketType packetType, ITranslator translator)`
	`109`	`+ {`
	`110`	`+ throw new NotImplementedException();`
	`111`	`+ }`
	`112`	`+`
`108`	`113`	`public void RoutePacket(int nodeId, INodePacket packet)`
`109`	`114`	`{`
`110`	`115`	`_dataReceivedContext = new DataReceivedContext(Thread.CurrentThread, packet);`