Skip to content
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.

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

Add constructor with string for SocketException #69266

Closed
wfurt opened this issue May 12, 2022 · 6 comments · Fixed by #74744
Closed

Add constructor with string for SocketException #69266

wfurt opened this issue May 12, 2022 · 6 comments · Fixed by #74744
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Net
Milestone

Comments

@wfurt
Copy link
Member

wfurt commented May 12, 2022

Background and motivation

In .NET Framework we used internal constructors to pass additional information (like EndPoint) to SocketException.
For example when Socket.Connect fails we would see something like

SocketException (111): Connection refused 127.0.0.1:54321

This is no longer possible with .NET Core as the SocketException lives in System.Net.Primitives but the use of it comes from System.Net.Sockets and System.Net.NameResolution. So we currently throw derived internal exception like

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)

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)

or we omit additional useful information.

API Proposal

namespace System.Net
{
    public partial class SocketException : System.ComponentModel.Win32Exception
    {
        public SocketException() { }
        public SocketException(int errorCode) { }
+       public SocketException(int errorCode, string? message) { }
}

Alternatives

Instead of string we could add EndPoint as this is only additional information we use. Since we already use EndPoint.ToString(), the string seems more flexible as we may be able to pass in additional info in future.

API Usage

System.Net.Sockets.SocketException (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) 

System.Net.Sockets.SocketException (111): Connection refused 127.0.0.1:54321
   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) 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

System.Net.Sockets.SocketException (00000001, 11): Resource temporarily unavailable
   at System.Net.Dns.GetHostEntryOrAddressesCore(String hostName, Boolean justAddresses, AddressFamily addressFamily, Int64 startingTimestamp)
   at System.Net.Dns.GetHostAddressesCore(String hostName, AddressFamily addressFamily, Int64 startingTimestamp)
   at System.Net.Dns.GetHostAddresses(String hostNameOrAddress, AddressFamily family)
   at System.Net.Dns.GetHostAddresses(String hostNameOrAddress) 

Risks

This is mostly for compatibility and primary use is runtime internals.

PoC and additional discussion is in #68918 and #37150

@wfurt wfurt added area-System.Net api-ready-for-review API is ready for review, it is NOT ready for implementation labels May 12, 2022
@wfurt wfurt self-assigned this May 12, 2022
@ghost ghost added the untriaged New issue has not been triaged by the area owner label May 12, 2022
@ghost
Copy link

ghost commented May 12, 2022

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

Issue Details

Background and motivation

In .NET Framework we used internal constructors to pass additional information (like EndPoint) to SocketException.
For example when Socket.Connect fails we would see something like

SocketException (111): Connection refused 127.0.0.1:54321

This is no longer possible with .NET Core as the SocketException lives in System.Net.Primitives but the use of it comes from System.Net.Sockets and System.Net.NameResolution. So we currently throw derived internal exception like

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)

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)

or we omit additional useful information.

API Proposal

namespace System.Net
{
    public partial class SocketException : System.ComponentModel.Win32Exception
    {
        public SocketException() { }
        public SocketException(int errorCode) { }
+       public SocketException(int errorCode, string? message) { }
}

Alternatives

Instead of string we could add EndPoint as this is only additional information we use. Since we already use EndPoint.ToString(), the string seems more flexible as we may be able to pass in additional info in future.

API Usage

System.Net.Sockets.SocketException (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) 

System.Net.Sockets.SocketException (111): Connection refused 127.0.0.1:54321
   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) 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

System.Net.Sockets.SocketException (00000001, 11): Resource temporarily unavailable
   at System.Net.Dns.GetHostEntryOrAddressesCore(String hostName, Boolean justAddresses, AddressFamily addressFamily, Int64 startingTimestamp)
   at System.Net.Dns.GetHostAddressesCore(String hostName, AddressFamily addressFamily, Int64 startingTimestamp)
   at System.Net.Dns.GetHostAddresses(String hostNameOrAddress, AddressFamily family)
   at System.Net.Dns.GetHostAddresses(String hostNameOrAddress) 

Risks

This is mostly for compatibility and primary use is runtime internals.

PoC and additional discussion is in #68918 and #37150

Author: wfurt
Assignees: wfurt
Labels:

area-System.Net, api-ready-for-review

Milestone: -

@antonfirsov
Copy link
Member

Why is message a nullable string? ? Feels like call-sites which decide to use the new constructor overload should always provide a non-null value.

@wfurt
Copy link
Member Author

wfurt commented May 13, 2022

This is for cases when the source itself is nullable, like the EndPoint in sockets. With the current proposal I can do

throw new SocketException(code, RightEndPoint?.ToString());

What I don't want is something like

if (RightEndPoint != null)
{
   throw new SocketException(code, RightEndPoint.ToString());
}
else
{
   throw new SocketException(code);
}

We could pass around empty string but that also feels weird.

@wfurt wfurt removed the untriaged New issue has not been triaged by the area owner label May 16, 2022
@MihaZupan MihaZupan added this to the 7.0.0 milestone Jun 6, 2022
@karelz
Copy link
Member

karelz commented Jul 19, 2022

This is a long-existing problem and it is late for new APIs in 7.0, moving to 8.0.

@karelz karelz modified the milestones: 7.0.0, 8.0.0 Jul 19, 2022
@terrajobst
Copy link
Member

terrajobst commented Aug 16, 2022

Video

  • Looks good as proposed.
namespace System.Net.Sockets;

public partial class SocketException : Win32Exception
{
    // Existing
    // public SocketException();
    // public SocketException(int errorCode);
    public SocketException(int errorCode, string? message);
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Aug 16, 2022
@liveans
Copy link
Member

liveans commented Aug 27, 2022

Can I implement this, if you don't mind? @wfurt

@karelz karelz assigned liveans and unassigned wfurt Aug 28, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 29, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Sep 7, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Oct 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Net
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants