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

Fix & re-enable DualMode socket tests #31923

Closed
wants to merge 15 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/libraries/Common/tests/Common.Tests.csproj
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<Project Sdk="Microsoft.NET.Sdk">
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<CoverageSupported>false</CoverageSupported>
Expand Down Expand Up @@ -108,13 +108,17 @@
<Compile Include="$(CoreLibSharedDir)System\PasteArguments.cs">
<Link>System\PasteArguments.cs</Link>
</Compile>
<Compile Include="System\Net\Configuration.cs" />
<Compile Include="System\Net\Configuration.Sockets.cs" />
<Compile Include="System\Net\Sockets\TestPortPool.cs" />
<Compile Include="Tests\Interop\cgroupsTests.cs" />
<Compile Include="Tests\Interop\procfsTests.cs" />
<Compile Include="Tests\System\CharArrayHelpersTests.cs" />
<Compile Include="Tests\System\IO\PathInternal.Tests.cs" />
<Compile Include="Tests\System\IO\StringParserTests.cs" />
<Compile Include="Tests\System\MarvinTests.cs" />
<Compile Include="Tests\System\Net\HttpDateParserTests.cs" />
<Compile Include="Tests\System\Net\TestPortPoolTests.cs" />
<Compile Include="Tests\System\PasteArgumentsTests.cs" />
<Compile Include="Tests\System\Security\IdentityHelperTests.cs" />
<Compile Include="Tests\System\Text\SimpleRegexTests.cs" />
Expand Down
16 changes: 16 additions & 0 deletions src/libraries/Common/tests/System/Net/Configuration.Sockets.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Threading;

namespace System.Net.Test.Common
{
public static partial class Configuration
Expand All @@ -11,6 +13,20 @@ public static partial class Sockets
public static Uri SocketServer => GetUriValue("DOTNET_TEST_NET_SOCKETS_SERVERURI", new Uri("http://" + DefaultAzureServer));

public static string InvalidHost => GetValue("DOTNET_TEST_NET_SOCKETS_INVALIDSERVER", "notahostname.invalid.corp.microsoft.com");

private static Lazy<(int Min, int Max)> s_portPoolRangeLazy = new Lazy<(int Min, int Max)>(() =>
{
string configString = GetValue("DOTNET_TEST_NET_SOCKETS_PORTPOOLRANGE", "17000 22000");
string[] portRange = configString.Split(' ', StringSplitOptions.RemoveEmptyEntries);
int minPort = int.Parse(portRange[0].Trim());
int maxPort = int.Parse(portRange[1].Trim());
return (minPort, maxPort);
}, LazyThreadSafetyMode.PublicationOnly);

/// <summary>
/// Min: inclusive, Max: exclusive.
/// </summary>
public static (int Min, int Max) TestPoolPortRange => s_portPoolRangeLazy.Value;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,12 @@ public static int BindToAnonymousPort(this Socket socket, IPAddress address)
return ((IPEndPoint)socket.LocalEndPoint).Port;
}

// Binds to an IP address and a port assigned by TestPortPool.
public static PortLease BindToPoolPort(this Socket socket, IPAddress address)
{
return TestPortPool.RentPortAndBindSocket(socket, address);
}

// Binds to an OS-assigned port.
public static TcpListener CreateAndStartTcpListenerOnAnonymousPort(out int port)
{
Expand Down
230 changes: 230 additions & 0 deletions src/libraries/Common/tests/System/Net/Sockets/TestPortPool.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,230 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Collections.Concurrent;
using System.Diagnostics;
using System.Globalization;
using System.Net.Security;
using System.Net.Test.Common;
using System.Runtime.CompilerServices;
using System.Threading;
using Microsoft.DotNet.RemoteExecutor;

namespace System.Net.Sockets.Tests
{
internal readonly struct PortLease : IDisposable
Copy link
Member

@tmds tmds Feb 17, 2020

Choose a reason for hiding this comment

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

Instead of managing the ports with PortLease, could we incrementally try ports from a range? If the range is large enough for all tests that need a unique port nr, we don't need to deal with returning (and possible issues by re-using port nrs).
There can be BindToTcpPoolPort/BindToUdpPoolPort methods which are similar to BindToAnonymousPort. (TCP and UDP ports are distinct).

Copy link
Member Author

Choose a reason for hiding this comment

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

That would mean creating and destroying temporary sockets, since we need to probe both IPV4 and IPV6.

I was afraid of possible side effects which are unknown to me, and rent/return seemed less risky. If you say there are none, I believe you :)

Copy link
Member

Choose a reason for hiding this comment

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

That would mean creating and destroying temporary sockets, since we need to probe both IPV4 and IPV6.

Why is probing needed?
If we incrementally use ports from the range, and not re-use any, do we still expect issues?

Copy link
Member Author

Choose a reason for hiding this comment

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

could we incrementally try ports from a range

I thought you meant probing by "trying" here. It seemed logical to probe both protocols to make sure we are safe.

But you are right, we don't need probing at all, if the port range is big enough compared to the number of tests. In that case I would just return the next port number, and let the test fail in the theoretical case of a collision (which we don't expect to happen in practice.)

