Skip to content

Include IP information in socket.ConnectAsync(endPoint) exceptions #37685

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.

Sign up for GitHub

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

Closed
antonfirsov wants to merge 7 commits into from
Closed

Include IP information in socket.ConnectAsync(endPoint) exceptions #37685

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b223a02
failing test
antonfirsov Jun 10, 2020
cfa3dee
include IP/port information in ConnectAsync exception messages
antonfirsov Jun 10, 2020
aef67a5
improve test
antonfirsov Jun 10, 2020
4d14f60
relax assertion in Connect_AfterDisconnect_Fails
antonfirsov Jun 10, 2020
b41023f
add test for DnsEndpoint
antonfirsov Jun 10, 2020
886222b
handle DnsEndPoint case
antonfirsov Jun 10, 2020
9e9cedf
simpler assertion
antonfirsov Jun 10, 2020
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
5 changes: 4 additions & 1 deletion src/libraries/System.Net.Sockets/src/System/Net/Sockets/MultipleConnectAsync.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,10 @@ private void InternalConnectCallback(object? sender, SocketAsyncEventArgs args)

SocketAsyncEventArgs args = _internalArgs!;
args.RemoteEndPoint = new IPEndPoint(attemptAddress, _endPoint!.Port);
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;
Copy link
Member

@wfurt wfurt Jun 19, 2020

Choose a reason for hiding this comment

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

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?

if (!pending)
{
InternalConnectCallback(null, args);
}
Expand Down
6 changes: 5 additions & 1 deletion src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
using System.Net.Internals;
using System.Runtime.CompilerServices;
using System.Runtime.ExceptionServices;
using System.Runtime.InteropServices;
Expand Down Expand Up @@ -1140,7 +1141,10 @@ private void ThrowException(SocketError error, CancellationToken cancellationTok

private Exception CreateException(SocketError error, bool forAsyncThrow = true)
{
Exception e = new SocketException((int)error);
// When available, include the IP endpoint information in connection exceptions:
Exception e = LastOperation == SocketAsyncOperation.Connect && _socketAddress != null ?
SocketExceptionFactory.CreateSocketException((int)error, _socketAddress.GetIPEndPoint()) :
new SocketException((int)error);

if (forAsyncThrow)
{
Expand Down
19 changes: 18 additions & 1 deletion src/libraries/System.Net.Sockets/tests/FunctionalTests/Connect.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,20 @@ public async Task Connect_MultipleIPAddresses_Success(IPAddress listenAt)
}
}

[OuterLoop("Slow on Windows")]
Copy link
Member

@wfurt wfurt Jun 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

[Theory]
[InlineData(false)]
[InlineData(true)]
public virtual async Task Connect_WhenFails_ThrowsSocketExceptionWithIPEndpointInfo(bool useDns)
{
// Port 288 is unassigned:
// https://www.iana.org/assignments/service-names-port-numbers/service-names-port-numbers.txt
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);
Copy link
Member

@wfurt wfurt Jun 19, 2020

Choose a reason for hiding this comment

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

It may be safer to bind on anonymous port like the example above. I think that would fit better existing test pattern.

}

[Fact]
public async Task Connect_OnConnectedSocket_Fails()
{
Expand Down Expand Up @@ -115,7 +129,7 @@ public async Task Connect_AfterDisconnect_Fails()
}
else
{
SocketException se = await Assert.ThrowsAsync<SocketException>(() => ConnectAsync(client, new IPEndPoint(IPAddress.Loopback, port)));
SocketException se = await Assert.ThrowsAnyAsync<SocketException>(() => ConnectAsync(client, new IPEndPoint(IPAddress.Loopback, port)));
Assert.Equal(SocketError.IsConnected, se.SocketErrorCode);
}
}
Expand Down Expand Up @@ -204,5 +218,8 @@ public ConnectTask(ITestOutputHelper output) : base(output) {}
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;
Copy link
Member

@wfurt wfurt Jun 19, 2020

Choose a reason for hiding this comment

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

Would it make sense to simply put the test function here?

}
}