Skip to content
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

Merged
merged 9 commits into from
Apr 25, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -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.
/// </summary>
private readonly GCHandle _context;
private GCHandle _context;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unintended leftover change?

Copy link
Member Author

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 copy

It probably is not a big deal since ReleaseHandle runs exactly once, but we still make sure to cleanup dangling pointers.


/// <summary>
/// Optional parent safe handle, used to increment/decrement reference count with the lifetime of this instance.
Expand Down
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
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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
Expand Down
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.");
}
}
}
}
Loading