Skip to content

PoC: retire internal SocketExceptionFactory #68918

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

Jump to bottom
Closed
wfurt wants to merge 2 commits into from
Closed

PoC: retire internal SocketExceptionFactory #68918

wfurt wants to merge 2 commits into from

Conversation

@wfurt
Copy link
Member

@wfurt wfurt commented May 5, 2022

This fixes #37150. I would like to get some feedback before dragging this to API review.

There are actually two separate places where the internals exception would show up. Let's consider

           try {
                Socket s1 = new Socket(SocketType.Stream, ProtocolType.Tcp);
                s1.Connect(IPEndPoint.Parse("127.0.0.1:54321"));
            }
            catch (Exception ex)
            {
                Console.WriteLine(ex);
            }

            try {
                Socket s2 = new Socket(SocketType.Stream, ProtocolType.Tcp);
                s2.ConnectAsync(IPEndPoint.Parse("127.0.0.1:54321")).GetAwaiter().GetResult();
            }
            catch (Exception ex)
            {
                Console.WriteLine(ex);
            }

            var foo = Dns.GetHostAddresses(Guid.NewGuid().ToString("N"));

it would currently produce

System.Net.Internals.SocketExceptionFactory+ExtendedSocketException (111): Connection refused [::ffff:127.0.0.1]:54321
   at System.Net.Sockets.Socket.DoConnect(EndPoint endPointSnapshot, SocketAddress socketAddress)
   at System.Net.Sockets.Socket.Connect(EndPoint remoteEP)
   at Program.Main(String[] args) in /home/furt/projects/exception/Program.cs:line 12

System.Net.Sockets.SocketException (111): Connection refused
   at System.Net.Sockets.Socket.AwaitableSocketAsyncEventArgs.ThrowException(SocketError error, CancellationToken cancellationToken)
   at System.Net.Sockets.Socket.AwaitableSocketAsyncEventArgs.System.Threading.Tasks.Sources.IValueTaskSource.GetResult(Int16 token)
   at System.Threading.Tasks.ValueTask.ValueTaskSourceAsTask.<>c.<.cctor>b__4_0(Object state)
--- End of stack trace from previous location ---
   at Program.Main(String[] args) in /home/furt/projects/exception/Program.cs:line 21

Unhandled exception. System.Net.Internals.SocketExceptionFactory+ExtendedSocketException (00000001, 11): Resource temporarily unavailable
   at System.Net.Dns.GetHostEntryOrAddressesCore(String hostName, Boolean justAddresses, AddressFamily addressFamily, ValueStopwatch stopwatch)
   at System.Net.Dns.GetHostAddresses(String hostNameOrAddress, AddressFamily family)
   at Program.Main(String[] args) in /home/furt/projects/exception/Program.cs:line 28
   at Program.<Main>(String[] args)

In Sockets, this differ for sync vs async. And we get internal exception for DNS errors.

