Uh oh!
There was an error while loading. Please reload this page.
Include IP information in socket.ConnectAsync(endPoint) exceptions - #37685
Include IP information in socket.ConnectAsync(endPoint) exceptions#37685antonfirsov wants to merge 7 commits into
Conversation
ghost
commented
Jun 10, 2020
Tagging subscribers to this area: @dotnet/ncl |
antonfirsov
commented
Jun 10, 2020
/azp run runtime-libraries outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| } | ||
| } | ||
| [OuterLoop("Slow on Windows")] |
There was a problem hiding this comment.
I did something like
using(Socketlistener=newSocket(AddressFamily.InterNetwork,SocketType.Stream,ProtocolType.Tcp)){listener.Bind(newIPEndPoint(IPAddress.Loopback,10000));Console.WriteLine("Connecting {0}",DateTime.Now);varclientSocket=newSocket(AddressFamily.InterNetwork,SocketType.Stream,ProtocolType.Tcp);try{awaitclientSocket.ConnectAsync(listener.LocalEndPoint);}catch(Exceptionex){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.
| 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?
stephentoub
commented
Sep 15, 2020
@antonfirsov, are you still working on this? |
antonfirsov
commented
Sep 15, 2020
@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. |
stephentoub
commented
Sep 15, 2020
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. |
antonfirsov
commented
Nov 3, 2020
Closing, since the code has been refactored into |
geoffkizer
commented
Nov 3, 2020
If we think this is something we should address, let's make sure we have an issue to track it. |
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)).