Repository navigation
fix TCP network info on OSX and add tests for that area - #32830
Conversation
| } | ||
|
|
||
| TcpConnectionInformation[] connectionInformations = new TcpConnectionInformation[infoCount]; | ||
| int skip = 0; |
There was a problem hiding this comment.
Why manage this as skip rather than managing the number of items copied, i.e. have a:
int nextResultIndex = 0;that you then just increment as part of:
connectionInformations[nextResultIndex++] = ...;and then you know if you need to resize if nextResultIndex != connectionInformations.Length at the end. I realize it's similar, but it seems cleaner, and you only need to increment in one place.
| } | ||
| public unsafe override TcpConnectionInformation[] GetActiveTcpConnections() | ||
| { | ||
| return GetTcpConnections(false); |
There was a problem hiding this comment.
Nit:
return GetTcpConnections(listeners: false);
| { | ||
| TcpConnectionInformation[] allConnections = GetActiveTcpConnections(); | ||
| return allConnections.Where(tci => tci.State != TcpState.Listen).Select(tci => tci.RemoteEndPoint).ToArray(); | ||
| TcpConnectionInformation[] allConnections = GetTcpConnections(true); |
| { | ||
| // OSX does not provide separated TCP-IPv4 and TCP-IPv6 stats. | ||
| throw new PlatformNotSupportedException(SR.net_InformationUnavailableOnPlatform); | ||
| return new OsxTcpStatistics(); |
There was a problem hiding this comment.
Maybe I missed it... why was this throwing previously if we already had the implementation for it?
There was a problem hiding this comment.
OSX (and FreeBSD) does not track transport (TCP/UDP) statistic per network protocol (IPv4/6)
For UDP we return same values for both IPv4 and IPv6. For TCP we did return common stats for IPv6 but not for IPv4. That popped when I added basic tests and it looked wrong.
This change makes it consistent even if not 100% correct. Alternatively we can throw for all four cases but that does not seems productive.
| TcpStatistics v4TcpSTat = gp.GetTcpIPv4Statistics(); | ||
| TcpStatistics v6TcpStat = gp.GetTcpIPv6Statistics(); | ||
| UdpStatistics v4UdpStat = gp.GetUdpIPv4Statistics(); | ||
| UdpStatistics v6UdpStat = gp.GetUdpIPv6Statistics(); |
There was a problem hiding this comment.
You could at least Assert.NotNull for all of these, access properties that won't throw, etc.
| foreach (TcpConnectionInformation ti in tcpCconnections) | ||
| { | ||
| if (ti.LocalEndPoint.Equals(client.LocalEndPoint) && ti.RemoteEndPoint.Equals(client.RemoteEndPoint) && | ||
| (ti.State == TcpState.Established)) |
| { | ||
| server.Bind(new IPEndPoint(address, 0)); | ||
| server.Listen(1); | ||
| _log.WriteLine("listening on {0}", server.LocalEndPoint); |
There was a problem hiding this comment.
Nit: Use interpolated string format instead?
_log.WriteLine($"listening on {server.LocalEndPoint}");There was a problem hiding this comment.
I was commented on before when using interpolated. Is there some advantage or is that just personal or team preference?
There was a problem hiding this comment.
In product src it's more questionable, unless it's in some debug-only or trace-only code, as it can lead to extra expenses. But it's also often more readable/maintainable, so in test code it can be preferred.
|
@dotnet-bot test Outerloop Windows x64 Debug Build |
| <Compile Include="IPInterfacePropertiesTest_Linux.cs" /> | ||
| <Compile Include="IPInterfacePropertiesTest_OSX.cs" /> | ||
| <Compile Include="IPInterfacePropertiesTest_Windows.cs" /> | ||
| <Compile Include="IPGlobalPropertiesTest.cs" /> |
There was a problem hiding this comment.
nit: this line should come before IPInterfacePropertiesTest_Linux.cs" to keep alphabetical ordering.
|
new tests are failing on some platforms. I'll investigate. |
|
@dotnet-bot test Outerloop Windows x64 Debug Build |
|
It looks like we have parsing bug for /proc/net/tcp6. Tracked in #33257 |
|
@dotnet-bot test Outerloop Windows x64 Debug Build |
|
Fedora.28 test failures are unrelated. |
* fix TCP netowrkinfo on OSX and add tests for that area * feedback from reviews * feedback from reviews * disable tests on Linux & Windows * fix WriteLine format
…x#32830) * fix TCP netowrkinfo on OSX and add tests for that area * feedback from reviews * feedback from reviews * disable tests on Linux & Windows * fix WriteLine format Commit migrated from dotnet/corefx@ee0f9aa
4 participants
fixes #32727, related to #32816
The root cause for not finding expected port is fact that kernel returns port in network byte order.
The other problem was that GetActiveTcpListeners() had wrong condition with "tci.State != TcpState.Listen" and got everything but listeners.
Documentation for GetActiveTcpConnections() states that it returns all TCP connections in state other than Listen. That make sense as the listener is really not a connection.
Since there were no tests for the touched functions I added few tests.