From 1c7b419097c07a21e4a6bd8d7bde85942c1c015d Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Tue, 16 Apr 2024 20:25:24 +0200 Subject: [PATCH 1/9] Avoid some defensive copies on readonly structs in System.Net.Quic --- .../Quic/Internal/MsQuicConfiguration.Cache.cs | 15 ++++++++------- .../System/Net/Quic/Internal/MsQuicSafeHandle.cs | 2 +- .../System/Net/Quic/Internal/ReceiveBuffers.cs | 2 +- .../src/System/Net/Quic/Interop/msquic.cs | 1 + 4 files changed, 11 insertions(+), 9 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs index 4fc86adfbd1565..0fe8031df05c00 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs @@ -7,6 +7,7 @@ using System.Collections.ObjectModel; using System.Security.Authentication; using System.Net.Security; +using System.Runtime.InteropServices; using System.Security.Cryptography.X509Certificates; using System.Threading; using Microsoft.Quic; @@ -39,13 +40,13 @@ private static bool GetConfigurationCacheEnabled() private static readonly ConcurrentDictionary s_configurationCache = new(); - private readonly struct CacheKey : IEquatable + private struct CacheKey : IEquatable { - public readonly List CertificateThumbprints; - public readonly QUIC_CREDENTIAL_FLAGS Flags; - public readonly QUIC_SETTINGS Settings; - public readonly List ApplicationProtocols; - public readonly QUIC_ALLOWED_CIPHER_SUITE_FLAGS AllowedCipherSuites; + private readonly List CertificateThumbprints; + private readonly QUIC_CREDENTIAL_FLAGS Flags; + private QUIC_SETTINGS Settings; // not readonly to be able to compare using MemoryMarshal + private readonly List ApplicationProtocols; + private readonly QUIC_ALLOWED_CIPHER_SUITE_FLAGS AllowedCipherSuites; public CacheKey(QUIC_SETTINGS settings, QUIC_CREDENTIAL_FLAGS flags, X509Certificate? certificate, ReadOnlyCollection? intermediates, List alpnProtocols, QUIC_ALLOWED_CIPHER_SUITE_FLAGS allowedCipherSuites) { @@ -98,7 +99,7 @@ public bool Equals(CacheKey other) return Flags == other.Flags && - Settings.Equals(other.Settings) && + MemoryMarshal.AsBytes(MemoryMarshal.CreateReadOnlySpan(ref Settings, 1)).SequenceEqual(MemoryMarshal.AsBytes(MemoryMarshal.CreateReadOnlySpan(ref other.Settings, 1))) && AllowedCipherSuites == other.AllowedCipherSuites; } 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 cf7d70a18e08d1..8e70ec20572454 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/Internal/ReceiveBuffers.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ReceiveBuffers.cs index 93f78acc87f028..b54a96bd616681 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ReceiveBuffers.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ReceiveBuffers.cs @@ -52,7 +52,7 @@ public int CopyFrom(ReadOnlySpan quicBuffers, int totalLength, bool int totalCopied = 0; for (int i = 0; i < quicBuffers.Length; ++i) { - Span quicBuffer = quicBuffers[i].Span; + ReadOnlySpan quicBuffer = quicBuffers[i].ReadOnlySpan; if (totalLength < quicBuffer.Length) { quicBuffer = quicBuffer.Slice(0, totalLength); 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 26296b1f6a89b8..ffcb3262b2b951 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 @@ -21,6 +21,7 @@ namespace Microsoft.Quic internal unsafe partial struct QUIC_BUFFER { public Span Span => new(Buffer, (int)Length); + public readonly ReadOnlySpan ReadOnlySpan => new(Buffer, (int)Length); } internal partial class MsQuic From 43981504242447f45e32015cb48a878969ecef36 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Tue, 16 Apr 2024 20:38:03 +0200 Subject: [PATCH 2/9] Keep readonly-ness of CacheKey --- .../Quic/Internal/MsQuicConfiguration.Cache.cs | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs index 0fe8031df05c00..9677bb59679914 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs @@ -7,6 +7,7 @@ using System.Collections.ObjectModel; using System.Security.Authentication; using System.Net.Security; +using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using System.Security.Cryptography.X509Certificates; using System.Threading; @@ -40,13 +41,15 @@ private static bool GetConfigurationCacheEnabled() private static readonly ConcurrentDictionary s_configurationCache = new(); - private struct CacheKey : IEquatable + private readonly struct CacheKey : IEquatable { - private readonly List CertificateThumbprints; - private readonly QUIC_CREDENTIAL_FLAGS Flags; - private QUIC_SETTINGS Settings; // not readonly to be able to compare using MemoryMarshal - private readonly List ApplicationProtocols; - private readonly QUIC_ALLOWED_CIPHER_SUITE_FLAGS AllowedCipherSuites; + public readonly List CertificateThumbprints; + public readonly QUIC_CREDENTIAL_FLAGS Flags; + public readonly QUIC_SETTINGS Settings; + public readonly List ApplicationProtocols; + public readonly QUIC_ALLOWED_CIPHER_SUITE_FLAGS AllowedCipherSuites; + + private readonly ReadOnlySpan SettingsSpan => MemoryMarshal.AsBytes(new ReadOnlySpan(ref Unsafe.AsRef(in Settings))); public CacheKey(QUIC_SETTINGS settings, QUIC_CREDENTIAL_FLAGS flags, X509Certificate? certificate, ReadOnlyCollection? intermediates, List alpnProtocols, QUIC_ALLOWED_CIPHER_SUITE_FLAGS allowedCipherSuites) { @@ -99,7 +102,7 @@ public bool Equals(CacheKey other) return Flags == other.Flags && - MemoryMarshal.AsBytes(MemoryMarshal.CreateReadOnlySpan(ref Settings, 1)).SequenceEqual(MemoryMarshal.AsBytes(MemoryMarshal.CreateReadOnlySpan(ref other.Settings, 1))) && + SettingsSpan.SequenceEqual(other.SettingsSpan) && AllowedCipherSuites == other.AllowedCipherSuites; } From 45bbf458d7e06664dd0aa95454e117b456680244 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Tue, 16 Apr 2024 20:52:00 +0200 Subject: [PATCH 3/9] Apply suggestions from code review --- .../src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs index 9677bb59679914..ebe5fc7804b8bc 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs @@ -2,12 +2,12 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; using System.Collections.Generic; using System.Collections.Concurrent; using System.Collections.ObjectModel; using System.Security.Authentication; using System.Net.Security; -using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using System.Security.Cryptography.X509Certificates; using System.Threading; @@ -49,7 +49,7 @@ private static bool GetConfigurationCacheEnabled() public readonly List ApplicationProtocols; public readonly QUIC_ALLOWED_CIPHER_SUITE_FLAGS AllowedCipherSuites; - private readonly ReadOnlySpan SettingsSpan => MemoryMarshal.AsBytes(new ReadOnlySpan(ref Unsafe.AsRef(in Settings))); + [UnscopedRef] private readonly ReadOnlySpan SettingsSpan => MemoryMarshal.AsBytes(new ReadOnlySpan(in Settings)); public CacheKey(QUIC_SETTINGS settings, QUIC_CREDENTIAL_FLAGS flags, X509Certificate? certificate, ReadOnlyCollection? intermediates, List alpnProtocols, QUIC_ALLOWED_CIPHER_SUITE_FLAGS allowedCipherSuites) { From 8f5ae6d59e2a0276aadae6c82acc62f97cb1b8ed Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Wed, 17 Apr 2024 09:46:57 +0200 Subject: [PATCH 4/9] Remove ReadOnlySpan property --- .../src/System/Net/Quic/Internal/ReceiveBuffers.cs | 2 +- .../System.Net.Quic/src/System/Net/Quic/Interop/msquic.cs | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ReceiveBuffers.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ReceiveBuffers.cs index b54a96bd616681..1326251a11e977 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ReceiveBuffers.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ReceiveBuffers.cs @@ -52,7 +52,7 @@ public int CopyFrom(ReadOnlySpan quicBuffers, int totalLength, bool int totalCopied = 0; for (int i = 0; i < quicBuffers.Length; ++i) { - ReadOnlySpan quicBuffer = quicBuffers[i].ReadOnlySpan; + ReadOnlySpan quicBuffer = quicBuffers[i].Span; if (totalLength < quicBuffer.Length) { quicBuffer = quicBuffer.Slice(0, totalLength); 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 ffcb3262b2b951..29b4c822d62367 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,8 +20,7 @@ namespace Microsoft.Quic { internal unsafe partial struct QUIC_BUFFER { - public Span Span => new(Buffer, (int)Length); - public readonly ReadOnlySpan ReadOnlySpan => new(Buffer, (int)Length); + public readonly Span Span => new(Buffer, (int)Length); } internal partial class MsQuic From c69ba960dcbef3d550326e7bf77607efab763863 Mon Sep 17 00:00:00 2001 From: Radek Zikmund <32671551+rzikm@users.noreply.github.com> Date: Wed, 17 Apr 2024 11:33:24 +0200 Subject: [PATCH 5/9] Update src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Marie Píchová <11718369+ManickaP@users.noreply.github.com> --- .../src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs index ebe5fc7804b8bc..b6199bdb80d5ec 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs @@ -49,7 +49,8 @@ private static bool GetConfigurationCacheEnabled() public readonly List ApplicationProtocols; public readonly QUIC_ALLOWED_CIPHER_SUITE_FLAGS AllowedCipherSuites; - [UnscopedRef] private readonly ReadOnlySpan SettingsSpan => MemoryMarshal.AsBytes(new ReadOnlySpan(in Settings)); + [UnscopedRef] + private readonly ReadOnlySpan SettingsSpan => MemoryMarshal.AsBytes(new ReadOnlySpan(in Settings)); public CacheKey(QUIC_SETTINGS settings, QUIC_CREDENTIAL_FLAGS flags, X509Certificate? certificate, ReadOnlyCollection? intermediates, List alpnProtocols, QUIC_ALLOWED_CIPHER_SUITE_FLAGS allowedCipherSuites) { From 7821590da636416c1df5ee41c4e9fcaef7913770 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Wed, 17 Apr 2024 11:41:50 +0200 Subject: [PATCH 6/9] Code review feedback --- .../System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs index b6199bdb80d5ec..54b32184a2ce53 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs @@ -49,7 +49,10 @@ private static bool GetConfigurationCacheEnabled() public readonly List ApplicationProtocols; public readonly QUIC_ALLOWED_CIPHER_SUITE_FLAGS AllowedCipherSuites; - [UnscopedRef] + // Span-based access to settings for fast comparison and hashing. This + // avoids boxing and defensive copies due to accessing readonly struct + // fields. + [UnscopedRef] private readonly ReadOnlySpan SettingsSpan => MemoryMarshal.AsBytes(new ReadOnlySpan(in Settings)); public CacheKey(QUIC_SETTINGS settings, QUIC_CREDENTIAL_FLAGS flags, X509Certificate? certificate, ReadOnlyCollection? intermediates, List alpnProtocols, QUIC_ALLOWED_CIPHER_SUITE_FLAGS allowedCipherSuites) @@ -117,7 +120,7 @@ public override int GetHashCode() } hash.Add(Flags); - hash.Add(Settings); + hash.AddBytes(SettingsSpan); foreach (var protocol in ApplicationProtocols) { From 1df80d9401cdc9cbbf4a0e2e574a55a1d17e2d91 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Mon, 22 Apr 2024 13:45:08 +0200 Subject: [PATCH 7/9] Implement IEquattable for QUIC_SETTINGS --- .../Internal/MsQuicConfiguration.Cache.cs | 10 +- .../Quic/Interop/QUIC_SETTINGS.IEquattable.cs | 91 +++++++++++++++++++ .../FunctionalTests/MsQuicInteropTests.cs | 78 ++++++++++++++++ 3 files changed, 171 insertions(+), 8 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/MsQuicConfiguration.Cache.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs index 54b32184a2ce53..f8af682e619534 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs @@ -49,12 +49,6 @@ private static bool GetConfigurationCacheEnabled() public readonly List ApplicationProtocols; public readonly QUIC_ALLOWED_CIPHER_SUITE_FLAGS AllowedCipherSuites; - // Span-based access to settings for fast comparison and hashing. This - // avoids boxing and defensive copies due to accessing readonly struct - // fields. - [UnscopedRef] - private readonly ReadOnlySpan SettingsSpan => MemoryMarshal.AsBytes(new ReadOnlySpan(in Settings)); - public CacheKey(QUIC_SETTINGS settings, QUIC_CREDENTIAL_FLAGS flags, X509Certificate? certificate, ReadOnlyCollection? intermediates, List alpnProtocols, QUIC_ALLOWED_CIPHER_SUITE_FLAGS allowedCipherSuites) { CertificateThumbprints = certificate == null ? new List() : new List { certificate.GetCertHash() }; @@ -106,7 +100,7 @@ public bool Equals(CacheKey other) return Flags == other.Flags && - SettingsSpan.SequenceEqual(other.SettingsSpan) && + Settings.Equals(other.Settings) && AllowedCipherSuites == other.AllowedCipherSuites; } @@ -120,7 +114,7 @@ public override int GetHashCode() } hash.Add(Flags); - hash.AddBytes(SettingsSpan); + hash.Add(Settings); foreach (var protocol in ApplicationProtocols) { 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 00000000000000..1056b557c20a91 --- /dev/null +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Interop/QUIC_SETTINGS.IEquattable.cs @@ -0,0 +1,91 @@ +// 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 + { + 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/tests/FunctionalTests/MsQuicInteropTests.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicInteropTests.cs new file mode 100644 index 00000000000000..78f74e95cfaee9 --- /dev/null +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicInteropTests.cs @@ -0,0 +1,78 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +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 readonly ITestOutputHelper _output; + + public MsQuicInteropTests(ITestOutputHelper output) + { + _output = output; + } + + 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(); + + 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."); + } + } + } +} From 2440bf0291258b97fa383ace96668487c2a34a95 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Mon, 22 Apr 2024 17:20:33 +0200 Subject: [PATCH 8/9] More code review feedback --- .../src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs | 2 -- .../src/System/Net/Quic/Internal/ReceiveBuffers.cs | 2 +- .../src/System/Net/Quic/Interop/QUIC_SETTINGS.IEquattable.cs | 4 ++++ 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs index f8af682e619534..4fc86adfbd1565 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs @@ -2,13 +2,11 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Diagnostics; -using System.Diagnostics.CodeAnalysis; using System.Collections.Generic; using System.Collections.Concurrent; using System.Collections.ObjectModel; using System.Security.Authentication; using System.Net.Security; -using System.Runtime.InteropServices; using System.Security.Cryptography.X509Certificates; using System.Threading; using Microsoft.Quic; diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ReceiveBuffers.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ReceiveBuffers.cs index 1326251a11e977..93f78acc87f028 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ReceiveBuffers.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ReceiveBuffers.cs @@ -52,7 +52,7 @@ public int CopyFrom(ReadOnlySpan quicBuffers, int totalLength, bool int totalCopied = 0; for (int i = 0; i < quicBuffers.Length; ++i) { - ReadOnlySpan quicBuffer = quicBuffers[i].Span; + Span quicBuffer = quicBuffers[i].Span; if (totalLength < quicBuffer.Length) { quicBuffer = quicBuffer.Slice(0, totalLength); 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 index 1056b557c20a91..b24d3d926c2a4b 100644 --- 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 @@ -7,6 +7,10 @@ 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 From b994c4727ddb87a03cb91d27a6cc7216e67bec55 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Thu, 25 Apr 2024 13:44:36 +0200 Subject: [PATCH 9/9] Code review feedback --- .../Quic/Interop/QUIC_SETTINGS.IEquattable.cs | 167 +++++++++--------- .../FunctionalTests/MsQuicInteropTests.cs | 12 +- 2 files changed, 87 insertions(+), 92 deletions(-) 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 index b24d3d926c2a4b..3f008c74136d1c 100644 --- 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 @@ -3,93 +3,92 @@ using System; -namespace Microsoft.Quic +namespace Microsoft.Quic; + +internal partial struct QUIC_SETTINGS : System.IEquatable { - 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. + // 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 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 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); - } + public override readonly bool Equals(object? obj) + { + return obj is QUIC_SETTINGS other && Equals(other); } } diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicInteropTests.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicInteropTests.cs index 78f74e95cfaee9..5ffd5b53510d24 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicInteropTests.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicInteropTests.cs @@ -1,6 +1,7 @@ // 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; @@ -16,13 +17,6 @@ namespace System.Net.Quic.Tests { public class MsQuicInteropTests { - private readonly ITestOutputHelper _output; - - public MsQuicInteropTests(ITestOutputHelper output) - { - _output = output; - } - private static MemberInfo[] GetMembers() { var members = typeof(T).FindMembers(MemberTypes.Field | MemberTypes.Property, BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.Public, (mi, _) => @@ -55,12 +49,14 @@ private static void ResetMember(MemberInfo member, object instance) } } - [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