-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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 8 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,95 @@ | ||||
// 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 | ||||
{ | ||||
rzikm marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
internal partial struct QUIC_SETTINGS : System.IEquatable<QUIC_SETTINGS> | ||||
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. Why is this needed? Is QUIC_SETTINGS being put into a Dictionary somewhere or something? 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. QUIC_SETTINGS is used as part of the cache key for MsQuicConfigurationHandle runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs Line 101 in e12e2fa
rzikm marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
{ | ||||
// 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,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<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(); | ||
|
||
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.