From aa07770039f5527f022fbb035bdcfa9422fc89e3 Mon Sep 17 00:00:00 2001 From: Radek Zikmund <32671551+rzikm@users.noreply.github.com> Date: Thu, 25 Apr 2024 18:32:18 +0200 Subject: [PATCH] Avoid some defensive copies on readonly structs in System.Net.Quic (#101133) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Avoid some defensive copies on readonly structs in System.Net.Quic * Keep readonly-ness of CacheKey * Apply suggestions from code review * Remove ReadOnlySpan property * Update src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs Co-authored-by: Marie Píchová <11718369+ManickaP@users.noreply.github.com> * Code review feedback * Implement IEquattable for QUIC_SETTINGS * More code review feedback * Code review feedback --------- Co-authored-by: Marie Píchová <11718369+ManickaP@users.noreply.github.com> --- .../Net/Quic/Internal/MsQuicSafeHandle.cs | 2 +- .../Quic/Interop/QUIC_SETTINGS.IEquattable.cs | 94 +++++++++++++++++++ .../src/System/Net/Quic/Interop/msquic.cs | 2 +- .../FunctionalTests/MsQuicInteropTests.cs | 74 +++++++++++++++ 4 files changed, 170 insertions(+), 2 deletions(-) create mode 100644 src/libraries/System.Net.Quic/src/System/Net/Quic/Interop/QUIC_SETTINGS.IEquattable.cs create mode 100644 src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicInteropTests.cs diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicSafeHandle.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicSafeHandle.cs index cf7d70a18e08d..8e70ec2057245 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicSafeHandle.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicSafeHandle.cs @@ -88,7 +88,7 @@ internal sealed class MsQuicContextSafeHandle : MsQuicSafeHandle /// Holds a weak reference to the managed instance. /// It allows delegating MsQuic events to the managed object while it still can be collected and finalized. /// - private readonly GCHandle _context; + private GCHandle _context; /// /// Optional parent safe handle, used to increment/decrement reference count with the lifetime of this instance. diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Interop/QUIC_SETTINGS.IEquattable.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Interop/QUIC_SETTINGS.IEquattable.cs new file mode 100644 index 0000000000000..3f008c74136d1 --- /dev/null +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Interop/QUIC_SETTINGS.IEquattable.cs @@ -0,0 +1,94 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; + +namespace Microsoft.Quic; + +internal partial struct QUIC_SETTINGS : System.IEquatable +{ + // Because QUIC_SETTINGS may contain gaps due to layout/alignment of individual + // fields, we implement IEquatable manually. If a new field is added, + // then there is a unit test which should fail. + + public readonly bool Equals(QUIC_SETTINGS other) + { + return Anonymous1.IsSetFlags == other.Anonymous1.IsSetFlags + && MaxBytesPerKey == other.MaxBytesPerKey + && HandshakeIdleTimeoutMs == other.HandshakeIdleTimeoutMs + && IdleTimeoutMs == other.IdleTimeoutMs + && MtuDiscoverySearchCompleteTimeoutUs == other.MtuDiscoverySearchCompleteTimeoutUs + && TlsClientMaxSendBuffer == other.TlsClientMaxSendBuffer + && TlsServerMaxSendBuffer == other.TlsServerMaxSendBuffer + && StreamRecvWindowDefault == other.StreamRecvWindowDefault + && StreamRecvBufferDefault == other.StreamRecvBufferDefault + && ConnFlowControlWindow == other.ConnFlowControlWindow + && MaxWorkerQueueDelayUs == other.MaxWorkerQueueDelayUs + && MaxStatelessOperations == other.MaxStatelessOperations + && InitialWindowPackets == other.InitialWindowPackets + && SendIdleTimeoutMs == other.SendIdleTimeoutMs + && InitialRttMs == other.InitialRttMs + && MaxAckDelayMs == other.MaxAckDelayMs + && DisconnectTimeoutMs == other.DisconnectTimeoutMs + && KeepAliveIntervalMs == other.KeepAliveIntervalMs + && CongestionControlAlgorithm == other.CongestionControlAlgorithm + && PeerBidiStreamCount == other.PeerBidiStreamCount + && PeerUnidiStreamCount == other.PeerUnidiStreamCount + && MaxBindingStatelessOperations == other.MaxBindingStatelessOperations + && StatelessOperationExpirationMs == other.StatelessOperationExpirationMs + && MinimumMtu == other.MinimumMtu + && MaximumMtu == other.MaximumMtu + && _bitfield == other._bitfield + && MaxOperationsPerDrain == other.MaxOperationsPerDrain + && MtuDiscoveryMissingProbeCount == other.MtuDiscoveryMissingProbeCount + && DestCidUpdateIdleTimeoutMs == other.DestCidUpdateIdleTimeoutMs + && Anonymous2.Flags == other.Anonymous2.Flags + && StreamRecvWindowBidiLocalDefault == other.StreamRecvWindowBidiLocalDefault + && StreamRecvWindowBidiRemoteDefault == other.StreamRecvWindowBidiRemoteDefault + && StreamRecvWindowUnidiDefault == other.StreamRecvWindowUnidiDefault; + } + + public override readonly int GetHashCode() + { + HashCode hash = default; + hash.Add(Anonymous1.IsSetFlags); + hash.Add(MaxBytesPerKey); + hash.Add(HandshakeIdleTimeoutMs); + hash.Add(IdleTimeoutMs); + hash.Add(MtuDiscoverySearchCompleteTimeoutUs); + hash.Add(TlsClientMaxSendBuffer); + hash.Add(TlsServerMaxSendBuffer); + hash.Add(StreamRecvWindowDefault); + hash.Add(StreamRecvBufferDefault); + hash.Add(ConnFlowControlWindow); + hash.Add(MaxWorkerQueueDelayUs); + hash.Add(MaxStatelessOperations); + hash.Add(InitialWindowPackets); + hash.Add(SendIdleTimeoutMs); + hash.Add(InitialRttMs); + hash.Add(MaxAckDelayMs); + hash.Add(DisconnectTimeoutMs); + hash.Add(KeepAliveIntervalMs); + hash.Add(CongestionControlAlgorithm); + hash.Add(PeerBidiStreamCount); + hash.Add(PeerUnidiStreamCount); + hash.Add(MaxBindingStatelessOperations); + hash.Add(StatelessOperationExpirationMs); + hash.Add(MinimumMtu); + hash.Add(MaximumMtu); + hash.Add(_bitfield); + hash.Add(MaxOperationsPerDrain); + hash.Add(MtuDiscoveryMissingProbeCount); + hash.Add(DestCidUpdateIdleTimeoutMs); + hash.Add(Anonymous2.Flags); + hash.Add(StreamRecvWindowBidiLocalDefault); + hash.Add(StreamRecvWindowBidiRemoteDefault); + hash.Add(StreamRecvWindowUnidiDefault); + return hash.ToHashCode(); + } + + public override readonly bool Equals(object? obj) + { + return obj is QUIC_SETTINGS other && Equals(other); + } +} diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Interop/msquic.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Interop/msquic.cs index 26296b1f6a89b..29b4c822d6236 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Interop/msquic.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Interop/msquic.cs @@ -20,7 +20,7 @@ namespace Microsoft.Quic { internal unsafe partial struct QUIC_BUFFER { - public Span Span => new(Buffer, (int)Length); + public readonly Span Span => new(Buffer, (int)Length); } internal partial class MsQuic diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicInteropTests.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicInteropTests.cs new file mode 100644 index 0000000000000..5ffd5b53510d2 --- /dev/null +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicInteropTests.cs @@ -0,0 +1,74 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Diagnostics; +using System.Net.Security; +using System.Threading.Tasks; +using System.Runtime.InteropServices; +using System.Reflection; +using System.Linq; +using Xunit; +using Xunit.Abstractions; + +using Microsoft.Quic; + +namespace System.Net.Quic.Tests +{ + public class MsQuicInteropTests + { + private static MemberInfo[] GetMembers() + { + var members = typeof(T).FindMembers(MemberTypes.Field | MemberTypes.Property, BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.Public, (mi, _) => + { + if (mi is PropertyInfo property && property.GetSetMethod() == null) + { + return false; + } + + return true; + }, null); + + Assert.NotEmpty(members); + + return members; + } + + private static void ResetMember(MemberInfo member, object instance) + { + switch (member) + { + case FieldInfo field: + field.SetValue(instance, Activator.CreateInstance(field.FieldType)); + break; + case PropertyInfo property: + property.SetValue(instance, Activator.CreateInstance(property.PropertyType)); + break; + default: + throw new InvalidOperationException($"Unexpected member type: {member.MemberType}"); + } + } + + [Fact] + public void QuicSettings_Equals_RespectsAllMembers() + { + QUIC_SETTINGS settings = new QUIC_SETTINGS(); + + // make sure the extension definition is included in compilation + Assert.Contains(typeof(IEquatable), typeof(QUIC_SETTINGS).GetInterfaces()); + + var settingsSpan = MemoryMarshal.AsBytes(new Span(ref settings)); + + // Fill the memory with 1s,we will try to zero out each member and compare + settingsSpan.Fill(0xFF); + + foreach (MemberInfo member in GetMembers()) + { + // copy and box the instance because reflection methods take a reference type arg + object boxed = settings; + ResetMember(member, boxed); + Assert.False(settings.Equals((QUIC_SETTINGS)boxed), $"Member {member.Name} is not compared."); + } + } + } +}