I will remove the returning logic, and it's backing dictionary. Seems like a reasonable simplification, thanks for the suggestion!

{
public int Port { get; }

internal PortLease(int port) => Port = port;

public void Dispose() => TestPortPool.Return(this);
}

internal class TestPortPoolExhaustedException : Exception
{
public TestPortPoolExhaustedException()
: base($"TestPortPool failed to find an available port after {TestPortPool.ThrowExhaustedAfter} attempts")
{
}
}

/// <summary>
/// Distributes unique ports from the range defined by <see cref="Configuration.Sockets.TestPoolPortRange"/>
/// Useful in socket testing scenarios, where port collisions across protocols are not acceptable.
/// This kind of uniqueness is not guaranteed when binding to OS ephemeral ports, and might lead to issues on Unix.
/// For more information see:
/// https://github.com/dotnet/runtime/issues/19162#issuecomment-523195762
/// </summary>
internal static class TestPortPool
{
internal const int ThrowExhaustedAfter = 10;

public static PortRange ConfiguredPortRange = new PortRange(
System.Net.Test.Common.Configuration.Sockets.TestPoolPortRange.Min,
System.Net.Test.Common.Configuration.Sockets.TestPoolPortRange.Max);


private static readonly ConcurrentDictionary<int, int> s_usedPorts = GetAllPortsUsedBySystem();
private static int s_counter = int.MinValue;

public static PortLease RentPort()
{
for (int i = 0; i < ThrowExhaustedAfter; i++)
{
// Although race may occur theoretically because the following code block is not atomic,
// it requires the s_counter to move at least PortRangeLength steps between Increment and TryAdd,
// which is very unlikely considering the actual port range and the low number of tests utilizing TestPortPool
long portLong = (long)Interlocked.Increment(ref s_counter) - int.MinValue;
portLong = (portLong % ConfiguredPortRange.Length) + ConfiguredPortRange.Min;
int port = (int)portLong;

if (s_usedPorts.TryAdd(port, 0))
{
return new PortLease(port);
}
}

throw new TestPortPoolExhaustedException();
}

public static void Return(PortLease portLease)
{
s_usedPorts.TryRemove(portLease.Port, out _);
}

public static PortLease RentPortAndBindSocket(Socket socket, IPAddress address)
{
PortLease lease = RentPort();
try
{
socket.Bind(new IPEndPoint(address, lease.Port));
return lease;
}
catch (SocketException)
{
lease.Dispose();
throw;
}
}

// Exclude ports which are unavailable at initialization time
private static ConcurrentDictionary<int, int> GetAllPortsUsedBySystem()
Copy link
Member

Choose a reason for hiding this comment

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

What if any of the ports in the allowed range are bound by other tests and/or processes after the static initializer runs?

Kestrel tests used to check if certain ports were available during xunit test discovery, but would find that the port would become unavailable by the time the test ran. Now, for tests that want to verify binding to a specific port, we test if the port is available at the start of each test, and skip the test otherwise. This improved reliability, but even that logic is still sometimes flaky, and that's despite running those tests serially in their own test group so they shouldn't be running in parallel with other tests.

Can we have RentPort() redo all the IsPortUsed() checks at the last possible moment in order to double check nothing else has taken the port? I think RentPortAndBindSocket should also retry on bind failures instead of throw.

Copy link
Member Author

@antonfirsov antonfirsov Feb 8, 2020

Choose a reason for hiding this comment

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

The configured port range for TestPortPool does not overlap with the OS ephemeral ports. I think if we make sure that this range s dedicated to the tests using TestPortPool, and no other tests try to arbitrarily bind to it's ports, we don't need to be concerned about this in System.Net.Sockets.Tests. We may add a README.MD, to make sure no one will violate this in the future. Or am I missing something?

I wanted to avoid the creation + destroyal of temporary socket handles per rental, because I was afraid of possible esoteric OS side effects I'm not aware of. We need to create & destroy 4 sockets, according to the "unique-across-protocols" contract guaranteed by TestPortPool. This might be a false concern though.

{
IPEndPoint ep4 = new IPEndPoint(IPAddress.Loopback, 0);
IPEndPoint ep6 = new IPEndPoint(IPAddress.IPv6Loopback, 0);

bool IsPortUsed(int port,
AddressFamily addressFamily,
SocketType socketType,
ProtocolType protocolType)
{
try
{
IPEndPoint ep = addressFamily == AddressFamily.InterNetwork ? ep4 : ep6;
ep.Port = port;
using Socket socket = new Socket(addressFamily, socketType, protocolType);
socket.Bind(ep);
return false;
}
catch (SocketException)
{
return true;
}
}

ConcurrentDictionary<int, int> result = new ConcurrentDictionary<int, int>();

for (int port = ConfiguredPortRange.Min; port < ConfiguredPortRange.Max; port++)
{
if (IsPortUsed(port, AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp) ||
IsPortUsed(port, AddressFamily.InterNetwork, SocketType.Dgram, ProtocolType.Udp) ||
IsPortUsed(port, AddressFamily.InterNetworkV6, SocketType.Stream, ProtocolType.Tcp) ||
IsPortUsed(port, AddressFamily.InterNetworkV6, SocketType.Dgram, ProtocolType.Udp))
{
result.TryAdd(port, 0);
}
}

return result;
}
}

