Include IP information in socket.ConnectAsync(endPoint) exceptions #37685
Conversation
|
Tagging subscribers to this area: @dotnet/ncl |
|
/azp run runtime-libraries outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| @@ -82,6 +82,20 @@ public async Task Connect_MultipleIPAddresses_Success(IPAddress listenAt) | |||
| } | |||
| } | |||
|
|
|||
| [OuterLoop("Slow on Windows")] | |||
There was a problem hiding this comment.
I did something like
using (Socket listener = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp))
{
listener.Bind(new IPEndPoint(IPAddress.Loopback, 10000));
Console.WriteLine("Connecting {0}", DateTime.Now);
var clientSocket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
try
{
await clientSocket.ConnectAsync(listener.LocalEndPoint);
}
catch(Exception ex)
{
Console.WriteLine("Connect failed {0} {1}", ex.Message, DateTime.Now);
}
}and I get this on Windows 10
Connecting 6/19/2020 10:48:07 AM
Connect failed No connection could be made because the target machine actively refused it. 6/19/2020 10:48:09 AM
it seems like it takes 2s to fail. That does not seems to bad.
| @@ -204,5 +218,8 @@ public sealed class ConnectTask : Connect<SocketHelperTask> | |||
| public sealed class ConnectEap : Connect<SocketHelperEap> | |||
| { | |||
| public ConnectEap(ITestOutputHelper output) : base(output) {} | |||
|
|
|||
| // We should skip this since the exception creation logic is defined in SocketHelperEap. | |||
| public override Task Connect_WhenFails_ThrowsSocketExceptionWithIPEndpointInfo(bool useDns) => Task.CompletedTask; | |||
There was a problem hiding this comment.
Would it make sense to simply put the test function here?
| EndPoint badEndpoint = useDns ? (EndPoint)new DnsEndPoint("localhost", 288) : new IPEndPoint(IPAddress.Loopback, 288); | ||
| using Socket client = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp); | ||
| SocketException ex = await Assert.ThrowsAnyAsync<SocketException>(() => ConnectAsync(client, badEndpoint)); | ||
| Assert.Contains("127.0.0.1:288", ex.Message); |
There was a problem hiding this comment.
It may be safer to bind on anonymous port like the example above. I think that would fit better existing test pattern.
| if (!attemptSocket.ConnectAsync(args)) | ||
| bool pending = attemptSocket.ConnectAsync(args); | ||
| // Copy the socket address so we can use it in exception message. | ||
| _userArgs!._socketAddress = _internalArgs!._socketAddress; |
There was a problem hiding this comment.
Is there reason why we don't use user supplied data? If we were connecting to name resolving to multiple addresses this would log just one, right?
|
@antonfirsov, are you still working on this? |
|
@stephentoub not right ATM, but I think the change in this PR still makes sense since the exception messages in sync and async connect variants do not match. Unless we state it explicitly that we don't want to spend team capacity for reviews here, I'd like to finish it since it's quite a small change, and comments were only concerned about test code so far. |
|
Ok, it's just been open with no progress for three months now. If all that remains to merge it is someone to review it, let's make that happen and get it merged (though I see @wfurt did review it and has feedback that hasn't been addressed), otherwise let's close it. Thanks. |
|
Closing, since the code has been refactored into |
|
If we think this is something we should address, let's make sure we have an issue to track it. |
ghost
locked as resolved and limited conversation to collaborators
Unlike the synchronous variant, the exception message of
SocketTaskExtensions.ConnectAsync(...)failures did not include the IP+Port information.Related to #1326, but does not solve the issue for
SocketsHttpHandler. It could be considered as a step towards the long-term solution however (see #1326 (comment)).