-
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
Fix & re-enable DualMode socket tests #31923
Changes from 6 commits
477acf8
05a3d83
55083d2
62f8817
31500a4
844e1e0
88c6d4e
fa36bef
d3233c9
d063f37
f421299
6ab1791
4ae280a
2d97ddb
de666af
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,134 @@ | ||
// 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.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 | ||
{ | ||
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; | ||
|
||
private static readonly int MinPort = | ||
System.Net.Test.Common.Configuration.Sockets.TestPoolPortRange.Min; | ||
private static readonly int MaxPort = | ||
System.Net.Test.Common.Configuration.Sockets.TestPoolPortRange.Max; | ||
private static readonly int PortRangeLength = MaxPort - MinPort; | ||
|
||
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 % PortRangeLength) + MinPort; | ||
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() | ||
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. 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. 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. The configured port range for 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 |
||
{ | ||
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 = MinPort; port < MaxPort; 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; | ||
} | ||
} | ||
} |
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.
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 toBindToAnonymousPort
. (TCP and UDP ports are distinct).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.
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 :)
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.
Why is probing needed?
If we incrementally use ports from the range, and not re-use any, do we still expect issues?
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.
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!