The DNS is easy to fix IMHO and it does not need API change. I can bring that forward if we want to. (fixes issue I hit with #67951)
For the socket exception I was wondering if we should take string instead of EndPoint as we can pass more information to it in the future. However, RemoteEndPoint.ToString() would always allocate but I'm not sure if that matters that much for failures. If We are happy with EndPoint, I can create API proposal and drag it through review.

With this change we get

System.Net.Sockets.SocketException (111): Connection refused [::ffff:127.0.0.1]:54321
   at System.Net.Sockets.Socket.DoConnect(EndPoint endPointSnapshot, SocketAddress socketAddress) in /home/furt/github/wfurt-runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs:line 3116
   at System.Net.Sockets.Socket.Connect(EndPoint remoteEP) in /home/furt/github/wfurt-runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs:line 839
   at Program.Main(String[] args) in /home/furt/projects/exception/Program.cs:line 12

System.Net.Sockets.SocketException (111): Connection refused 127.0.0.1:54321
   at System.Net.Sockets.Socket.AwaitableSocketAsyncEventArgs.ThrowException(SocketError error, CancellationToken cancellationToken) in /home/furt/github/wfurt-runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs:line 1451
   at System.Net.Sockets.Socket.AwaitableSocketAsyncEventArgs.System.Threading.Tasks.Sources.IValueTaskSource.GetResult(Int16 token) in /home/furt/github/wfurt-runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs:line 1365
   at System.Threading.Tasks.ValueTask.ValueTaskSourceAsTask.<>c.<.cctor>b__4_0(Object state) in /home/furt/github/wfurt-runtime/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/ValueTask.cs:line 250
--- End of stack trace from previous location ---
   at Program.Main(String[] args) in /home/furt/projects/exception/Program.cs:line 21

Unhandled exception. System.Net.Sockets.SocketException (00000001, 11): Resource temporarily unavailable
   at System.Net.Dns.GetHostEntryOrAddressesCore(String hostName, Boolean justAddresses, AddressFamily addressFamily, Int64 startingTimestamp) in /home/furt/github/wfurt-runtime/src/libraries/System.Net.NameResolution/src/System/Net/Dns.cs:line 393
   at System.Net.Dns.GetHostAddressesCore(String hostName, AddressFamily addressFamily, Int64 startingTimestamp) in /home/furt/github/wfurt-runtime/src/libraries/System.Net.NameResolution/src/System/Net/Dns.cs:line 374
   at System.Net.Dns.GetHostAddresses(String hostNameOrAddress, AddressFamily family) in /home/furt/github/wfurt-runtime/src/libraries/System.Net.NameResolution/src/System/Net/Dns.cs:line 208
   at System.Net.Dns.GetHostAddresses(String hostNameOrAddress) in /home/furt/github/wfurt-runtime/src/libraries/System.Net.NameResolution/src/System/Net/Dns.cs:line 180
   at Program.Main(String[] args) in /home/furt/projects/exception/Program.cs:line 28
   at Program.<Main>(String[] args)

so both sync & async Connect has same exception type with details and DNS failure is SocketException with same details as before.

@wfurt wfurt requested review from antonfirsov and stephentoub May 5, 2022 17:20
@ghost
Copy link

ghost commented May 5, 2022

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost ghost assigned wfurt May 5, 2022
@ghost
Copy link

ghost commented May 5, 2022

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

This fixes #37150. I would like to get some feedback before dragging this to API review.

There are actually two separate places where the internals exception would show up. Let's consider

           try {
                Socket s1 = new Socket(SocketType.Stream, ProtocolType.Tcp);
                s1.Connect(IPEndPoint.Parse("127.0.0.1:54321"));
            }
            catch (Exception ex)
            {
                Console.WriteLine(ex);
            }

            try {
                Socket s2 = new Socket(SocketType.Stream, ProtocolType.Tcp);
                s2.ConnectAsync(IPEndPoint.Parse("127.0.0.1:54321")).GetAwaiter().GetResult();
            }
            catch (Exception ex)
            {
                Console.WriteLine(ex);
            }

            var foo = Dns.GetHostAddresses(Guid.NewGuid().ToString("N"));

it would currently produce

System.Net.Internals.SocketExceptionFactory+ExtendedSocketException (111): Connection refused [::ffff:127.0.0.1]:54321
   at System.Net.Sockets.Socket.DoConnect(EndPoint endPointSnapshot, SocketAddress socketAddress)
   at System.Net.Sockets.Socket.Connect(EndPoint remoteEP)
   at Program.Main(String[] args) in /home/furt/projects/exception/Program.cs:line 12

System.Net.Sockets.SocketException (111): Connection refused
   at System.Net.Sockets.Socket.AwaitableSocketAsyncEventArgs.ThrowException(SocketError error, CancellationToken cancellationToken)
   at System.Net.Sockets.Socket.AwaitableSocketAsyncEventArgs.System.Threading.Tasks.Sources.IValueTaskSource.GetResult(Int16 token)
   at System.Threading.Tasks.ValueTask.ValueTaskSourceAsTask.<>c.<.cctor>b__4_0(Object state)
--- End of stack trace from previous location ---
   at Program.Main(String[] args) in /home/furt/projects/exception/Program.cs:line 21

Unhandled exception. System.Net.Internals.SocketExceptionFactory+ExtendedSocketException (00000001, 11): Resource temporarily unavailable
   at System.Net.Dns.GetHostEntryOrAddressesCore(String hostName, Boolean justAddresses, AddressFamily addressFamily, ValueStopwatch stopwatch)
   at System.Net.Dns.GetHostAddresses(String hostNameOrAddress, AddressFamily family)
   at Program.Main(String[] args) in /home/furt/projects/exception/Program.cs:line 28
   at Program.<Main>(String[] args)

In Sockets, this differ for sync vs async. And we get internal exception for DNS errors.

The DNS is easy to fix IMHO and it does not need API change. I can bring that forward if we want to. (fixes issue I hit with #67951)
For the socket exception I was wondering if we should take string instead of EndPoint as we can pass more information to it in the future. However, RemoteEndPoint.ToString() would always allocate but I'm not sure if that matters that much for failures. If We are happy with EndPoint, I can create API proposal and drag it through review.

With this change we get

System.Net.Sockets.SocketException (111): Connection refused [::ffff:127.0.0.1]:54321
   at System.Net.Sockets.Socket.DoConnect(EndPoint endPointSnapshot, SocketAddress socketAddress) in /home/furt/github/wfurt-runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs:line 3116
   at System.Net.Sockets.Socket.Connect(EndPoint remoteEP) in /home/furt/github/wfurt-runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs:line 839
   at Program.Main(String[] args) in /home/furt/projects/exception/Program.cs:line 12

System.Net.Sockets.SocketException (111): Connection refused 127.0.0.1:54321
   at System.Net.Sockets.Socket.AwaitableSocketAsyncEventArgs.ThrowException(SocketError error, CancellationToken cancellationToken) in /home/furt/github/wfurt-runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs:line 1451
   at System.Net.Sockets.Socket.AwaitableSocketAsyncEventArgs.System.Threading.Tasks.Sources.IValueTaskSource.GetResult(Int16 token) in /home/furt/github/wfurt-runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs:line 1365
   at System.Threading.Tasks.ValueTask.ValueTaskSourceAsTask.<>c.<.cctor>b__4_0(Object state) in /home/furt/github/wfurt-runtime/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/ValueTask.cs:line 250
--- End of stack trace from previous location ---
   at Program.Main(String[] args) in /home/furt/projects/exception/Program.cs:line 21

Unhandled exception. System.Net.Sockets.SocketException (00000001, 11): Resource temporarily unavailable
   at System.Net.Dns.GetHostEntryOrAddressesCore(String hostName, Boolean justAddresses, AddressFamily addressFamily, Int64 startingTimestamp) in /home/furt/github/wfurt-runtime/src/libraries/System.Net.NameResolution/src/System/Net/Dns.cs:line 393
   at System.Net.Dns.GetHostAddressesCore(String hostName, AddressFamily addressFamily, Int64 startingTimestamp) in /home/furt/github/wfurt-runtime/src/libraries/System.Net.NameResolution/src/System/Net/Dns.cs:line 374
   at System.Net.Dns.GetHostAddresses(String hostNameOrAddress, AddressFamily family) in /home/furt/github/wfurt-runtime/src/libraries/System.Net.NameResolution/src/System/Net/Dns.cs:line 208
   at System.Net.Dns.GetHostAddresses(String hostNameOrAddress) in /home/furt/github/wfurt-runtime/src/libraries/System.Net.NameResolution/src/System/Net/Dns.cs:line 180
   at Program.Main(String[] args) in /home/furt/projects/exception/Program.cs:line 28
   at Program.<Main>(String[] args)

so both sync & async Connect has same exception type with details and DNS failure is SocketException with same details as before.

Author: wfurt
Assignees: -
Labels:

area-System.Net.Sockets, new-api-needs-documentation
Milestone: -

antonfirsov
antonfirsov reviewed May 5, 2022
View reviewed changes
src/libraries/System.Net.Primitives/src/System/Net/SocketException.cs
// but that's the least bad option right now.
}

public SocketException(int errorCode, EndPoint? endPoint) : this((SocketError)errorCode)
Copy link
Contributor

@antonfirsov antonfirsov May 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think SocketException(int errorCode, string message) would be indeed more future-proof, and feels like a missing API regardless of the current issue, while storing an IPEndpoint feels like an arbitrary decision leading to a bunch of questions: what endpoint? remote? local? why not both? etc.

However, RemoteEndPoint.ToString() would always allocate but I'm not sure if that matters that much for failures.

Exception throwing is already very expensive, and there is a high chance that someone somewhere would log the message anyways, so I would just pass that string in every case.

@karelz
Copy link
Member

karelz commented May 10, 2022

@stephentoub can you please help make the decision on string vs. EndPoint here?

@stephentoub
Copy link
Member

stephentoub commented May 11, 2022

I think it should just take a string. If there's future value in also taking an EndPoint, we can add it in the future, but I doubt that will happen, given that the only reason the implementation takes the EndPoint today is to ToString it.

This was referenced May 12, 2022
Closed
Merged
@wfurt wfurt closed this May 19, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jun 18, 2022
@karelz karelz added this to the 7.0.0 milestone Jul 19, 2022
@wfurt wfurt deleted the socketException branch January 7, 2026 20:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stephentoub stephentoub Awaiting requested review from stephentoub

Copilot code review Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

1 more reviewer

@antonfirsov antonfirsov antonfirsov left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@wfurt wfurt

Labels

area-System.Net.Sockets new-api-needs-documentation

Projects

None yet

Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

System.Net.Internals.SocketExceptionFactory+ExtendedSocketException

4 participants

@wfurt @karelz @stephentoub @antonfirsov