internal readonly struct PortRange
{
public int Min { get; }
public int Max { get; }

public int Length => Max - Min;

public PortRange(int min, int max)
{
Min = min;
Max = max;
}

public override string ToString() => $"({Min} .. {Max})";

public static bool AreOverlappingRanges(in PortRange a, in PortRange b)
{
return a.Min < b.Min ? a.Max > b.Min : b.Max > a.Min;
}

public static PortRange GetDefaultOsDynamicPortRange()
{
if (PlatformDetection.IsWindows)
{
// For TestPortPool functionality, we need to take the intersection of 4 intervals:
PortRange ipv4Tcp = ParseCmdletOutputWindows(RunCmldet("netsh", "int ipv4 show dynamicport tcp"));
PortRange ipv4Udp = ParseCmdletOutputWindows(RunCmldet("netsh", "int ipv4 show dynamicport udp"));
PortRange ipv6Tcp = ParseCmdletOutputWindows(RunCmldet("netsh", "int ipv6 show dynamicport tcp"));
PortRange ipv6Udp = ParseCmdletOutputWindows(RunCmldet("netsh", "int ipv6 show dynamicport udp"));

int min = Math.Max(ipv4Tcp.Min, Math.Max(ipv4Udp.Min, Math.Max(ipv6Tcp.Min, ipv6Udp.Min)));
int max = Math.Min(ipv4Tcp.Max, Math.Min(ipv4Udp.Max, Math.Min(ipv6Tcp.Max, ipv6Udp.Max)));
return new PortRange(min, max);
}

if (PlatformDetection.IsOSX)
{
return ParseCmdletOutputMac(RunCmldet("sysctl", "net.inet.ip.portrange.first net.inet.ip.portrange.last"));
}

// Handle the Linux case otherwise.
// We may need to introduce additional branches as we extend our CI support.
return ParseCmdletOutputLinux(RunCmldet("cat", "/proc/sys/net/ipv4/ip_local_port_range"));
}

internal static PortRange ParseCmdletOutputWindows(string cmdOutput)
{
PortRange temp = ParseCmdletOutputMac(cmdOutput);

// On Windows, second number is 'Number of Ports'
return new PortRange(temp.Min, temp.Min + temp.Max);
}

internal static PortRange ParseCmdletOutputLinux(string cmdOutput)
{
ReadOnlySpan<char> span = cmdOutput.AsSpan();
int firstWhiteSpace = span.IndexOf('\t');
if (firstWhiteSpace < 0) firstWhiteSpace = span.IndexOf(' ');
ReadOnlySpan<char> left = span.Slice(0, firstWhiteSpace).Trim();
ReadOnlySpan<char> right = span.Slice(firstWhiteSpace).Trim();
return new PortRange(int.Parse(left), int.Parse(right));
}

internal static PortRange ParseCmdletOutputMac(string cmdOutput)
{
int semicolon1 = cmdOutput.IndexOf(':', 0);
int eol1 = cmdOutput.IndexOf('\n', semicolon1);
int semicolon2 = cmdOutput.IndexOf(':', eol1);
int eol2 = cmdOutput.IndexOf('\n', semicolon2);
if (eol2 < 0) eol2 = cmdOutput.Length;

int start = ParseImpl(cmdOutput, semicolon1 + 1, eol1);
int end = ParseImpl(cmdOutput, semicolon2 + 1, eol2);
return new PortRange(start, end);
}

private static string RunCmldet(string cmdlet, string args)
{
ProcessStartInfo psi = new ProcessStartInfo()
{
FileName = cmdlet,
Arguments = args,
RedirectStandardOutput = true
};

using Process process = Process.Start(psi);
process.WaitForExit(10000);
return process.StandardOutput.ReadToEnd();
}

private static int ParseImpl(string cmdOutput, int start, int end)
{
ReadOnlySpan<char> span = cmdOutput.AsSpan(start, end - start).Trim();
return int.Parse(span);
}
}
}
Loading