-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Avoid some defensive copies on readonly structs in System.Net.Quic #101133
Changes from all commits
1c7b419
4398150
45bbf45
8f5ae6d
c69ba96
7821590
1df80d9
2440bf0
b994c47
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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<QUIC_SETTINGS> | ||
{ | ||
// Because QUIC_SETTINGS may contain gaps due to layout/alignment of individual | ||
// fields, we implement IEquatable<QUIC_SETTINGS> 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); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,7 +20,7 @@ namespace Microsoft.Quic | |
{ | ||
internal unsafe partial struct QUIC_BUFFER | ||
{ | ||
public Span<byte> Span => new(Buffer, (int)Length); | ||
public readonly Span<byte> Span => new(Buffer, (int)Length); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't remember exactly, but if this is generated code from MsQuic, it should be fixed there as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. right, I thought this was added manually by us (since the QUIC_BUFFER definition is in another auto-generated file). I will follow up to make the change in their repo as well. |
||
} | ||
|
||
internal partial class MsQuic | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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<T>() | ||
{ | ||
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<QUIC_SETTINGS>), typeof(QUIC_SETTINGS).GetInterfaces()); | ||
|
||
var settingsSpan = MemoryMarshal.AsBytes(new Span<QUIC_SETTINGS>(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<QUIC_SETTINGS>()) | ||
{ | ||
// copy and box the instance because reflection methods take a reference type arg | ||
object boxed = settings; | ||
rzikm marked this conversation as resolved.
Show resolved
Hide resolved
|
||
ResetMember(member, boxed); | ||
Assert.False(settings.Equals((QUIC_SETTINGS)boxed), $"Member {member.Name} is not compared."); | ||
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unintended leftover change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one should stay, otherwise the
.Free()
is executed on a defensive copyruntime/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicSafeHandle.cs
Line 133 in 1c7b419
It probably is not a big deal since
ReleaseHandle
runs exactly once, but we still make sure to cleanup dangling pointers.